ESQL: Fix CASE when conditions are multivalued #112401

nik9000 · 2024-08-30T16:28:41Z

When CASE hits a multivalued field it was previously either crashing on fold or evaluating it to the first value. Since booleans are loaded in sorted order from lucene that usually means false. This changes the behavior to line up with the rest of ESQL - now multivalued fields are treated as false with a warning.

You might say "hey wait! multivalued fields usually become null, not false!". Yes, dear reader, you are right. Very right. But! CASE's contract is to immediatly convert its values into true or false using the standard boolean tri-valued logic. So null just become false immediately. This is how PostgreSQL, MySQL, and SQLite behave:

> SELECT CASE WHEN null THEN 1 ELSE 2 END;
2

They turn that null into a false. And we're right there with them. Except, of course, that we're turning [false, false] and the like into null first. See!? It's consitent. Consistently confusing, but sane at least.

The warning message just says "treating multivalued field as false" rather than explaining all of that.

This also fixes up a few of CASE's docs which I noticed were kind of busted while working on CASE. I think the docs generation is having a lot of trouble with CASE so I've manually hacked the right thing into place, but we should figure out a better solution eventually.

Closes #112359

When CASE hits a multivalued field it was previously either crashing on fold or evaluating it to the first value. Since booleans are loaded in sorted order from lucene that *usually* means `false`. This changes the behavior to line up with the rest of ESQL - now multivalued fields are treated as `false` with a warning. You might say "hey wait! multivalued fields usually become `null`, not `false`!". Yes, dear reader, you are right. Very right. But! `CASE`'s contract is to immediatly convert its values into `true` or `false` using the standard boolean tri-valued logic. So `null` just become `false` immediately. This is how PostgreSQL, MySQL, and SQLite behave: ``` > SELECT CASE WHEN null THEN 1 ELSE 2 END; 2 ``` They turn that `null` into a false. And we're right there with them. Except, of course, that we're turning `[false, false]` and the like into `null` first. See!? It's consitent. Consistently confusing, but sane at least. The warning message just says "treating multivalued field as false" rather than explaining all of that. This also fixes up a few of CASE's docs which I noticed were kind of busted while working on CASE. I think the docs generation is having a lot of trouble with CASE so I've manually hacked the right thing into place, but we should figure out a better solution eventually. Closes elastic#112359

github-actions · 2024-08-30T16:28:52Z

Documentation preview:

✨ Changed pages

elasticsearchmachine · 2024-08-30T16:29:04Z

Pinging @elastic/es-analytical-engine (Team:Analytics)

elasticsearchmachine · 2024-08-30T16:29:04Z

Hi @nik9000, I've created a changelog YAML for you.

…p_case

nik9000 · 2024-08-30T16:34:54Z

x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/expression/function/Warnings.java

     * Create a new warnings object based on the given mode
     * @param warningsMode The warnings collection strategy to use
-     * @param source used to indicate where in the query the warning occured
+     * @param source used to indicate where in the query the warning occurred


I've given us a little more flexibility, specifically so I can tell folks that multivalues in CASE default to false. If it's confusing why we do that read the PR description. Er, well, it's still confusing a bit. But that tries to explain it.

.../src/main/java/org/elasticsearch/xpack/esql/expression/function/scalar/conditional/Case.java

nik9000 · 2024-08-30T16:43:35Z

.../src/main/java/org/elasticsearch/xpack/esql/expression/function/scalar/conditional/Case.java

-                    + ", elseVal="
-                    + elseValueFactory
-                    + ']';
+                return "CaseEvaluator[conditions=" + conditionsFactories + ", elseVal=" + elseValueFactory + ']';


This was just making testing harder and doesn't really provide any debug value.

nik9000 · 2024-08-30T16:44:26Z

.../src/main/java/org/elasticsearch/xpack/esql/expression/function/scalar/conditional/Case.java

+                                        new IllegalArgumentException("CASE expects a single-valued boolean")
+                                    );
+                                    continue;
+                                }


Here's the core of the fix! It's fun how it's like 5 lines of fix, 200 testing, 200 ceremony to plumb the fix properly, and another 200 of docs updates.

nik9000 · 2024-08-30T16:44:43Z

...src/test/java/org/elasticsearch/xpack/esql/expression/function/AbstractFunctionTestCase.java


        List<Set<String>> typesFromSignature = new ArrayList<>();
-        Set<String> returnFromSignature = new HashSet<>();
+        Set<String> returnFromSignature = new TreeSet<>();


Makes comparing the errors easier because they are in alphabetical order....

nik9000 · 2024-08-30T16:45:45Z

...gin/esql/src/test/java/org/elasticsearch/xpack/esql/expression/function/RailRoadDiagram.java

