Skip to content

Fix for union-types for multiple columns with the same name #110793

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
craigtaverner merged 22 commits into from
Jul 15, 2024
Merged

Fix for union-types for multiple columns with the same name #110793

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
22 commits
Select commit Hold shift + click to select a range
3511131
Make union types use unique attribute names
alex-spies Jul 11, 2024
ead0948
Cleanup leftover
alex-spies Jul 11, 2024
5541673
Added failing test and final fix to EsRelation
craigtaverner Jul 11, 2024
a3b7933
Implement FieldAttribute.fieldName()
alex-spies Jul 12, 2024
5905a0f
Fix tests
alex-spies Jul 12, 2024
0fb1c37
Refactor
alex-spies Jul 12, 2024
4ecf677
Do not ignore union typed field's parent
alex-spies Jul 12, 2024
6f4833f
Fix important typo
alex-spies Jul 12, 2024
69fba3e
Mute unrelated (part of) test
alex-spies Jul 12, 2024
03a2255
Move capability to better location
alex-spies Jul 12, 2024
5878131
Fix analyzer tests
alex-spies Jul 12, 2024
3b4cea8
multi-node tests with an earlier version of union-types (before this …
craigtaverner Jul 15, 2024
7d315f3
Add capability to remaining failing tests
alex-spies Jul 15, 2024
4aa2857
Remove variable
alex-spies Jul 15, 2024
21352ad
Add more complex test
alex-spies Jul 15, 2024
99c093d
Consolidate union type cleanup rules
alex-spies Jul 15, 2024
8839cad
Add 3 more required_capability's to make CI happy
alex-spies Jul 15, 2024
26dff9b
Update caps for union type subfield yaml tests
alex-spies Jul 15, 2024
2bde896
Update docs/changelog/110793.yaml
craigtaverner Jul 15, 2024
f6ba8ea
Refined changelog text
craigtaverner Jul 15, 2024
1b6a5a8
Mute BWC for 8.15.0 for failing YAML tests
craigtaverner Jul 15, 2024
88e01b6
union_types_remove_fields for all 160_union_types tests
craigtaverner Jul 15, 2024
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
7 changes: 7 additions & 0 deletions docs/changelog/110793.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
pr: 110793
summary: Fix for union-types for multiple columns with the same name
area: ES|QL
type: bug
issues:
- 110490
- 109916
21 changes: 19 additions & 2 deletions .../esql-core/src/main/java/org/elasticsearch/xpack/esql/core/expression/FieldAttribute.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,10 @@
* - nestedParent - if nested, what's the parent (which might not be the immediate one)
*/
public class FieldAttribute extends TypedAttribute {
// TODO: This constant should not be used if possible; use .synthetic()
// https://github.com/elastic/elasticsearch/issues/105821
public static final String SYNTHETIC_ATTRIBUTE_NAME_PREFIX = "$$";

static final NamedWriteableRegistry.Entry ENTRY = new NamedWriteableRegistry.Entry(
Attribute.class,
"FieldAttribute",
Expand Down Expand Up @@ -72,12 +76,11 @@ public FieldAttribute(
boolean synthetic
) {
super(source, name, type, qualifier, nullability, id, synthetic);
this.path = parent != null ? parent.name() : StringUtils.EMPTY;
this.path = parent != null ? parent.fieldName() : StringUtils.EMPTY;
this.parent = parent;
this.field = field;
}

@SuppressWarnings("unchecked")
public FieldAttribute(StreamInput in) throws IOException {
/*
* The funny casting dance with `(StreamInput & PlanStreamInput) in` is required
Expand Down Expand Up @@ -131,6 +134,20 @@ public String path() {
return path;
}

/**
* The full name of the field in the index, including all parent fields. E.g. {@code parent.subfield.this_field}.
*/
public String fieldName() {
// Before 8.15, the field name was the same as the attribute's name.
// 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
if (name().startsWith(SYNTHETIC_ATTRIBUTE_NAME_PREFIX) == false) {
return name();
}
return Strings.hasText(path) ? path + "." + field.getName() : field.getName();
}

public String qualifiedPath() {
// return only the qualifier is there's no path
return qualifier() != null ? qualifier() + (Strings.hasText(path) ? "." + path : StringUtils.EMPTY) : path;
Expand Down
59 changes: 59 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 @@ -97,6 +97,7 @@ multiIndexIpString
required_capability: union_types
required_capability: metadata_fields
required_capability: casting_operator
required_capability: union_types_remove_fields

FROM sample_data, sample_data_str METADATA _index
| EVAL client_ip = client_ip::ip
Expand Down Expand Up @@ -125,6 +126,7 @@ multiIndexIpStringRename
required_capability: union_types
required_capability: metadata_fields
required_capability: casting_operator
required_capability: union_types_remove_fields

FROM sample_data, sample_data_str METADATA _index
| EVAL host_ip = client_ip::ip
Expand Down Expand Up @@ -152,6 +154,7 @@ sample_data_str | 2023-10-23T12:15:03.360Z | 172.21.2.162 | 3450233
multiIndexIpStringRenameToString
required_capability: union_types
required_capability: metadata_fields
required_capability: union_types_remove_fields

FROM sample_data, sample_data_str METADATA _index
| EVAL host_ip = TO_STRING(TO_IP(client_ip))
Expand Down Expand Up @@ -179,6 +182,7 @@ sample_data_str | 2023-10-23T12:15:03.360Z | 172.21.2.162 | 3450233
multiIndexWhereIpString
required_capability: union_types
required_capability: metadata_fields
required_capability: union_types_remove_fields

FROM sample_data, sample_data_str METADATA _index
| WHERE STARTS_WITH(TO_STRING(client_ip), "172.21.2")
Expand All @@ -196,6 +200,7 @@ sample_data_str | 2023-10-23T12:15:03.360Z | 3450233 | Connected
multiIndexWhereIpStringLike
required_capability: union_types
required_capability: metadata_fields
required_capability: union_types_remove_fields

FROM sample_data, sample_data_str METADATA _index
| WHERE TO_STRING(client_ip) LIKE "172.21.2.*"
Expand All @@ -210,9 +215,39 @@ sample_data_str | 2023-10-23T12:27:28.948Z | 2764889 | Connected
sample_data_str | 2023-10-23T12:15:03.360Z | 3450233 | Connected to 10.1.0.3
;

multiIndexSortIpString
required_capability: union_types
required_capability: casting_operator
required_capability: union_types_remove_fields

FROM sample_data, sample_data_str
Copy link
Copy Markdown
Contributor

@astefan astefan Jul 15, 2024

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown
Contributor

@alex-spies alex-spies Jul 15, 2024

Choose a reason for hiding this comment

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

That's a good one.

| SORT client_ip::ip
| LIMIT 1
;

@timestamp:date | client_ip:null | event_duration:long | message:keyword
2023-10-23T13:33:34.937Z | null | 1232382 | Disconnected
;

multiIndexSortIpStringEval
required_capability: union_types
required_capability: casting_operator
required_capability: union_types_remove_fields

FROM sample_data, sample_data_str
| SORT client_ip::ip, @timestamp ASC
| EVAL client_ip_as_ip = client_ip::ip
| LIMIT 1
;

@timestamp:date | client_ip:null | event_duration:long | message:keyword | client_ip_as_ip:ip
2023-10-23T13:33:34.937Z | null | 1232382 | Disconnected | 172.21.0.5
;

multiIndexIpStringStats
required_capability: union_types
required_capability: casting_operator
required_capability: union_types_remove_fields

FROM sample_data, sample_data_str
| EVAL client_ip = client_ip::ip
Expand All @@ -231,6 +266,7 @@ count:long | client_ip:ip
multiIndexIpStringRenameStats
required_capability: union_types
required_capability: casting_operator
required_capability: union_types_remove_fields

FROM sample_data, sample_data_str
| EVAL host_ip = client_ip::ip
Expand All @@ -248,6 +284,7 @@ count:long | host_ip:ip

multiIndexIpStringRenameToStringStats
required_capability: union_types
required_capability: union_types_remove_fields

FROM sample_data, sample_data_str
| EVAL host_ip = TO_STRING(TO_IP(client_ip))
Expand Down Expand Up @@ -333,6 +370,7 @@ mc:l | count:l

multiIndexWhereIpStringStats
required_capability: union_types
required_capability: union_types_remove_fields

FROM sample_data, sample_data_str
| WHERE STARTS_WITH(TO_STRING(client_ip), "172.21.2")
Expand All @@ -349,6 +387,7 @@ count:long | message:keyword
multiIndexTsLong
required_capability: union_types
required_capability: metadata_fields
required_capability: union_types_remove_fields

FROM sample_data, sample_data_ts_long METADATA _index
| EVAL @timestamp = TO_DATETIME(@timestamp)
Expand Down Expand Up @@ -376,6 +415,7 @@ sample_data_ts_long | 2023-10-23T12:15:03.360Z | 172.21.2.162 | 3450233
multiIndexTsLongRename
required_capability: union_types
required_capability: metadata_fields
required_capability: union_types_remove_fields

FROM sample_data, sample_data_ts_long METADATA _index
| EVAL ts = TO_DATETIME(@timestamp)
Expand Down Expand Up @@ -403,6 +443,7 @@ sample_data_ts_long | 2023-10-23T12:15:03.360Z | 172.21.2.162 | 3450233
multiIndexTsLongRenameToString
required_capability: union_types
required_capability: metadata_fields
required_capability: union_types_remove_fields

FROM sample_data, sample_data_ts_long METADATA _index
| EVAL ts = TO_STRING(TO_DATETIME(@timestamp))
Expand Down Expand Up @@ -430,6 +471,7 @@ sample_data_ts_long | 2023-10-23T12:15:03.360Z | 172.21.2.162 | 3450233
multiIndexWhereTsLong
required_capability: union_types
required_capability: metadata_fields
required_capability: union_types_remove_fields

FROM sample_data, sample_data_ts_long METADATA _index
| WHERE TO_LONG(@timestamp) < 1698068014937
Expand All @@ -446,6 +488,7 @@ sample_data_ts_long | 172.21.2.162 | 3450233 | Connected to 10.

multiIndexTsLongStats
required_capability: union_types
required_capability: union_types_remove_fields

FROM sample_data, sample_data_ts_long
| EVAL @timestamp = DATE_TRUNC(1 hour, TO_DATETIME(@timestamp))
Expand Down Expand Up @@ -517,6 +560,7 @@ mc:l | count:l
multiIndexTsLongStatsStats
required_capability: union_types
required_capability: union_types_agg_cast
required_capability: union_types_remove_fields

FROM sample_data, sample_data_ts_long
| EVAL ts = TO_STRING(@timestamp)
Expand All @@ -531,6 +575,7 @@ mc:l | count:l

multiIndexTsLongRenameStats
required_capability: union_types
required_capability: union_types_remove_fields

FROM sample_data, sample_data_ts_long
| EVAL hour = DATE_TRUNC(1 hour, TO_DATETIME(@timestamp))
Expand All @@ -546,6 +591,7 @@ count:long | hour:date

multiIndexTsLongRenameToDatetimeToStringStats
required_capability: union_types
required_capability: union_types_remove_fields

FROM sample_data, sample_data_ts_long
| EVAL hour = LEFT(TO_STRING(TO_DATETIME(@timestamp)), 13)
Expand All @@ -561,6 +607,7 @@ count:long | hour:keyword

multiIndexTsLongRenameToStringStats
required_capability: union_types
required_capability: union_types_remove_fields

FROM sample_data, sample_data_ts_long
| EVAL mess = LEFT(TO_STRING(@timestamp), 7)
Expand All @@ -579,6 +626,7 @@ count:long | mess:keyword

multiIndexTsLongStatsInline
required_capability: union_types
required_capability: union_types_remove_fields

FROM sample_data, sample_data_ts_long
| STATS count=COUNT(*), max=MAX(TO_DATETIME(@timestamp))
Expand All @@ -603,6 +651,7 @@ count:long

multiIndexWhereTsLongStats
required_capability: union_types
required_capability: union_types_remove_fields

FROM sample_data, sample_data_ts_long
| WHERE TO_LONG(@timestamp) < 1698068014937
Expand All @@ -619,6 +668,7 @@ count:long | message:keyword
multiIndexIpStringTsLong
required_capability: union_types
required_capability: metadata_fields
required_capability: union_types_remove_fields

FROM sample_data* METADATA _index
| EVAL @timestamp = TO_DATETIME(@timestamp), client_ip = TO_IP(client_ip)
Expand Down Expand Up @@ -687,6 +737,7 @@ sample_data_ts_long | 8268153 | Connection error
multiIndexIpStringTsLongRename
required_capability: union_types
required_capability: metadata_fields
required_capability: union_types_remove_fields

FROM sample_data* METADATA _index
| EVAL ts = TO_DATETIME(@timestamp), host_ip = TO_IP(client_ip)
Expand Down Expand Up @@ -755,6 +806,7 @@ sample_data_ts_long | 8268153 | Connection error
multiIndexIpStringTsLongRenameToString
required_capability: union_types
required_capability: metadata_fields
required_capability: union_types_remove_fields

FROM sample_data* METADATA _index
| EVAL ts = TO_STRING(TO_DATETIME(@timestamp)), host_ip = TO_STRING(TO_IP(client_ip))
Expand Down Expand Up @@ -789,6 +841,7 @@ sample_data_ts_long | 2023-10-23T12:15:03.360Z | 172.21.2.162 | 3450233
multiIndexWhereIpStringTsLong
required_capability: union_types
required_capability: metadata_fields
required_capability: union_types_remove_fields

FROM sample_data* METADATA _index
| WHERE TO_LONG(@timestamp) < 1698068014937 AND TO_STRING(client_ip) == "172.21.2.162"
Expand All @@ -804,6 +857,7 @@ sample_data_ts_long | 3450233 | Connected to 10.1.0.3

multiIndexWhereIpStringTsLongStats
required_capability: union_types
required_capability: union_types_remove_fields

FROM sample_data*
| WHERE TO_LONG(@timestamp) < 1698068014937 AND TO_STRING(client_ip) == "172.21.2.162"
Expand All @@ -819,6 +873,7 @@ count:long | message:keyword
multiIndexWhereIpStringLikeTsLong
required_capability: union_types
required_capability: metadata_fields
required_capability: union_types_remove_fields

FROM sample_data* METADATA _index
| WHERE TO_LONG(@timestamp) < 1698068014937 AND TO_STRING(client_ip) LIKE "172.21.2.16?"
Expand All @@ -834,6 +889,7 @@ sample_data_ts_long | 3450233 | Connected to 10.1.0.3

multiIndexWhereIpStringLikeTsLongStats
required_capability: union_types
required_capability: union_types_remove_fields

FROM sample_data*
| WHERE TO_LONG(@timestamp) < 1698068014937 AND TO_STRING(client_ip) LIKE "172.21.2.16?"
Expand All @@ -849,6 +905,7 @@ count:long | message:keyword
multiIndexMultiColumnTypesRename
required_capability: union_types
required_capability: metadata_fields
required_capability: union_types_remove_fields

FROM sample_data* METADATA _index
| WHERE event_duration > 8000000
Expand All @@ -865,6 +922,7 @@ null | null | 8268153 | Connection error | samp
multiIndexMultiColumnTypesRenameAndKeep
required_capability: union_types
required_capability: metadata_fields
required_capability: union_types_remove_fields

FROM sample_data* METADATA _index
| WHERE event_duration > 8000000
Expand All @@ -882,6 +940,7 @@ sample_data_ts_long | 2023-10-23T13:52:55.015Z | 1698069175015 | 16
multiIndexMultiColumnTypesRenameAndDrop
required_capability: union_types
required_capability: metadata_fields
required_capability: union_types_remove_fields

FROM sample_data* METADATA _index
| WHERE event_duration > 8000000
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 @@ -129,6 +129,11 @@ public enum Cap {
*/
UNION_TYPES_INLINE_FIX,

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

/**
* Fix a parsing issue where numbers below Long.MIN_VALUE threw an exception instead of parsing as doubles.
* see <a href="https://github.com/elastic/elasticsearch/issues/104323"> Parsing large numbers is inconsistent #104323 </a>
Expand Down
Loading