Fix for union-types for multiple columns with the same name by craigtaverner · Pull Request #110793 · elastic/elasticsearch

craigtaverner · 2024-07-11T16:44:22Z

There have been several issues during the union-types work that lead to multiple columns with the same name, and there have been a couple of fixes to this, however more cases have been found, so this PR takes a new approach to finding a more general fix to this problem. Instead of dropping them immediately after using them, we put a single drop at the top of the plan. This makes the plan much less brittle against plan re-ordering rules. Also, we make all synthetic union-type FieldAttributes (transient ones used internally only, not the ones published in the results) use unique internal names, to protect against rules that rely on name uniqueness, and also make debugging easier.

Fixes #109916
Fixes #110490

elasticsearchmachine · 2024-07-11T16:44:45Z

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

D'oh

craigtaverner

Excellent! This is the fix we've been looking for! 👋

craigtaverner · 2024-07-12T12:53:13Z

+        /**
+         * Fix for union-types when sorting a type-casted field. We changed how we remove synthetic union-types fields.
+         */
+        UNION_TYPES_REMOVE_FIELDS;


In related discussions we realized that adding capabilities to the end of the enum increases the risk of clashes when back-porting. Not a big issue, but requires a little extra manual step when back-porting.

Moved up a little, under the other union type capability.

craigtaverner · 2024-07-12T12:54:40Z

            };
        }

