Skip to content

Commit ac0c508

Browse files
authored
ESQL: Fix inconsistent column order in MV_EXPAND (#129745)
The new attribute generated by MV_EXPAND should remain in the original position. The projection added by ProjectAwayColumns does not respect the original order of attributes. Make ProjectAwayColumns respect the order of attributes to fix this.
1 parent adcb2a5 commit ac0c508

File tree

7 files changed

+350
-243
lines changed

7 files changed

+350
-243
lines changed

docs/changelog/129745.yaml

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,6 @@
1+
pr: 129745
2+
summary: "ESQL: Fix `mv_expand` inconsistent column order"
3+
area: ES|QL
4+
type: bug
5+
issues:
6+
- 129000

x-pack/plugin/esql-core/src/main/java/org/elasticsearch/xpack/esql/core/expression/AttributeSet.java

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -261,5 +261,9 @@ public boolean isEmpty() {
261261
public AttributeSet build() {
262262
return new AttributeSet(mapBuilder.build());
263263
}
264+
265+
public void clear() {
266+
mapBuilder.keySet().clear();
267+
}
264268
}
265269
}

x-pack/plugin/esql/qa/server/src/main/java/org/elasticsearch/xpack/esql/qa/rest/generative/GenerativeRestTest.java

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -53,7 +53,6 @@ public abstract class GenerativeRestTest extends ESRestTestCase {
5353
"Data too large", // Circuit breaker exceptions eg. https://github.com/elastic/elasticsearch/issues/130072
5454

5555
// Awaiting fixes for correctness
56-
"Expecting the following columns \\[.*\\], got", // https://github.com/elastic/elasticsearch/issues/129000
5756
"Expecting at most \\[.*\\] columns, got \\[.*\\]" // https://github.com/elastic/elasticsearch/issues/129561
5857
);
5958

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

Lines changed: 62 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -419,3 +419,65 @@ emp_no:integer | job_positions:keyword
419419
10001 | Accountant
420420
10001 | Senior Python Developer
421421
;
422+
423+
testMvExpandInconsistentColumnOrder1
424+
required_capability: fix_mv_expand_inconsistent_column_order
425+
from message_types
426+
| eval foo_1 = 1, foo_2 = 2
427+
| sort message
428+
| mv_expand foo_1
429+
;
430+
431+
message:keyword | type:keyword | foo_1:integer | foo_2:integer
432+
Connected to 10.1.0.1 | Success | 1 | 2
433+
Connected to 10.1.0.2 | Success | 1 | 2
434+
Connected to 10.1.0.3 | Success | 1 | 2
435+
Connection error | Error | 1 | 2
436+
Development environment | Development | 1 | 2
437+
Disconnected | Disconnected | 1 | 2
438+
Production environment | Production | 1 | 2
439+
;
440+
441+
testMvExpandInconsistentColumnOrder2
442+
required_capability: fix_mv_expand_inconsistent_column_order
443+
from message_types
444+
| eval foo_1 = [1, 3], foo_2 = 2
445+
| sort message
446+
| mv_expand foo_1
447+
;
448+
449+
message:keyword | type:keyword | foo_1:integer | foo_2:integer
450+
Connected to 10.1.0.1 | Success | 1 | 2
451+
Connected to 10.1.0.1 | Success | 3 | 2
452+
Connected to 10.1.0.2 | Success | 1 | 2
453+
Connected to 10.1.0.2 | Success | 3 | 2
454+
Connected to 10.1.0.3 | Success | 1 | 2
455+
Connected to 10.1.0.3 | Success | 3 | 2
456+
Connection error | Error | 1 | 2
457+
Connection error | Error | 3 | 2
458+
Development environment | Development | 1 | 2
459+
Development environment | Development | 3 | 2
460+
Disconnected | Disconnected | 1 | 2
461+
Disconnected | Disconnected | 3 | 2
462+
Production environment | Production | 1 | 2
463+
Production environment | Production | 3 | 2
464+
;
465+
466+
testMvExpandInconsistentColumnOrder3
467+
required_capability: fix_mv_expand_inconsistent_column_order
468+
from message_types
469+
| sort type
470+
| eval language_code = 1, `language_name` = false, message = true, foo_3 = 1, foo_2 = null
471+
| eval foo_3 = "1", `foo_3` = -1, foo_1 = 1, `language_code` = null, `foo_2` = "1"
472+
| mv_expand foo_1
473+
| limit 5
474+
;
475+
476+
type:keyword | language_name:boolean | message:boolean | foo_3:integer | foo_1:integer | language_code:null | foo_2:keyword
477+
Development | false | true | -1 | 1 | null | 1
478+
Disconnected | false | true | -1 | 1 | null | 1
479+
Error | false | true | -1 | 1 | null | 1
480+
Production | false | true | -1 | 1 | null | 1
481+
Success | false | true | -1 | 1 | null | 1
482+
;
483+

x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/action/EsqlCapabilities.java

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1235,6 +1235,12 @@ public enum Cap {
12351235
*/
12361236
NO_PLAIN_STRINGS_IN_LITERALS,
12371237

1238+
/**
1239+
* Support for the mv_expand target attribute should be retained in its original position.
1240+
* see <a href="https://github.com/elastic/elasticsearch/issues/129000"> ES|QL: inconsistent column order #129000 </a>
1241+
*/
1242+
FIX_MV_EXPAND_INCONSISTENT_COLUMN_ORDER,
1243+
12381244
/**
12391245
* (Re)Added EXPLAIN command
12401246
*/

x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/optimizer/rules/physical/ProjectAwayColumns.java

Lines changed: 13 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -76,7 +76,19 @@ public PhysicalPlan apply(PhysicalPlan plan) {
7676

7777
// no need for projection when dealing with aggs
7878
if (logicalFragment instanceof Aggregate == false) {
79-
List<Attribute> output = new ArrayList<>(requiredAttrBuilder.build());
79+
// we should respect the order of the attributes
80+
List<Attribute> output = new ArrayList<>();
81+
for (Attribute attribute : logicalFragment.output()) {
82+
if (requiredAttrBuilder.contains(attribute)) {
83+
output.add(attribute);
84+
requiredAttrBuilder.remove(attribute);
85+
}
86+
}
87+
// requiredAttrBuilder should be empty unless the plan is inconsistent due to a bug.
88+
// This can happen in case of remote ENRICH, see https://github.com/elastic/elasticsearch/issues/118531
89+
// TODO: stop adding the remaining required attributes once remote ENRICH is fixed.
90+
output.addAll(requiredAttrBuilder.build());
91+
8092
// if all the fields are filtered out, it's only the count that matters
8193
// however until a proper fix (see https://github.com/elastic/elasticsearch/issues/98703)
8294
// add a synthetic field (so it doesn't clash with the user defined one) to return a constant

0 commit comments

Comments
 (0)