SQL: Implement GREATEST and LEAST functions#35879
Conversation
Add GREATEST(expr1, expr2, ... exprN) and LEAST(expr1, expr2, exprN) functions which are in the family of CONDITIONAL functions. Implementation follows PostgreSQL behaviour, so the functions return `NULL` when all of their arguments evaluate to `NULL`. Renamed `CoalescePipe` and `CoalesceProcessor` to `ConditionalPipe` and `ConditionalProcessor` respectively, to be able to reuse them for `Greatest` and `Least` evaluations. To achieve that `ConditionalOperation` has been added to differentiate between the functionalities at execution time. Closes: elastic#35878
|
Pinging @elastic/es-search |
| if (pj instanceof ScalarFunction) { | ||
| ScalarFunction f = (ScalarFunction) pj; | ||
| processors.put(f.toAttribute(), Expressions.pipe(f)); | ||
| } else if (pj instanceof In) { |
There was a problem hiding this comment.
Leftover: In is already extending ScalarFunction.
costin
left a comment
There was a problem hiding this comment.
Looks good however the optimizer rule could be improved as well as the class hierarchy.
| return least(values); | ||
| } | ||
|
|
||
| private static Object getMinOrMax(Collection<Object> values, BiFunction<Object, Object, Boolean> comparison) { |
There was a problem hiding this comment.
Since Min and Max are the actual functions, extremum seems like a better name.
There was a problem hiding this comment.
Also can a type (Number) be inferred to the function instead of Object`?
There was a problem hiding this comment.
But the arguments can also be Strings.
|
|
||
| @Override | ||
| public Object fold() { | ||
| LinkedHashSet<Object> values = new LinkedHashSet<>(children().size()); |
There was a problem hiding this comment.
Looks like a candidate for Foldables.
| } | ||
|
|
||
| @Override | ||
| protected String scriptMethodName() { |
There was a problem hiding this comment.
Isn't it possible to move this and the method below to the parent?
Make the function accept a ConditionalOperation and invoke its scriptMethodName and create a ConditionalPipe for it.
| } | ||
| } | ||
|
|
||
| if (e instanceof Greatest || e instanceof Least) { |
There was a problem hiding this comment.
The logic is exactly that of Coalesce so simply do a check for Greatest, Least and Coalesce - maybe they have a common parent class so use that then after filtering the children simply call expression.replaceChildren(newChildren).
| } | ||
|
|
||
| @Override | ||
| protected String scriptMethodName() { |
There was a problem hiding this comment.
?
This is typically an indication that the parent is not generic enough.
|
|
||
| public enum ConditionalOperation implements Function<Collection<Object>, Object> { | ||
|
|
||
| COALESCE(Conditionals::coalesce, Conditionals::coalesceOnInput), |
There was a problem hiding this comment.
Looks like coalesce/greatest/least should have a common parent more specialized then the others conditionals.
|
@costin Thanks for the review. Pushed commits to address comments. |
costin
left a comment
There was a problem hiding this comment.
Left some minor polishing comments.
LGTM!
| return foldAndAdd(list, to, new LinkedHashSet<>(list.size())); | ||
| } | ||
|
|
||
| private static <T, C extends Collection<T>> C foldAndAdd(Collection<Expression> expressions, DataType to, C values) { |
There was a problem hiding this comment.
Please rename it to foldTo - I was thinking the values are folded and added together based on the current name.
| */ | ||
| public abstract class ArbitraryConditionalFunction extends ConditionalFunction { | ||
|
|
||
| private ConditionalOperation operation; |
| return null; | ||
| } | ||
|
|
||
| static Object coalesceOnInput(List<Processor> processors, Object input) { |
There was a problem hiding this comment.
The On is superfluous. - coalesceInput, greatestInput, etc...
astefan
left a comment
There was a problem hiding this comment.
LGTM. Left one question only.
| if ((i == 0) || (result == null) || (comparison.apply(value, result) == Boolean.TRUE)) { | ||
| result = value; | ||
| } | ||
| i++; |
There was a problem hiding this comment.
I think here the i logic can be replaced by a boolean? Seems to be a true/false scenario.
Add GREATEST(expr1, expr2, ... exprN) and LEAST(expr1, expr2, exprN) functions which are in the family of CONDITIONAL functions. Implementation follows PostgreSQL behaviour, so the functions return `NULL` when all of their arguments evaluate to `NULL`. Renamed `CoalescePipe` and `CoalesceProcessor` to `ConditionalPipe` and `ConditionalProcessor` respectively, to be able to reuse them for `Greatest` and `Least` evaluations. To achieve that `ConditionalOperation` has been added to differentiate between the functionalities at execution time. Closes: #35878
|
Backported to |
Add GREATEST(expr1, expr2, ... exprN) and LEAST(expr1, expr2, exprN)
functions which are in the family of CONDITIONAL functions.
Implementation follows PostgreSQL behaviour, so the functions return
NULLwhen all of their arguments evaluate toNULL.Renamed
CoalescePipeandCoalesceProcessortoConditionalPipeandConditionalProcessorrespectively, to be able to reuse them forGreatestandLeastevaluations. To achieve thatConditionalOperationhas been added to differentiate between the functionalities at execution
time.
Closes: #35878