-        private LogicalPlan dropConvertedAttributes(LogicalPlan plan, List<FieldAttribute> unionFieldAttributes) {


Nice that this could finally go. It did look wrong.

craigtaverner · 2024-07-12T12:56:24Z

                if (missing.isEmpty() == false) {
-                    output.addAll(missing);
-                    return new EsRelation(esr.source(), esr.index(), output, esr.indexMode(), esr.frozen());
+                    List<Attribute> newOutput = new ArrayList<>(esr.output());


I definitely missed the subtlety of the mutating state behaviour here, and did not realize this was why my earlier edit to EsRelation could be deleted. Thanks for fixing this and bringing back EsRelation fixes.

craigtaverner · 2024-07-12T12:57:40Z

+    @Override
+    public String fieldName() {
+        // The super fieldName uses parents to compute the path; this class ignores parents, so we need to rely on the name instead.
+        // Using field().getName() would be wrong: for subfields like parent.subfield that would return only the last part, subfield.


nice observation!

craigtaverner · 2024-07-12T12:59:20Z

                for (NamedExpression projection : projections) {
-                    if (projection instanceof FieldAttribute f && stats.exists(f.qualifiedName()) == false) {
+                    // Do not use the attribute name, this can deviate from the field name for union types.
+                    if (projection instanceof FieldAttribute f && stats.exists(f.fieldName()) == false) {


Perhaps in the fieldName javadoc we should mention why we don't use qualified name (the fact that qualifiers are always null in ESQL, and this is an artifact of SQL code)?

I hope we can merge #110581 soon, making the comment here very short lived. Update I mean any comment about qualified names, not the existing comment.

In any case, the qualified name might even be pretty wrong here - our indices don't know about qualifiers, only field names!

craigtaverner · 2024-07-12T13:00:30Z

+        return Objects.equals(index, other.index)
+            && indexMode == other.indexMode()
+            && frozen == other.frozen
+            && Objects.equals(attrs, other.attrs);


Excellent. I had this fix in an earlier version of union-types, but removed it when I discovered it was not needed. I did not realize that it was only not needed due to another bug. It feels good to see this code come back.

I think it's quite important; this can mask test failures (it did), and it can also prevent optimizations from taking effect if all they do is change an EsRelation's attributes (like for union types).

This is needed because now (sometime last year when EsRelation has been changed to work "better" in ESQL) attributes can also be provided through a new public constructor. Until the public constructor that also accepted attributes, they were built from EsIndex. There was an immutability and a link betwee EsIndex and EsRelation and I think that's why equals didn't need the attributes (the object was built once and it stayed as is from the _field_caps call until the query was executed on ES and results returned).

craigtaverner · 2024-07-12T13:01:17Z

            ElementType elementType = PlannerUtils.toElementType(dataType, fieldExtractPreference);
-            String fieldName = attr.name();
+            // Do not use the field attribute name, this can deviate from the field name for union types.
+            String fieldName = attr instanceof FieldAttribute fa ? fa.fieldName() : attr.name();


I was wondering, surely all attributes in this method must be field attributes?

I think metadata attributes, like _id or _source could also occur, at least in principle.

alex-spies

This should be good to go once the remaining test failures are fixed.

…change) fail

astefan

LGTM in general.
I've left some comments, kind of optional, or nice to haves.

astefan · 2024-07-15T09:13:29Z

+        // On later versions, the attribute can be renamed when creating synthetic attributes.
+        // TODO: We should use synthetic() to check for that case.
+        // https://github.com/elastic/elasticsearch/issues/105821
+        boolean isSynthetic = name().startsWith(SYNTHETIC_ATTRIBUTE_NAME_PREFIX);


I don't think this var here improves readbility. It's used only once and can be removed.

astefan · 2024-07-15T09:39:36Z

+required_capability: casting_operator
+required_capability: union_types_remove_fields
+
+FROM sample_data, sample_data_str


It's worth adding here another more advanced variant (but still trying to keep the original query that brought up this issue): FROM sample_data, sample_data_str | SORT client_ip::ip, @timestamp ASC| eval client_ip_as_ip = client_ip::ip.

That's a good one.

astefan · 2024-07-15T09:53:48Z

+            new UnresolveUnionTypes(),
+            new DropSyntheticUnionTypeAttributes()


It feels like these two rules could be only one, an union-types cleanup step kind of rule. They both do some very specific union-types operations and I don't see them not living together. I don't have an idea on how to merge them, but if you do and can do it, I kinda want to see them in only one rule.

I'll consolidate them.

astefan · 2024-07-15T09:55:49Z

        ) {
-            var unionFieldAttribute = new FieldAttribute(fa.source(), fa.name(), resolvedField);  // Generates new ID for the field
+            // Generate new ID for the field and suffix it with the data type to maintain unique attribute names.
+            String unionTypedFieldName = SubstituteSurrogates.rawTemporaryName(


The more I see rawTemporaryName being used, they more it feels like it can belong to FieldAttribute, or somewhere else outside the SubstituteSurrogates rule.

Agree and so I just tried that; it would make sense to move to esql.core.Attribute, but then we'd likely want to move temporaryName as well; the latter, however, relies on AggregateFunction which is outside of esql.core.

Maybe let's leave this for another time to not cause too much code movement; this PR needs backporting to 8.15, that could become harder if we also refactor outside of the PR's immediate scope.

astefan · 2024-07-15T10:58:05Z

+        return Objects.equals(index, other.index)
+            && indexMode == other.indexMode()
+            && frozen == other.frozen
+            && Objects.equals(attrs, other.attrs);


This is needed because now (sometime last year when EsRelation has been changed to work "better" in ESQL) attributes can also be provided through a new public constructor. Until the public constructor that also accepted attributes, they were built from EsIndex. There was an immutability and a link betwee EsIndex and EsRelation and I think that's why equals didn't need the attributes (the object was built once and it stayed as is from the _field_caps call until the query was executed on ES and results returned).

alex-spies · 2024-07-15T12:33:30Z

Thanks for the review @astefan , I addressed your comments!

elasticsearchmachine · 2024-07-15T13:59:24Z

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

The tests fail spordically, so safer to mute the entire suite.

…110793) * Make union types use unique attribute names * Cleanup leftover * Added failing test and final fix to EsRelation * Implement FieldAttribute.fieldName() * Fix tests * Refactor * Do not ignore union typed field's parent * Fix important typo D'oh * Mute unrelated (part of) test * Move capability to better location * Fix analyzer tests * multi-node tests with an earlier version of union-types (before this change) fail * Add capability to remaining failing tests * Remove variable * Add more complex test * Consolidate union type cleanup rules * Add 3 more required_capability's to make CI happy * Update caps for union type subfield yaml tests * Update docs/changelog/110793.yaml * Refined changelog text * Mute BWC for 8.15.0 for failing YAML tests * union_types_remove_fields for all 160_union_types tests The tests fail spordically, so safer to mute the entire suite. --------- Co-authored-by: Alexander Spies <alexander.spies@elastic.co>

* Fix bug in union-types with type-casting in grouping key of STATS (#110476) * Allow auto-generated type-cast fields in CsvTests This allows, for example, a csv-spec test result header like `client_ip::ip:ip`, which is generated with a command like `STATS count=count(*) BY client_ip::ip` It is also a small cleanup of the header parsing code, since it was using Strings.split() in an odd way. * Fix bug in union-types with type-casting in grouping key of STATS * Update docs/changelog/110476.yaml * Added casting_operator required capability Using the new `::` syntax requires disabling support for older versions in multi-cluster tests. * Added more tests for inline stats over long/datetime * Trying to fix the STATS...STATS bug This makes two changes: * Keeps the Alias in the aggs.aggregates from the grouping key, so that ReplaceStatsNestedExpressionWithEval still works * Adds explicit support for union-types conversion at grouping key loading in the ordinalGroupingOperatorFactory Neither fix the particular edge case, but do seem correct * Added EsqlCapability for this change So that mixed cluster tests don't fail these new queries. * Fix InsertFieldExtract for union types Union types require a FieldExtractExec to be performed first thing at the bottom of local physical plans. In queries like ``` from testidx* | eval x = to_string(client_ip) | stats c = count(*) by x | keep c ``` The `stats` has the grouping `x` but the aggregates get pruned to just `c`. In cases like this, we did not insert a FieldExtractExec, which this fixes. * Revert query that previously failed With Alex's fix, this query now passes. * Revert integration of union-types to ordinals aggregator This is because we have not found a test case that actually demonstrates this is necessary. * More tests that would fail without the latest fix * Correct code style * Fix failing case when aggregating on union-type with invalid grouping key * Capabilities restrictions on the new YML tests * Update docs/changelog/110476.yaml --------- Co-authored-by: Alexander Spies <alexander.spies@elastic.co> * An alternative approach to supporting union-types on stats grouping field (#110600) * Added union-types field extration to ordinals aggregation * Revert previous approach to getting union-types working in aggregations Where the grouping field is erased by later commands, like a subsequent stats. Instead we include union-type supports in the ordinals aggregation and mark the block loader as not supporting ordinals. * Fix union-types when aggregating on inline conversion function (#110652) A query like: ``` FROM sample_data, sample_data_str | STATS count=count(*) BY client_ip = TO_IP(client_ip) | SORT count DESC, client_ip ASC | KEEP count, client_ip ``` Failed due to unresolved aggregates from the union-type in the grouping key * Fix for union-types for multiple columns with the same name (#110793) * Make union types use unique attribute names * Cleanup leftover * Added failing test and final fix to EsRelation * Implement FieldAttribute.fieldName() * Fix tests * Refactor * Do not ignore union typed field's parent * Fix important typo D'oh * Mute unrelated (part of) test * Move capability to better location * Fix analyzer tests * multi-node tests with an earlier version of union-types (before this change) fail * Add capability to remaining failing tests * Remove variable * Add more complex test * Consolidate union type cleanup rules * Add 3 more required_capability's to make CI happy * Update caps for union type subfield yaml tests * Update docs/changelog/110793.yaml * Refined changelog text * Mute BWC for 8.15.0 for failing YAML tests * union_types_remove_fields for all 160_union_types tests The tests fail spordically, so safer to mute the entire suite. --------- Co-authored-by: Alexander Spies <alexander.spies@elastic.co> --------- Co-authored-by: Alexander Spies <alexander.spies@elastic.co>

* Make union types use unique attribute names * Cleanup leftover * Added failing test and final fix to EsRelation * Implement FieldAttribute.fieldName() * Fix tests * Refactor * Do not ignore union typed field's parent * Fix important typo D'oh * Mute unrelated (part of) test * Move capability to better location * Fix analyzer tests * multi-node tests with an earlier version of union-types (before this change) fail * Add capability to remaining failing tests * Remove variable * Add more complex test * Consolidate union type cleanup rules * Add 3 more required_capability's to make CI happy * Update caps for union type subfield yaml tests * Update docs/changelog/110793.yaml * Refined changelog text * Mute BWC for 8.15.0 for failing YAML tests * union_types_remove_fields for all 160_union_types tests The tests fail spordically, so safer to mute the entire suite. --------- Co-authored-by: Alexander Spies <alexander.spies@elastic.co>

craigtaverner added >non-issue Team:Analytics Meta label for analytical engine team (ESQL/Aggs/Geo) :Analytics/ES|QL AKA ESQL labels Jul 11, 2024

craigtaverner requested review from alex-spies and astefan July 11, 2024 16:44

elasticsearchmachine added the v8.16.0 label Jul 11, 2024

alex-spies and others added 3 commits July 11, 2024 19:09

Make union types use unique attribute names

3511131

Cleanup leftover

ead0948

Added failing test and final fix to EsRelation

5541673

craigtaverner force-pushed the union_types_remove_fields branch from 0745971 to 5541673 Compare July 11, 2024 17:10

alex-spies added 3 commits July 12, 2024 13:26

Implement FieldAttribute.fieldName()

a3b7933

Fix tests

5905a0f

Refactor

0fb1c37

alex-spies mentioned this pull request Jul 12, 2024

ESQL: Validate unique plan attribute names #110488

Merged

alex-spies added 2 commits July 12, 2024 14:23

Do not ignore union typed field's parent

4ecf677

Fix important typo

6f4833f

D'oh

alex-spies added v8.15.0 auto-backport Automatically create backport pull requests when merged labels Jul 12, 2024

craigtaverner commented Jul 12, 2024

View reviewed changes

alex-spies added 2 commits July 12, 2024 15:16

Mute unrelated (part of) test

69fba3e

Move capability to better location

03a2255

alex-spies approved these changes Jul 12, 2024

View reviewed changes

alex-spies and others added 3 commits July 12, 2024 15:54

Fix analyzer tests

5878131

multi-node tests with an earlier version of union-types (before this …

3b4cea8

…change) fail

Add capability to remaining failing tests

7d315f3

astefan approved these changes Jul 15, 2024

View reviewed changes

alex-spies added 3 commits July 15, 2024 13:37

Remove variable

4aa2857

Add more complex test

21352ad

Consolidate union type cleanup rules

99c093d

Add 3 more required_capability's to make CI happy

8839cad

alex-spies removed the auto-backport Automatically create backport pull requests when merged label Jul 15, 2024

craigtaverner mentioned this pull request Jul 15, 2024

Backport bugfixes for union-types to 8.15 #110872

Merged

Update caps for union type subfield yaml tests

26dff9b

craigtaverner added >bug and removed >non-issue labels Jul 15, 2024

Update docs/changelog/110793.yaml

2bde896

craigtaverner changed the title ~~Fix for union-types when sorting a type-casted field~~ Fix for union-types for multiple columns with the same name Jul 15, 2024

craigtaverner added 3 commits July 15, 2024 16:11

Refined changelog text

f6ba8ea

Mute BWC for 8.15.0 for failing YAML tests

1b6a5a8

union_types_remove_fields for all 160_union_types tests

88e01b6

The tests fail spordically, so safer to mute the entire suite.

craigtaverner merged commit 3a93757 into elastic:main Jul 15, 2024

alex-spies mentioned this pull request Sep 16, 2024

ESQL: Remove parent from FieldAttribute #112881

Merged

		new UnresolveUnionTypes(),
		new DropSyntheticUnionTypeAttributes()

Conversation

craigtaverner commented Jul 11, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

elasticsearchmachine commented Jul 11, 2024

Uh oh!

craigtaverner 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!

alex-spies Jul 12, 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!

alex-spies left a comment

Choose a reason for hiding this comment

Uh oh!

astefan 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!

alex-spies commented Jul 15, 2024

Uh oh!

elasticsearchmachine commented Jul 15, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

craigtaverner commented Jul 11, 2024 •

edited

Loading

alex-spies Jul 12, 2024 •

edited

Loading