SQL: Implement CASE... WHEN... THEN... ELSE... END by matriv · Pull Request #41349 · elastic/elasticsearch

matriv · 2019-04-18T15:16:47Z

Implement the ANSI SQL CASE expression which provides the if/else
functionality common to most programming languages.

The CASE expression can have multiple WHEN branches and becomes a
powerful tool for SQL queries as it can be used in SELECT, WHERE,
GROUP BY, HAVING and ORDER BY clauses.

Closes: #36200

Implement the ANSI SQL CASE expression which provides the if/else functionality common to most programming languages. The CASE expression can have multiple WHEN branches and becomes a powerful tool for SQL queries as it can be used in SELECT, WHERE, GROUP BY, HAVING and ORDER BY clauses. Closes: elastic#36200

elasticmachine · 2019-04-18T15:16:50Z

Pinging @elastic/es-search

matriv · 2019-04-18T15:24:53Z

x-pack/plugin/sql/qa/src/main/resources/command.csv-spec

 SUM_OF_SQUARES   |AGGREGATE      
 VAR_POP          |AGGREGATE      
 HISTOGRAM        |GROUPING        
+CASE             |CONDITIONAL


Do you agree with listing CASE as a conditional function?
Case is quite special and doesn't follow at all the traditional functional format.

matriv · 2019-04-18T15:27:47Z

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