+            // CASE is so weird let's just hack this together manually
+            Sequence seq = new Sequence(new Literal("condition"), new Syntax(","), new Literal("trueValue"));
+            expressions.add(new Repetition(seq, 1, null));
+            expressions.add(new Repetition(new Literal("elseValue"), 0, 1));


I think CASE is the only function that has this funny signature. It's actually pretty helpful to see the railroad diagram for CASE too!

nik9000 · 2024-08-30T16:46:24Z

...test/java/org/elasticsearch/xpack/esql/expression/function/scalar/conditional/CaseTests.java

+            DataType.GEO_POINT,
+            DataType.CARTESIAN_SHAPE,
+            DataType.GEO_SHAPE
+        )) {


Previously these were all written by hand, but we know a lot more about how to write these now.

alex-spies

I'll have a second go at this tomorrow; I agree with the proposed semantics of CASE when presented with multi-valued booleans.

@luigidellaquila: I think CASE is something where users would profit from keywords like ANY and ALL. I think it's reasonable to have a multi-valued field and wanting to express something like CASE (ANY multivalued_field, 1, 0) or CASE(ALL multivalued_field, 1, 0); without the warning that this PR adds, it could look like CASE already does it, so it's good we are being explicit about it in the warning.

alex-spies · 2024-09-02T15:40:15Z

x-pack/plugin/esql/qa/testFixtures/src/main/resources/conditional.csv-spec

Nice tests!

I'd add one with ROW foo = CASE([true, true], ... just to make it obvious that CASE doesn't have implicit ANY or ALL logic.

Should we add ROW a = [true, false] | EVAL c = CASE(MV_SLICE(a, 0, 2), "foo", "bar") as well? This one failed differently from others, so I guess it exercised a different code path? A test case with an mv expression inside CASE is probably a good idea, anyway. Or, similarly but more realistically, using a conversion function like ROW a = ["true", "false"] | EVAL c = CASE(a::boolean, "foo", "bar")

If we want to be supremely paranoid, we could also check the interaction with multivalued union types, e.g. FROM idx* | EVAL x = CASE(field::boolean, 1, 2); I'd only do that if we expect weird interactions with block loading, though, which probably is not the case.

Generally if you ask "should we add this csv-spec test?" the answer is just yes. It's cheap.

alex-spies

Thanks Nik, this is great. I think there's a bug in Case.foldable which we should fix before merging, otherwise my remarks are rather minor (suggesting more tests, as is becoming tradition :D ).

alex-spies · 2024-09-03T08:05:36Z

docs/reference/esql/functions/types/case.asciidoc

Thanks for getting this up to date!

alex-spies · 2024-09-03T08:10:54Z

x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/expression/function/Warnings.java

-            }
-        }
-        throw new IllegalStateException("Unreachable");
+        return createWarnings(warningsMode, source, "treating result as null");


nit: to avoid drift, we could put treating result as null into a string constant.

alex-spies · 2024-09-03T08:24:46Z

.../src/main/java/org/elasticsearch/xpack/esql/expression/function/scalar/conditional/Case.java

