SQL: Implement GREATEST and LEAST functions #35879

matriv · 2018-11-25T01:10:31Z

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

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

elasticmachine · 2018-11-25T01:10:32Z

Pinging @elastic/es-search

matriv · 2018-11-25T01:11:41Z

x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/planner/QueryFolder.java

@@ -140,9 +139,6 @@ protected PhysicalPlan rule(ProjectExec project) {
                        if (pj instanceof ScalarFunction) {
                            ScalarFunction f = (ScalarFunction) pj;
                            processors.put(f.toAttribute(), Expressions.pipe(f));
-                        } else if (pj instanceof In) {


Leftover: In is already extending ScalarFunction.

costin

Looks good however the optimizer rule could be improved as well as the class hierarchy.

costin · 2018-11-25T19:15:58Z

...src/main/java/org/elasticsearch/xpack/sql/expression/predicate/conditional/Conditionals.java

+        return least(values);
+    }
+
+    private static Object getMinOrMax(Collection<Object> values, BiFunction<Object, Object, Boolean> comparison) {


Since Min and Max are the actual functions, extremum seems like a better name.

Also can a type (Number) be inferred to the function instead of Object`?

But the arguments can also be Strings.

costin · 2018-11-25T19:17:25Z

...sql/src/main/java/org/elasticsearch/xpack/sql/expression/predicate/conditional/Greatest.java

+
+    @Override
+    public Object fold() {
+        LinkedHashSet<Object> values = new LinkedHashSet<>(children().size());


Looks like a candidate for Foldables.

costin · 2018-11-25T19:18:41Z

...sql/src/main/java/org/elasticsearch/xpack/sql/expression/predicate/conditional/Greatest.java

+    }
+
+    @Override
+    protected String scriptMethodName() {


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.

costin · 2018-11-25T19:24:08Z

x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/optimizer/Optimizer.java

@@ -1229,6 +1232,27 @@ protected Expression rule(Expression e) {
                    return new Coalesce(c.location(), newChildren);
                }
            }
+
+            if (e instanceof Greatest || e instanceof Least) {


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).

costin · 2018-11-25T19:25:59Z

...n/sql/src/main/java/org/elasticsearch/xpack/sql/expression/predicate/conditional/NullIf.java

@@ -68,6 +68,11 @@ public Object fold() {
        return NullIfProcessor.apply(children().get(0).fold(), children().get(1).fold());
    }

+    @Override
+    protected String scriptMethodName() {


?
This is typically an indication that the parent is not generic enough.

costin · 2018-11-25T19:27:51Z

.../java/org/elasticsearch/xpack/sql/expression/predicate/conditional/ConditionalProcessor.java

+
+    public enum ConditionalOperation implements Function<Collection<Object>, Object> {
+
+        COALESCE(Conditionals::coalesce, Conditionals::coalesceOnInput),


Looks like coalesce/greatest/least should have a common parent more specialized then the others conditionals.

matriv · 2018-11-26T14:42:12Z

@costin Thanks for the review. Pushed commits to address comments.

costin

Left some minor polishing comments.
LGTM!

costin · 2018-11-26T14:53:35Z

x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/Foldables.java

+        return foldAndAdd(list, to, new LinkedHashSet<>(list.size()));
+    }
+
+    private static <T, C extends Collection<T>> C foldAndAdd(Collection<Expression> expressions, DataType to, C values) {


Please rename it to foldTo - I was thinking the values are folded and added together based on the current name.

costin · 2018-11-26T14:54:07Z

...g/elasticsearch/xpack/sql/expression/predicate/conditional/ArbitraryConditionalFunction.java

+ */
+public abstract class ArbitraryConditionalFunction extends ConditionalFunction {
+
+    private ConditionalOperation operation;


costin · 2018-11-26T14:55:34Z

...src/main/java/org/elasticsearch/xpack/sql/expression/predicate/conditional/Conditionals.java

+        return null;
+    }
+
+    static Object coalesceOnInput(List<Processor> processors, Object input) {


The On is superfluous. - coalesceInput, greatestInput, etc...

astefan

LGTM. Left one question only.

astefan · 2018-11-26T15:31:11Z

...src/main/java/org/elasticsearch/xpack/sql/expression/predicate/conditional/Conditionals.java

+            if ((i == 0) || (result == null) || (comparison.apply(value, result) == Boolean.TRUE)) {
+                result = value;
+            }
+            i++;


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

matriv · 2018-11-26T17:37:42Z

Backported to 6.x with 44d6c74

matriv added >enhancement v7.0.0 :Analytics/SQL SQL querying v6.6.0 labels Nov 25, 2018

matriv requested review from costin and astefan November 25, 2018 01:10

matriv commented Nov 25, 2018

View reviewed changes

costin requested changes Nov 25, 2018

View reviewed changes

matriv added 3 commits November 26, 2018 14:24

Address comments

35cc812

Merge remote-tracking branch 'upstream/master' into mt/impl-35878

2fc8fa2

Address comments

fe8133a

costin approved these changes Nov 26, 2018

View reviewed changes

Address comments

4c116a9

astefan approved these changes Nov 26, 2018

View reviewed changes

Adress comment

1e607f3

matriv merged commit 3f7cae3 into elastic:master Nov 26, 2018

matriv deleted the mt/impl-35878 branch November 26, 2018 17:21

colings86 added v7.0.0-beta1 and removed v7.0.0 labels Feb 7, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

SQL: Implement GREATEST and LEAST functions #35879

SQL: Implement GREATEST and LEAST functions #35879

matriv commented Nov 25, 2018

elasticmachine commented Nov 25, 2018

matriv Nov 25, 2018

costin left a comment

costin Nov 25, 2018

costin Nov 25, 2018

matriv Nov 26, 2018

costin Nov 25, 2018

costin Nov 25, 2018

costin Nov 25, 2018

costin Nov 25, 2018

costin Nov 25, 2018

matriv commented Nov 26, 2018

costin left a comment

costin Nov 26, 2018

costin Nov 26, 2018

costin Nov 26, 2018

astefan left a comment

astefan Nov 26, 2018

matriv commented Nov 26, 2018


		public enum ConditionalOperation implements Function<Collection<Object>, Object> {

		COALESCE(Conditionals::coalesce, Conditionals::coalesceOnInput),

SQL: Implement GREATEST and LEAST functions #35879

SQL: Implement GREATEST and LEAST functions #35879

Conversation

matriv commented Nov 25, 2018

elasticmachine commented Nov 25, 2018

Choose a reason for hiding this comment

costin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

matriv commented Nov 26, 2018

costin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

astefan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

matriv commented Nov 26, 2018