From 39dedf2e431551ee4abdcca1f721c38f30796f48 Mon Sep 17 00:00:00 2001 From: Craig Taverner Date: Wed, 3 Sep 2025 11:44:42 +0200 Subject: [PATCH 01/10] WIP Using only 'synthetic' in union types removal to fix fork bug --- .../src/main/resources/union_types.csv-spec | 43 +++++++++++++++++++ .../xpack/esql/analysis/Analyzer.java | 6 ++- 2 files changed, 47 insertions(+), 2 deletions(-) diff --git a/x-pack/plugin/esql/qa/testFixtures/src/main/resources/union_types.csv-spec b/x-pack/plugin/esql/qa/testFixtures/src/main/resources/union_types.csv-spec index f87a96f65fd26..d84e3bab3a416 100644 --- a/x-pack/plugin/esql/qa/testFixtures/src/main/resources/union_types.csv-spec +++ b/x-pack/plugin/esql/qa/testFixtures/src/main/resources/union_types.csv-spec @@ -1397,6 +1397,22 @@ event_duration:long | _index:keyword | ts:date | ts_str: 8268153 | sample_data_ts_nanos | 2023-10-23T13:52:55.015Z | 2023-10-23T13:52:55.015123456Z | 1698069175015123456 | 172.21.3.15 | 172.21.3.15 ; +multiIndexInlinestatsOfMultiTypedField +required_capability: inlinestats_v11 +// https://github.com/elastic/elasticsearch/issues/133973 +FROM apps, apps_short +| INLINESTATS s = sum(id::integer) +| SORT name, version +| LIMIT 5 +; + +id:unsupported | name:keyword | version:version | s:long +null | aaaaa | 1 | 210 +null | aaaaa | 1 | 210 +null | aaaaa | 1.2.3.4 | 210 +null | aaaaa | 1.2.3.4 | 210 +null | bbbbb | 2.1 | 210 +; inlineStatsUnionGroup-Ignore required_capability: union_types @@ -1720,6 +1736,33 @@ id:integer | name:keyword | count:long 14 | mmmmm | 2 ; +shortIntegerWideningFork +required_capability: union_types +required_capability: casting_operator +required_capability: union_types_numeric_widening +required_capability: fork_v9 + +FROM apps, apps_short +| EVAL x = id::integer +| FORK (WHERE true) (WHERE true) +| DROP _fork +| SORT x ASC, name ASC, version ASC +| LIMIT 10 +; + +id:unsupported | name:keyword | version:version | x:integer +null | aaaaa | 1 | 1 +null | aaaaa | 1 | 1 +null | aaaaa | 1 | 1 +null | aaaaa | 1 | 1 +null | bbbbb | 2.1 | 2 +null | bbbbb | 2.1 | 2 +null | bbbbb | 2.1 | 2 +null | bbbbb | 2.1 | 2 +null | ccccc | 2.3.4 | 3 +null | ccccc | 2.3.4 | 3 +; + ImplicitCastingMultiTypedFieldsKeepSort required_capability: date_nanos_type required_capability: implicit_casting_date_and_date_nanos diff --git a/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/analysis/Analyzer.java b/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/analysis/Analyzer.java index 50a061ab0d6ef..dc14d76ee8950 100644 --- a/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/analysis/Analyzer.java +++ b/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/analysis/Analyzer.java @@ -812,7 +812,9 @@ private LogicalPlan resolveFork(Fork fork, AnalyzerContext context) { // We don't want to keep the same attributes that are outputted by the FORK branches. // Keeping the same attributes can have unintended side effects when applying optimizations like constant folding. for (Attribute attr : outputUnion) { - newOutput.add(new ReferenceAttribute(attr.source(), null, attr.name(), attr.dataType())); + newOutput.add( + new ReferenceAttribute(attr.source(), null, attr.name(), attr.dataType(), Nullability.FALSE, null, attr.synthetic()) + ); } return changed ? new Fork(fork.source(), newSubPlans, newOutput) : fork; @@ -1960,7 +1962,7 @@ private static LogicalPlan planWithoutSyntheticAttributes(LogicalPlan plan) { for (Attribute attr : output) { // Do not let the synthetic union type field attributes end up in the final output. - if (attr.synthetic() && attr instanceof FieldAttribute) { + if (attr.synthetic()) { continue; } newOutput.add(attr); From c9bf571fbf9b3913d5c2507236e86f4cc75b51be Mon Sep 17 00:00:00 2001 From: Craig Taverner Date: Wed, 3 Sep 2025 11:50:38 +0200 Subject: [PATCH 02/10] Update docs/changelog/134033.yaml --- docs/changelog/134033.yaml | 6 ++++++ 1 file changed, 6 insertions(+) create mode 100644 docs/changelog/134033.yaml diff --git a/docs/changelog/134033.yaml b/docs/changelog/134033.yaml new file mode 100644 index 0000000000000..225226d900597 --- /dev/null +++ b/docs/changelog/134033.yaml @@ -0,0 +1,6 @@ +pr: 134033 +summary: Fix FORK with union-types +area: ES|QL +type: bug +issues: + - 133973 From 3e81a637fb60eed44dc7c09e479f854fb669b37a Mon Sep 17 00:00:00 2001 From: Craig Taverner Date: Wed, 3 Sep 2025 12:06:54 +0200 Subject: [PATCH 03/10] Add missing ESQLCapability --- .../qa/testFixtures/src/main/resources/union_types.csv-spec | 2 ++ .../elasticsearch/xpack/esql/action/EsqlCapabilities.java | 5 +++++ 2 files changed, 7 insertions(+) diff --git a/x-pack/plugin/esql/qa/testFixtures/src/main/resources/union_types.csv-spec b/x-pack/plugin/esql/qa/testFixtures/src/main/resources/union_types.csv-spec index d84e3bab3a416..39df727afc251 100644 --- a/x-pack/plugin/esql/qa/testFixtures/src/main/resources/union_types.csv-spec +++ b/x-pack/plugin/esql/qa/testFixtures/src/main/resources/union_types.csv-spec @@ -1399,6 +1399,7 @@ event_duration:long | _index:keyword | ts:date | ts_str: multiIndexInlinestatsOfMultiTypedField required_capability: inlinestats_v11 +required_capability: fork_union_types // https://github.com/elastic/elasticsearch/issues/133973 FROM apps, apps_short | INLINESTATS s = sum(id::integer) @@ -1690,6 +1691,7 @@ required_capability: union_types required_capability: index_metadata_field required_capability: casting_operator required_capability: union_types_numeric_widening +required_capability: fork_union_types FROM apps, apps_short METADATA _index | EVAL id = id::integer diff --git a/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/action/EsqlCapabilities.java b/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/action/EsqlCapabilities.java index dc9f6f3b3978d..dffe62e72862a 100644 --- a/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/action/EsqlCapabilities.java +++ b/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/action/EsqlCapabilities.java @@ -1095,6 +1095,11 @@ public enum Cap { */ FORK_V9, + /** + * Support for union types in FORK + */ + FORK_UNION_TYPES, + /** * Support for the {@code leading_zeros} named parameter. */ From d65bef551f25189749bfb35af280f34c87961540 Mon Sep 17 00:00:00 2001 From: ncordon Date: Wed, 3 Sep 2025 13:23:19 +0200 Subject: [PATCH 04/10] Adds required capability to test that uses fork --- .../esql/qa/testFixtures/src/main/resources/union_types.csv-spec | 1 + 1 file changed, 1 insertion(+) diff --git a/x-pack/plugin/esql/qa/testFixtures/src/main/resources/union_types.csv-spec b/x-pack/plugin/esql/qa/testFixtures/src/main/resources/union_types.csv-spec index 39df727afc251..469f657fa60b4 100644 --- a/x-pack/plugin/esql/qa/testFixtures/src/main/resources/union_types.csv-spec +++ b/x-pack/plugin/esql/qa/testFixtures/src/main/resources/union_types.csv-spec @@ -1740,6 +1740,7 @@ id:integer | name:keyword | count:long shortIntegerWideningFork required_capability: union_types +required_capability: fork_union_types required_capability: casting_operator required_capability: union_types_numeric_widening required_capability: fork_v9 From 21ac430b7935e6d06379e1b3c9962941e606b7a4 Mon Sep 17 00:00:00 2001 From: ncordon Date: Wed, 3 Sep 2025 13:57:56 +0200 Subject: [PATCH 05/10] Tries to exclude NO_FIELDS from the union type cleaning --- .../java/org/elasticsearch/xpack/esql/analysis/Analyzer.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/analysis/Analyzer.java b/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/analysis/Analyzer.java index dc14d76ee8950..7fe6815b0188f 100644 --- a/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/analysis/Analyzer.java +++ b/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/analysis/Analyzer.java @@ -1962,7 +1962,7 @@ private static LogicalPlan planWithoutSyntheticAttributes(LogicalPlan plan) { for (Attribute attr : output) { // Do not let the synthetic union type field attributes end up in the final output. - if (attr.synthetic()) { + if (attr.synthetic() && attr.name().equals(Analyzer.NO_FIELDS_NAME) == false) { continue; } newOutput.add(attr); From aeadedfa8651a31cd1e9b0c507025fd320906136 Mon Sep 17 00:00:00 2001 From: ncordon Date: Wed, 3 Sep 2025 15:20:36 +0200 Subject: [PATCH 06/10] Finds better way of filtering out NO_FIELDS --- .../qa/testFixtures/src/main/resources/union_types.csv-spec | 1 - .../java/org/elasticsearch/xpack/esql/analysis/Analyzer.java | 3 +-- .../java/org/elasticsearch/xpack/esql/plan/logical/Fork.java | 4 +++- 3 files changed, 4 insertions(+), 4 deletions(-) diff --git a/x-pack/plugin/esql/qa/testFixtures/src/main/resources/union_types.csv-spec b/x-pack/plugin/esql/qa/testFixtures/src/main/resources/union_types.csv-spec index 469f657fa60b4..6b66630649fb7 100644 --- a/x-pack/plugin/esql/qa/testFixtures/src/main/resources/union_types.csv-spec +++ b/x-pack/plugin/esql/qa/testFixtures/src/main/resources/union_types.csv-spec @@ -1691,7 +1691,6 @@ required_capability: union_types required_capability: index_metadata_field required_capability: casting_operator required_capability: union_types_numeric_widening -required_capability: fork_union_types FROM apps, apps_short METADATA _index | EVAL id = id::integer diff --git a/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/analysis/Analyzer.java b/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/analysis/Analyzer.java index 7fe6815b0188f..1bd0822e68805 100644 --- a/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/analysis/Analyzer.java +++ b/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/analysis/Analyzer.java @@ -1959,10 +1959,9 @@ static Attribute checkUnresolved(FieldAttribute fa) { private static LogicalPlan planWithoutSyntheticAttributes(LogicalPlan plan) { List output = plan.output(); List newOutput = new ArrayList<>(output.size()); - for (Attribute attr : output) { // Do not let the synthetic union type field attributes end up in the final output. - if (attr.synthetic() && attr.name().equals(Analyzer.NO_FIELDS_NAME) == false) { + if (attr.synthetic() && attr != NO_FIELDS.getFirst()) { continue; } newOutput.add(attr); diff --git a/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/plan/logical/Fork.java b/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/plan/logical/Fork.java index 09719774f8d78..3099e8612f9af 100644 --- a/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/plan/logical/Fork.java +++ b/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/plan/logical/Fork.java @@ -29,6 +29,8 @@ import java.util.function.BiConsumer; import java.util.stream.Collectors; +import static org.elasticsearch.xpack.esql.analysis.Analyzer.NO_FIELDS; + /** * A Fork is a n-ary {@code Plan} where each child is a sub plan, e.g. * {@code FORK [WHERE content:"fox" ] [WHERE content:"dog"] } @@ -121,7 +123,7 @@ public static List outputUnion(List subplans) { continue; } - if (names.contains(attr.name()) == false && attr.name().equals(Analyzer.NO_FIELDS_NAME) == false) { + if (names.contains(attr.name()) == false && attr != NO_FIELDS.getFirst()) { names.add(attr.name()); output.add(attr); } From 3589c501a0bea67c1f27169a504cc846f8d878a1 Mon Sep 17 00:00:00 2001 From: ncordon Date: Fri, 5 Sep 2025 12:23:35 +0200 Subject: [PATCH 07/10] Adds EVAL to check variable can still be used after the FORK --- .../src/main/resources/union_types.csv-spec | 21 ++++++++++--------- 1 file changed, 11 insertions(+), 10 deletions(-) diff --git a/x-pack/plugin/esql/qa/testFixtures/src/main/resources/union_types.csv-spec b/x-pack/plugin/esql/qa/testFixtures/src/main/resources/union_types.csv-spec index 6b66630649fb7..cb990991e6c9e 100644 --- a/x-pack/plugin/esql/qa/testFixtures/src/main/resources/union_types.csv-spec +++ b/x-pack/plugin/esql/qa/testFixtures/src/main/resources/union_types.csv-spec @@ -1749,20 +1749,21 @@ FROM apps, apps_short | FORK (WHERE true) (WHERE true) | DROP _fork | SORT x ASC, name ASC, version ASC +| EVAL x = x + 1 | LIMIT 10 ; id:unsupported | name:keyword | version:version | x:integer -null | aaaaa | 1 | 1 -null | aaaaa | 1 | 1 -null | aaaaa | 1 | 1 -null | aaaaa | 1 | 1 -null | bbbbb | 2.1 | 2 -null | bbbbb | 2.1 | 2 -null | bbbbb | 2.1 | 2 -null | bbbbb | 2.1 | 2 -null | ccccc | 2.3.4 | 3 -null | ccccc | 2.3.4 | 3 +null | aaaaa | 1 | 2 +null | aaaaa | 1 | 2 +null | aaaaa | 1 | 2 +null | aaaaa | 1 | 2 +null | bbbbb | 2.1 | 3 +null | bbbbb | 2.1 | 3 +null | bbbbb | 2.1 | 3 +null | bbbbb | 2.1 | 3 +null | ccccc | 2.3.4 | 4 +null | ccccc | 2.3.4 | 4 ; ImplicitCastingMultiTypedFieldsKeepSort From 502bbc396e834752b97ca96a89ed8ddafe4f0315 Mon Sep 17 00:00:00 2001 From: ncordon Date: Fri, 5 Sep 2025 14:49:19 +0200 Subject: [PATCH 08/10] This should be working now --- .../esql/qa/rest/generative/GenerativeForkRestTest.java | 5 ----- 1 file changed, 5 deletions(-) diff --git a/x-pack/plugin/esql/qa/server/src/main/java/org/elasticsearch/xpack/esql/qa/rest/generative/GenerativeForkRestTest.java b/x-pack/plugin/esql/qa/server/src/main/java/org/elasticsearch/xpack/esql/qa/rest/generative/GenerativeForkRestTest.java index 30af8d4045d19..5b2ea9a525489 100644 --- a/x-pack/plugin/esql/qa/server/src/main/java/org/elasticsearch/xpack/esql/qa/rest/generative/GenerativeForkRestTest.java +++ b/x-pack/plugin/esql/qa/server/src/main/java/org/elasticsearch/xpack/esql/qa/rest/generative/GenerativeForkRestTest.java @@ -55,11 +55,6 @@ protected void shouldSkipTest(String testName) throws IOException { testCase.requiredCapabilities.contains(UNMAPPED_FIELDS.capabilityName()) ); - assumeFalse( - "Tests using implicit_casting_date_and_date_nanos are not supported for now", - testCase.requiredCapabilities.contains(IMPLICIT_CASTING_DATE_AND_DATE_NANOS.capabilityName()) - ); - assumeTrue("Cluster needs to support FORK", hasCapabilities(adminClient(), List.of(FORK_V9.capabilityName()))); } } From 70d3a1f1d0839f67690acea7713c757d97084abb Mon Sep 17 00:00:00 2001 From: ncordon Date: Fri, 5 Sep 2025 14:49:47 +0200 Subject: [PATCH 09/10] Un-ignores INLINESTATS tests that should be working MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Spoiler: they are not 😢 --- .../src/main/resources/inlinestats.csv-spec | 17 ++++------------- 1 file changed, 4 insertions(+), 13 deletions(-) diff --git a/x-pack/plugin/esql/qa/testFixtures/src/main/resources/inlinestats.csv-spec b/x-pack/plugin/esql/qa/testFixtures/src/main/resources/inlinestats.csv-spec index ae0eba15fed64..4ffce29e10fab 100644 --- a/x-pack/plugin/esql/qa/testFixtures/src/main/resources/inlinestats.csv-spec +++ b/x-pack/plugin/esql/qa/testFixtures/src/main/resources/inlinestats.csv-spec @@ -3154,7 +3154,6 @@ count:long | message:keyword multiIndexInlinestatsOfMultiTypedField required_capability: inlinestats_v11 -// https://github.com/elastic/elasticsearch/issues/133973 FROM apps, apps_short | INLINESTATS s = sum(id::integer) | SORT name, version @@ -3396,10 +3395,8 @@ warningRegex:java.lang.IllegalArgumentException: milliSeconds \[-1457696696640\] 2023-10-23T13:53:55.832Z |2023-10-23T13:53:55.832987654Z|1698069235832987654|19 ; -ImplicitCastingMultiTypedDateTruncInlinestats_By-Ignore +ImplicitCastingMultiTypedDateTruncInlinestats_By required_capability: inlinestats_v11 -// https://github.com/elastic/elasticsearch/issues/133973 -// optimized incorrectly due to missing references [$$emp_no$converted_to$long{f$}# FROM employees, employees_incompatible | KEEP emp_no, hire_date @@ -3416,10 +3413,8 @@ c:long | yr:date_nanos 8 | 1994-01-01T00:00:00.000Z ; -ImplicitCastingMultiTypedDateTruncInlinestats_ByWithFilter-Ignore +ImplicitCastingMultiTypedDateTruncInlinestats_ByWithFilter required_capability: inlinestats_v11 -// https://github.com/elastic/elasticsearch/issues/133973 -// optimized incorrectly due to missing references [$$emp_no$converted_to$long{f$}# FROM employees, employees_incompatible | KEEP emp_no, hire_date @@ -3436,10 +3431,8 @@ c:long | yr:date_nanos 0 | 1994-01-01T00:00:00.000Z ; -ImplicitCastingMultiTypedDateTruncInlinestats_ByWithEval-Ignore +ImplicitCastingMultiTypedDateTruncInlinestats_ByWithEval required_capability: inlinestats_v11 -// https://github.com/elastic/elasticsearch/issues/133973 -// optimized incorrectly due to missing references [$$emp_no$converted_to$long{f$}# FROM employees, employees_incompatible | KEEP emp_no, hire_date @@ -3457,10 +3450,8 @@ c:long | yr:date_nanos 8 | 1994-01-01T00:00:00.000Z ; -ImplicitCastingMultiTypedDateTruncInlinestats_ByWithEvalWithFilter-Ignore +ImplicitCastingMultiTypedDateTruncInlinestats_ByWithEvalWithFilter required_capability: inlinestats_v11 -// https://github.com/elastic/elasticsearch/issues/133973 -// optimized incorrectly due to missing references [$$emp_no$converted_to$long{f$}# FROM employees, employees_incompatible | KEEP emp_no, hire_date From 70d2bb8061fe4124668609ff6bfd51f61cd8fc30 Mon Sep 17 00:00:00 2001 From: ncordon Date: Fri, 5 Sep 2025 16:33:51 +0200 Subject: [PATCH 10/10] Re-ignores tests --- .../src/main/resources/inlinestats.csv-spec | 17 +++++++++++++---- 1 file changed, 13 insertions(+), 4 deletions(-) diff --git a/x-pack/plugin/esql/qa/testFixtures/src/main/resources/inlinestats.csv-spec b/x-pack/plugin/esql/qa/testFixtures/src/main/resources/inlinestats.csv-spec index 4ffce29e10fab..ae0eba15fed64 100644 --- a/x-pack/plugin/esql/qa/testFixtures/src/main/resources/inlinestats.csv-spec +++ b/x-pack/plugin/esql/qa/testFixtures/src/main/resources/inlinestats.csv-spec @@ -3154,6 +3154,7 @@ count:long | message:keyword multiIndexInlinestatsOfMultiTypedField required_capability: inlinestats_v11 +// https://github.com/elastic/elasticsearch/issues/133973 FROM apps, apps_short | INLINESTATS s = sum(id::integer) | SORT name, version @@ -3395,8 +3396,10 @@ warningRegex:java.lang.IllegalArgumentException: milliSeconds \[-1457696696640\] 2023-10-23T13:53:55.832Z |2023-10-23T13:53:55.832987654Z|1698069235832987654|19 ; -ImplicitCastingMultiTypedDateTruncInlinestats_By +ImplicitCastingMultiTypedDateTruncInlinestats_By-Ignore required_capability: inlinestats_v11 +// https://github.com/elastic/elasticsearch/issues/133973 +// optimized incorrectly due to missing references [$$emp_no$converted_to$long{f$}# FROM employees, employees_incompatible | KEEP emp_no, hire_date @@ -3413,8 +3416,10 @@ c:long | yr:date_nanos 8 | 1994-01-01T00:00:00.000Z ; -ImplicitCastingMultiTypedDateTruncInlinestats_ByWithFilter +ImplicitCastingMultiTypedDateTruncInlinestats_ByWithFilter-Ignore required_capability: inlinestats_v11 +// https://github.com/elastic/elasticsearch/issues/133973 +// optimized incorrectly due to missing references [$$emp_no$converted_to$long{f$}# FROM employees, employees_incompatible | KEEP emp_no, hire_date @@ -3431,8 +3436,10 @@ c:long | yr:date_nanos 0 | 1994-01-01T00:00:00.000Z ; -ImplicitCastingMultiTypedDateTruncInlinestats_ByWithEval +ImplicitCastingMultiTypedDateTruncInlinestats_ByWithEval-Ignore required_capability: inlinestats_v11 +// https://github.com/elastic/elasticsearch/issues/133973 +// optimized incorrectly due to missing references [$$emp_no$converted_to$long{f$}# FROM employees, employees_incompatible | KEEP emp_no, hire_date @@ -3450,8 +3457,10 @@ c:long | yr:date_nanos 8 | 1994-01-01T00:00:00.000Z ; -ImplicitCastingMultiTypedDateTruncInlinestats_ByWithEvalWithFilter +ImplicitCastingMultiTypedDateTruncInlinestats_ByWithEvalWithFilter-Ignore required_capability: inlinestats_v11 +// https://github.com/elastic/elasticsearch/issues/133973 +// optimized incorrectly due to missing references [$$emp_no$converted_to$long{f$}# FROM employees, employees_incompatible | KEEP emp_no, hire_date