+            if (o instanceof List) {
+                /*
+                 * multivalued fields fold to null which folds to false.
+                 * So they *are* foldable if the value is foldable.
+                 */
                return condition.value.foldable();


I think this is wrong, and it's inconsistent with partiallyFold.

We special case for a MV - in which case we know that the value will never be evaluated, so we probably should continue, as we'll fall down into the next condition.

I think this prevents situations where ConstantFolding could already fully trivialize a Case and bubble that further up in the expression; instead it'll take another run of the optimizer rules because we need PartiallyFoldCase to do the job here.

Since the foldability is less than trivial for CASE, I think we should add one or two cases to the CaseExtraTests and check if Case.foldable() returns correct results. Or, insideLogicalPlanOptimizerTests , create a logical plan with a CASE and only run ConstantFolding, resp. only PartiallyFoldCase, on it, rather than the whole optimizer; currently, PartiallyFoldCase jumps to the rescue, even if ConstantFolding can't do its job correctly, so we don't see this bug take effect.

Can the list be empty? if so, is that the same as false?

I don't think lists occur unless they have at least 2 elements.

I think the intention is to treat any multivalue the same as false, which I think is correct, but the implementation in these lines is a bug.

More interestingly, there could be lists with 1 element which is true. I'd say that's a bug because that should simply be a boolean literal, but I'm not sure we never create such literals, either.

Empty lists can't occur, not. If it's empty it's null instead.

alex-spies · 2024-09-03T09:03:43Z

.../src/main/java/org/elasticsearch/xpack/esql/expression/function/scalar/conditional/Case.java

+            Object o = condition.condition.fold();
+            if (o instanceof List) {
+                // multivalued field folds to null which folds to false
+                continue;
+            }


Maybe we should use a helper method (or two) on Condition to tell us whether the condition is always true/false? That could be reused in Case.foldable and would avoid the foldability logic to drift apart.

alex-spies · 2024-09-03T09:16:04Z

.../src/main/java/org/elasticsearch/xpack/esql/expression/function/scalar/conditional/Case.java

+                 * We treat failures as null just like any other failure.
+                 * It's just that we then *immediately* convert it to
+                 * true or false using the tri-valued boolean logic stuff.
+                 * And that makes it into false. This is, *exactly* what
+                 * happens in PostgreSQL and MySQL and SQLite:
+                 * > SELECT CASE WHEN null THEN 1 ELSE 2 END;
+                 * 2
+                 * Rather than go into depth about this in the warning message,
+                 * we just say "false".


CC @costin and @astefan : Nik is nicely making the MV semantics for CASE consistent.

Just FYI because we could also choose to define CASE([true, true], ... ) == null similarly to functions, which applied to multivalues just become null. I think Nik's approach is more useful, because making CASE on multivalues always return null would probably also imply that CASE(true, a, [true, true], b) should return null, even though it doesn't matter that the second condition is multivalued.

I think Nik's approach is more useful

It's also the same approach as PostgreSQL.

I don't think Case makes the equivalence of null == false rather it checks if the condition is true.
That is SELECT CASE WHEN null THEN 1 ELSE 2 END returns 2 because the condition null is not true.
It's a subtle difference but it means CASE(true, a, [true, true], b) should return a because the condition is true so the else branch is not even evaluated.

alex-spies · 2024-09-03T09:33:55Z

...src/test/java/org/elasticsearch/xpack/esql/expression/function/AbstractFunctionTestCase.java

        FunctionDefinition definition = definition(name);
        if (definition != null) {
            EsqlFunctionRegistry.FunctionDescription description = EsqlFunctionRegistry.description(definition);
+            if (name.equals("case")) {


nit: Should we use a constant for case here and in RailRoadDiagram.java? Maybe also in the function registry?

alex-spies · 2024-09-03T09:37:46Z

...java/org/elasticsearch/xpack/esql/expression/function/scalar/conditional/CaseExtraTests.java

+    public void testPartialFoldMv() {
+        Case c = new Case(
+            Source.synthetic("case"),
+            new Literal(Source.EMPTY, List.of(true, true), DataType.BOOLEAN),


Maybe let's randomize the booleans and the number of MVs?

Maybe let's also consider cases with multiple multivalued conditions, not just one?

Also maybe let's add cases where partiallyFold can fully fold, and returns the same as fold?

The first two I'm doing now. The last one I'll have a think about. I think I'd like to make a method to test partiallFold inside CaseTests so it can use the test cases.

I've added a bunch of cases.

alex-spies · 2024-09-03T09:45:38Z

...test/java/org/elasticsearch/xpack/esql/expression/function/scalar/conditional/CaseTests.java

+            suppliers.add(threeArgCase(null, false, type, List.of()));
+            suppliers.add(
+                threeArgCase(
+                    List.of(true, true),


nit: Let's randomize?

alex-spies · 2024-09-03T09:47:28Z

...test/java/org/elasticsearch/xpack/esql/expression/function/scalar/conditional/CaseTests.java

A bit out of scope, but: shouldn't we also at least test the 4 and 5 parameter cases?

Yeah. We get those tests from the Extra tests right now. Not great.

costin

I might be overlooking some implementation details however I would focus on defining the contract of Case as the condition having to be a single-value true; anything else (multi value, false, null, etc...) would cause the else branch to be evaluated which would repeat the above process.
There's a question on whether we should give a warning when checking for true - which we could add (better be safe than sorry) however to your point, this is not about null == false rather about the value not being true.

costin · 2024-09-03T22:41:30Z

.../src/main/java/org/elasticsearch/xpack/esql/expression/function/scalar/conditional/Case.java

+            if (o instanceof List) {
+                /*
+                 * multivalued fields fold to null which folds to false.
+                 * So they *are* foldable if the value is foldable.
+                 */
                return condition.value.foldable();


Can the list be empty? if so, is that the same as false?

costin · 2024-09-03T22:47:29Z

.../src/main/java/org/elasticsearch/xpack/esql/expression/function/scalar/conditional/Case.java

+                 * We treat failures as null just like any other failure.
+                 * It's just that we then *immediately* convert it to
+                 * true or false using the tri-valued boolean logic stuff.
+                 * And that makes it into false. This is, *exactly* what
+                 * happens in PostgreSQL and MySQL and SQLite:
+                 * > SELECT CASE WHEN null THEN 1 ELSE 2 END;
+                 * 2
+                 * Rather than go into depth about this in the warning message,
+                 * we just say "false".


I don't think Case makes the equivalence of null == false rather it checks if the condition is true.
That is SELECT CASE WHEN null THEN 1 ELSE 2 END returns 2 because the condition null is not true.
It's a subtle difference but it means CASE(true, a, [true, true], b) should return a because the condition is true so the else branch is not even evaluated.

.../src/main/java/org/elasticsearch/xpack/esql/expression/function/scalar/conditional/Case.java

nik9000

@alex-spies wanted to have another look at this one with all the new tests.

alex-spies

Hey Nik, don't wait for another review from me! I was just curious about the added testing that you showed me the other day, but you had my thumbs-up already before that :)

alex-spies · 2024-09-06T14:25:59Z

.../src/main/java/org/elasticsearch/xpack/esql/expression/function/scalar/conditional/Case.java

            }
-            Object o = condition.condition.fold();
-            if (o instanceof List) {
+            if (Boolean.TRUE.equals(condition.condition.fold())) {


alex-spies · 2024-09-06T14:28:41Z

.../src/main/java/org/elasticsearch/xpack/esql/expression/function/scalar/conditional/Case.java

+            if (Boolean.TRUE.equals(condition.condition.fold())) {
                /*
                 * multivalued fields fold to null which folds to false.
                 * So they *are* foldable if the value is foldable.


I think the comment here means the converse.

Suggested change

* So they *are* foldable if the value is foldable.

* So they *are* foldable if remaining list of conditions is foldable.

Same in line 271.

nik9000 added >bug :Analytics/ES|QL AKA ESQL v8.16.0 labels Aug 30, 2024

elasticsearchmachine added the Team:Analytics Meta label for analytical engine team (ESQL/Aggs/Geo) label Aug 30, 2024

Update docs/changelog/112401.yaml

ecf9598

nik9000 mentioned this pull request Aug 30, 2024

ESQL: Speed up CASE for some parameters #112295

Merged

nik9000 added 2 commits August 30, 2024 12:42

Fix partial fold too

c91a561

Merge remote-tracking branch 'nik9000/esql_fixup_case' into esql_fixu…

3164322

…p_case

nik9000 commented Aug 30, 2024

View reviewed changes

nik9000 requested review from ivancea and alex-spies August 30, 2024 16:46

alex-spies reviewed Sep 2, 2024

View reviewed changes

alex-spies approved these changes Sep 3, 2024

View reviewed changes

Merge branch 'main' into esql_fixup_case

0981475

costin approved these changes Sep 3, 2024

View reviewed changes

nik9000 added 2 commits September 4, 2024 14:00

More tests

0dad8df

Much more tests

e4084b7

nik9000 mentioned this pull request Sep 5, 2024

ESQL: More love for functions #100558

Closed

86 tasks

Even more tests

2df137d

nik9000 commented Sep 5, 2024

View reviewed changes

nik9000 added 2 commits September 6, 2024 09:07

Merge branch 'main' into esql_fixup_case

9a6437d

Merge branch 'main' into esql_fixup_case

8dd6b0d

alex-spies approved these changes Sep 6, 2024

View reviewed changes

nik9000 added 3 commits September 6, 2024 16:07

Merge branch 'main' into esql_fixup_case

b6644ff

Merge branch 'main' into esql_fixup_case

859b53f

Explain

2702313

nik9000 added the auto-merge-without-approval Automatically merge pull request when CI checks pass (NB doesn't wait for reviews!) label Sep 6, 2024

Merge branch 'main' into esql_fixup_case

7ef7330

nik9000 requested a review from a team as a code owner September 9, 2024 15:30

elasticsearchmachine merged commit ef3a5a1 into elastic:main Sep 9, 2024
14 checks passed

nik9000 deleted the esql_fixup_case branch September 9, 2024 16:32

	* So they are foldable if the value is foldable.
	* So they are foldable if remaining list of conditions is foldable.

ESQL: Fix CASE when conditions are multivalued #112401

ESQL: Fix CASE when conditions are multivalued #112401

Uh oh!

Conversation

nik9000 commented Aug 30, 2024

Uh oh!

github-actions bot commented Aug 30, 2024

Uh oh!

elasticsearchmachine commented Aug 30, 2024

Uh oh!

elasticsearchmachine commented Aug 30, 2024

Uh oh!

Choose a reason for hiding this comment

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!

alex-spies 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!

alex-spies 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!

nik9000 Sep 3, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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

nik9000 Sep 3, 2024 •

edited

Loading