Skip to content

Fix FORK with union-types #134033

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 15 commits into from
Sep 5, 2025
Merged

Fix FORK with union-types #134033

Show file tree
Hide file tree
Changes from 10 commits
Commits
Show all changes
15 commits
Select commit Hold shift + click to select a range
39dedf2
WIP Using only 'synthetic' in union types removal to fix fork bug
craigtaverner Sep 3, 2025
c9bf571
Update docs/changelog/134033.yaml
craigtaverner Sep 3, 2025
3e81a63
Add missing ESQLCapability
craigtaverner Sep 3, 2025
d65bef5
Adds required capability to test that uses fork
ncordon Sep 3, 2025
9e75a9c
Merge branch 'main' into fix_fork_union_types
ncordon Sep 3, 2025
21ac430
Tries to exclude NO_FIELDS from the union type cleaning
ncordon Sep 3, 2025
aeadedf
Finds better way of filtering out NO_FIELDS
ncordon Sep 3, 2025
ff371e8
Merge branch 'main' into fix_fork_union_types
craigtaverner Sep 3, 2025
a4cbd00
Merge branch 'main' into fix_fork_union_types
ncordon Sep 4, 2025
75c37d0
Merge branch 'main' into fix_fork_union_types
craigtaverner Sep 4, 2025
3589c50
Adds EVAL to check variable can still be used after the FORK
ncordon Sep 5, 2025
510c2a8
Merge branch 'main' into fix_fork_union_types
ncordon Sep 5, 2025
502bbc3
This should be working now
ncordon Sep 5, 2025
70d3a1f
Un-ignores INLINESTATS tests that should be working
ncordon Sep 5, 2025
70d2bb8
Re-ignores tests
ncordon Sep 5, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions docs/changelog/134033.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
pr: 134033
summary: Fix FORK with union-types
area: ES|QL
type: bug
issues:
- 133973
45 changes: 45 additions & 0 deletions x-pack/plugin/esql/qa/testFixtures/src/main/resources/union_types.csv-spec
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1397,6 +1397,23 @@ 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
;

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You might want to look at inlinestats.csv-spec file and search for https://github.com/elastic/elasticsearch/issues/133973. There are some ignored tests in there that need a check.

Copy link
Contributor

@ncordon ncordon Sep 5, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We checked these tests cannot be unignored because there seems to be a bug with INLINESTATS in queries where we use a union type, which seems unrelated to what we've done for FORK:

FROM employees, employees_incompatible
| KEEP emp_no, hire_date 
| EVAL yr = DATE_TRUNC(1 year, hire_date)
| INLINESTATS c = count(emp_no::long) BY yr
| SORT yr DESC
| LIMIT 5
;

To be investigated as a separate piece of work

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)
| 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
Expand Down Expand Up @@ -1720,6 +1737,34 @@ id:integer | name:keyword | count:long
14 | mmmmm | 2
;

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 apps, apps_short
| EVAL x = id::integer
| FORK (WHERE true) (WHERE true)
| DROP _fork
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we do something with x after FORK? for example adding EVAL x = x + 1 or something similar?
just to validate that the value can be used.

| 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
Expand Down
5 changes: 5 additions & 0 deletions x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/action/EsqlCapabilities.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.
*/
Expand Down
7 changes: 4 additions & 3 deletions x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/analysis/Analyzer.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -809,7 +809,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;
Expand Down Expand Up @@ -1946,10 +1948,9 @@ static Attribute checkUnresolved(FieldAttribute fa) {
private static LogicalPlan planWithoutSyntheticAttributes(LogicalPlan plan) {
List<Attribute> output = plan.output();
List<Attribute> 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 instanceof FieldAttribute) {
if (attr.synthetic() && attr != NO_FIELDS.getFirst()) {
continue;
}
newOutput.add(attr);
Expand Down
4 changes: 3 additions & 1 deletion x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/plan/logical/Fork.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"] }
Expand Down Expand Up @@ -121,7 +123,7 @@ public static List<Attribute> outputUnion(List<LogicalPlan> 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);
}
Expand Down