+    private final Expression defaultElse;
+
+    @SuppressWarnings("unchecked")
+    public Case(Source source, List<Expression> expressions) {


This "weird" ctor is here as public instead of a more useful (for the ExpressionBuilder) one which would like:
public Case(Source source, List<IfConditional> conditionals, Expression defaultElse) because of FunctionRegistry which gets confused if there are 2 public ctors, and obviously the one with IfConditional cannot be used in the FunctionRegistry.

matriv · 2019-04-19T08:05:30Z

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

+        super(source, expressions);
+        this.conditions = (List<IfConditional>) (List<?>) expressions.subList(0, expressions.size() - 1);
+        this.defaultElse = expressions.get(expressions.size() - 1);
+        setDataType();


will apply the same logic as here: https://github.com/elastic/elasticsearch/pull/41355/files#diff-026c7791c54d1c165a2cd3f516a500beR36 once that PR is merged.

astefan

Nice work! I left some comments.

x-pack/plugin/sql/qa/src/main/resources/conditionals.csv-spec

astefan · 2019-04-19T12:14:31Z

docs/reference/sql/functions/conditional.asciidoc

+
+*Input*:
+
+Multiple but at least one WHEN *condition* THEN *result* clause and optional ELSE *defaultResult* clause.


This sentence sounds weird. Maybe get rid of "Multiple" and rephrase?
At least one _WHEN *condition* THEN *result*_ clause is required and the expression can optionally have an _ELSE *default_result*_ clause. Every *condition* must be boolean expression.

I would rephrase with something like "One or multiple WHEN ... are used and the ..."

astefan · 2019-04-19T12:16:11Z

docs/reference/sql/functions/conditional.asciidoc

+Multiple but at least one WHEN *condition* THEN *result* clause and optional ELSE *defaultResult* clause.
+Every *condition* should be boolean expression.
+
+*Output*: one of the *result* expressions if the corresponding WHEN *condition* evaluates to `true`,


*Output*: one of the *result* expressions if the corresponding _WHEN *condition*_ evaluates to trueor the *default_result* if all _WHEN *condition*_ clauses evaluate tofalse. If the optional _ELSE *default_result*_ clause is missing and all _WHEN *condition*_ clauses evaluate to falsethennull is returned.

astefan · 2019-04-19T15:09:25Z

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

+                Case c = (Case) e;
+
+                // Remove or foldable conditions that fold to FALSE
+                // Stop at the 1st foldable condition taht folds to TRUE


Typo: that.

astefan · 2019-04-19T15:22:09Z

...ql/src/test/java/org/elasticsearch/xpack/sql/expression/predicate/conditional/CaseTests.java

+
+    @Override
+    protected Case mutate(Case instance) {
+        return randomCase();


I don't think this mutate logic will always have a changed, non-equal to original, instance of CASE. I think the mutation should have as many IfConditional instances as constructor values that can change (2 in this case with randomStringLiteral()). And from Case point of view, it will also need to mutate the randomIntLiteral() parameter as well. Something like this: https://github.com/elastic/elasticsearch/blob/master/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/expression/function/scalar/string/ConcatFunctionPipeTests.java#L96

astefan · 2019-04-19T15:23:14Z

...ql/src/test/java/org/elasticsearch/xpack/sql/expression/predicate/conditional/CaseTests.java

+    }
+
+    @Override
+    public void testTransform() {


Why is this one empty? Same for testReplaceChildren.

I added this Class as a quick fix to the failing NodeSubclassTests (because of the special IfConditional as an expression) and forgot to properly implement everything there.

Fixed.

astefan · 2019-04-19T15:32:08Z

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

+
+    @Override
+    protected TypeResolution resolveType() {
+        DataType expectedResultDataType = null;


Why not initializing expectedResultDataType directly with defaultElse().dataType()?

Because we want to match the results of the THEN <result> clauses to the 1st non-null result of them.

Wouldn't that happen anyway in the following loop (that checks the conditions)?

astefan · 2019-04-19T15:36:22Z

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

+        }
+
+        for (IfConditional conditional : conditions) {
+            dataType = DataTypeConversion.commonType(dataType, conditional.dataType());


In case conditions.isEmpty() == false the first commonType check is with itself, isn't it? Maybe worth doing a for with an index that starts either at 0 or 1 depending on the previous isEmpty() check?

Indeed, I would just start with the defaultElse() type and then iterate over the conditions - this avoid repetition.

astefan · 2019-04-19T15:44:30Z

x-pack/plugin/sql/qa/src/main/resources/conditionals.csv-spec

+SELECT emp_no, CASE WHEN emp_no - 10000 < 10 THEN 'First 10' ELSE 'Second 10' END as "case" FROM test_emp WHERE emp_no >= 10005
+ORDER BY emp_no LIMIT 10;
+
+ emp_no |   case


Following this PR - #41355 - can you change one of these tests to not have an alias?

costin

Looks good. This feature will make a lot of folks happy.

costin · 2019-04-19T15:40:29Z

docs/reference/sql/functions/conditional.asciidoc

+
+*Input*:
+
+Multiple but at least one WHEN *condition* THEN *result* clause and optional ELSE *defaultResult* clause.


I would rephrase with something like "One or multiple WHEN ... are used and the ..."

costin · 2019-04-19T16:01:45Z

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

+    @Override
+    public boolean foldable() {
+        return defaultElse.foldable() && (conditions.isEmpty() ||
+            (conditions.size() == 1 && conditions.get(0).condition().foldable() && conditions.get(0).result().foldable()));


I'm not sure this method is correct - why is the defaultElse checked first - if a condition is foldable and true, defaultElse is not relevant.
I think it should be:

return (conditions.isEmpty() && defaultElse.foldable()) || (conditions.size() > 0 && conditions.get(0).condition().foldable());

costin · 2019-04-19T16:06:02Z

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

+            return defaultElse.fold();
+        }
+        if (conditions.get(0).condition().fold() == Boolean.TRUE) {
+            return conditions.get(0).result().fold();


The defaultElse.fold() repetition should be avoided:

if (conditions.size() > 0 && conditions.get(0).condition().fold() == Boolean.TRUE) { ... } return defaultElse.fold();

costin · 2019-04-19T16:07:12Z

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

+        }
+
+        for (IfConditional conditional : conditions) {
+            dataType = DataTypeConversion.commonType(dataType, conditional.dataType());


Indeed, I would just start with the defaultElse() type and then iterate over the conditions - this avoid repetition.

costin · 2019-04-19T16:08:34Z

...rc/main/java/org/elasticsearch/xpack/sql/expression/predicate/conditional/IfConditional.java

+import java.util.Objects;
+
+/**
+ * Helper function (cannot be directly from a query) to model a {@code WHEN <condition> ELSE <result>}


Considering this does not extend Function I wouldn't call it a Helper function - instead a conditional expression mapping WHEN ELSE backing CASE and potentially IIF

costin · 2019-04-19T16:09:20Z

...rc/main/java/org/elasticsearch/xpack/sql/expression/predicate/conditional/IfConditional.java

+        this(source, Arrays.asList(condition, result));
+    }
+
+    private IfConditional(Source source, List<Expression> children) {


Why is this constructor needed?

No reason after all, it's a leftover from a different design. changing it.

costin · 2019-04-19T16:09:58Z

...rc/main/java/org/elasticsearch/xpack/sql/expression/predicate/conditional/IfConditional.java

+    private final Expression result;
+
+    public IfConditional(Source source, Expression condition, Expression result) {
+        this(source, Arrays.asList(condition, result));


this(source, asList(condition,result)); this.condition = condition; this.result = result;

costin · 2019-04-19T16:10:24Z

...rc/main/java/org/elasticsearch/xpack/sql/expression/predicate/conditional/IfConditional.java

+        if (newChildren.size() < 2) {
+            throw new IllegalArgumentException("expected at least [2] children but received [" + newChildren.size() + "]");
+        }
+        return new IfConditional(source(), newChildren);


IfConditional(source(), newChildren.get(0), newChildren(1));

matriv · 2019-04-22T11:45:04Z

@costin @astefan Thank you for the review.
Please check again, hope didn't miss anything.

astefan

LGTM

costin

LGTM

costin · 2019-04-22T12:59:40Z

docs/reference/sql/functions/conditional.asciidoc

+*Input*:
+
+One or multiple _WHEN *condition* THEN *result_* clauses are used and the expression can optionally have
+an _ELSE *default_result_* clause. Every *condition* should be boolean expression.


should be a boolean expression

Implement the ANSI SQL CASE expression which provides the if/else functionality common to most programming languages. The CASE expression can have multiple WHEN branches and becomes a powerful tool for SQL queries as it can be used in SELECT, WHERE, GROUP BY, HAVING and ORDER BY clauses. Closes: #36200 (cherry picked from commit 8b25774)

Add a TIP on how to use CASE to achieve custom bucketing with GROUP BY. Follows: elastic#41349

* SQL: [Docs] Add example for custom bucketing with CASE Add a TIP on how to use CASE to achieve custom bucketing with GROUP BY. Follows: #41349 * address comments * address comment

Add a TIP on how to use CASE to achieve custom bucketing with GROUP BY. Follows: #41349 (cherry picked from commit eb5f5d4)

Implement the ANSI SQL CASE expression which provides the if/else functionality common to most programming languages. The CASE expression can have multiple WHEN branches and becomes a powerful tool for SQL queries as it can be used in SELECT, WHERE, GROUP BY, HAVING and ORDER BY clauses. Closes: elastic#36200

* SQL: [Docs] Add example for custom bucketing with CASE Add a TIP on how to use CASE to achieve custom bucketing with GROUP BY. Follows: elastic#41349 * address comments * address comment

matriv added >feature WIP :Analytics/SQL SQL querying v8.0.0 v7.2.0 labels Apr 18, 2019

matriv commented Apr 18, 2019

View reviewed changes

remove empty lines

1f17aca

matriv commented Apr 19, 2019

View reviewed changes

Added integration tests

0a53fb6

matriv requested review from astefan and costin April 19, 2019 09:34

fix test

1bd870f

matriv removed the WIP label Apr 19, 2019

Fixed an issue with types verification and enhanced integ tests

83dc691

astefan reviewed Apr 19, 2019

View reviewed changes

costin reviewed Apr 19, 2019

View reviewed changes

matriv added 3 commits April 22, 2019 11:17

Merge remote-tracking branch 'upstream/master' into impl-36200

365f135

address comments pt1

1853714

address comments pt2

a791a84

matriv requested review from astefan and costin April 22, 2019 11:44

fixed docs

5793884

astefan approved these changes Apr 22, 2019

View reviewed changes

costin approved these changes Apr 22, 2019

View reviewed changes

fix docs

fda4180

matriv and others added 2 commits April 22, 2019 16:36

Merge remote-tracking branch 'upstream/master' into impl-36200

94d5a60

add missing ';'

7527f49

matriv merged commit 8b25774 into elastic:master Apr 22, 2019

matriv deleted the impl-36200 branch April 22, 2019 16:26

matriv added a commit to matriv/elasticsearch that referenced this pull request May 3, 2019

SQL: [Docs] Add example for custom bucketing with CASE

795a945

Add a TIP on how to use CASE to achieve custom bucketing with GROUP BY. Follows: elastic#41349

matriv mentioned this pull request May 3, 2019

SQL: [Docs] Add example for custom bucketing with CASE #41787

Merged

matriv added a commit that referenced this pull request May 6, 2019

SQL: [Docs] Add example for custom bucketing with CASE (#41787)

228d23d

Add a TIP on how to use CASE to achieve custom bucketing with GROUP BY. Follows: #41349 (cherry picked from commit eb5f5d4)

astefan mentioned this pull request Jun 3, 2019

SQL: consider adding conditional expression support #32984

Closed

jakelandis added v8.0.0-alpha1 and removed v8.0.0 labels Jul 26, 2021


		Input:

		Multiple but at least one WHEN condition THEN result clause and optional ELSE defaultResult clause.

Conversation

matriv commented Apr 18, 2019

Uh oh!

elasticmachine commented Apr 18, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

astefan left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

costin left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

matriv commented Apr 22, 2019

Uh oh!

astefan left a comment

Choose a reason for hiding this comment

Uh oh!

costin left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development