Skip to content

Commit aa8133f

Browse files
committed
Replace any Attribute type when pushing down past Project
This will allow any attribute type be replaced in rule that's being pushed down past a Project. So far only `ReferenceAttribute`s were replaced. Some nodes reference other types, like `FieldAttribute` (like `OrderBy`), which would otherwise be pushed down unchanged, and thus wrongly.
1 parent a15c735 commit aa8133f

File tree

3 files changed

+66
-2
lines changed

3 files changed

+66
-2
lines changed

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

Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -5598,3 +5598,21 @@ id_int:integer | name_str:keyword | extra1:keyword | other1:keyword | other2:int
55985598
13 | Mia | thud | xi | 14000
55995599
14 | Nina | foo2 | omicron | 15000
56005600
;
5601+
5602+
// https://github.com/elastic/elasticsearch/issues/134407
5603+
SortOnFieldNullifiedDueToJoinKeyMissingInSomeOfTheIndices
5604+
required_capability: join_lookup_v12
5605+
5606+
from sample_data,languages_mixed_numerics,apps
5607+
| rename language_code_byte as language_code
5608+
| lookup join languages_lookup on language_code
5609+
| rename id as language_code // `id` can be missing locally, so "joined" `language_code` is eval'd to null hereafter
5610+
| lookup join languages_lookup on language_code
5611+
| sort language_code_short DESC NULLS LAST, name ASC NULLS LAST, language_name
5612+
| limit 2
5613+
;
5614+
5615+
@timestamp:datetime|client_ip:ip|event_duration:l| language_code:i | language_code_double:d | language_code_float:d |language_code_half_float:d|language_code_integer:i| language_code_long:l |language_code_scaled_float:d|language_code_short:i| message:keyword | name:keyword | version:version | language_name:keyword
5616+
null |null |null |null |32767.0 |32767.0 |32768.0 |32767 |32767 |32767.0 |32767 |null |null |null |null
5617+
null |null |null |null |128.0 |128.0 |128.0 |128 |128 |128.0 |128 |null |null |null |null
5618+
;

x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/optimizer/rules/logical/PushDownUtils.java

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -14,7 +14,6 @@
1414
import org.elasticsearch.xpack.esql.core.expression.Expression;
1515
import org.elasticsearch.xpack.esql.core.expression.Expressions;
1616
import org.elasticsearch.xpack.esql.core.expression.NamedExpression;
17-
import org.elasticsearch.xpack.esql.core.expression.ReferenceAttribute;
1817
import org.elasticsearch.xpack.esql.expression.Order;
1918
import org.elasticsearch.xpack.esql.plan.GeneratingPlan;
2019
import org.elasticsearch.xpack.esql.plan.logical.Eval;
@@ -206,7 +205,7 @@ private static <P extends LogicalPlan> P resolveRenamesFromProject(P plan, Proje
206205

207206
@SuppressWarnings("unchecked")
208207
public static <P extends LogicalPlan> P resolveRenamesFromMap(P plan, AttributeMap<Expression> map) {
209-
return (P) plan.transformExpressionsOnly(ReferenceAttribute.class, r -> map.resolve(r, r));
208+
return (P) plan.transformExpressionsOnly(Attribute.class, r -> map.resolve(r, r));
210209
}
211210

212211
private record AttributeReplacement(List<Expression> rewrittenExpressions, AttributeMap<Alias> replacedAttributes) {}

x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/optimizer/LogicalPlanOptimizerTests.java

Lines changed: 47 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -97,6 +97,7 @@
9797
import org.elasticsearch.xpack.esql.optimizer.rules.logical.OptimizerRules;
9898
import org.elasticsearch.xpack.esql.optimizer.rules.logical.PruneRedundantOrderBy;
9999
import org.elasticsearch.xpack.esql.optimizer.rules.logical.PushDownAndCombineLimits;
100+
import org.elasticsearch.xpack.esql.optimizer.rules.logical.PushDownAndCombineOrderBy;
100101
import org.elasticsearch.xpack.esql.optimizer.rules.logical.PushDownEnrich;
101102
import org.elasticsearch.xpack.esql.optimizer.rules.logical.PushDownEval;
102103
import org.elasticsearch.xpack.esql.optimizer.rules.logical.PushDownInferencePlan;
@@ -165,6 +166,7 @@
165166
import static org.elasticsearch.xpack.esql.EsqlTestUtils.localSource;
166167
import static org.elasticsearch.xpack.esql.EsqlTestUtils.randomLiteral;
167168
import static org.elasticsearch.xpack.esql.EsqlTestUtils.referenceAttribute;
169+
import static org.elasticsearch.xpack.esql.EsqlTestUtils.relation;
168170
import static org.elasticsearch.xpack.esql.EsqlTestUtils.singleValue;
169171
import static org.elasticsearch.xpack.esql.EsqlTestUtils.withDefaultLimitWarning;
170172
import static org.elasticsearch.xpack.esql.analysis.Analyzer.NO_FIELDS;
@@ -5738,6 +5740,51 @@ public void testPushShadowingGeneratingPlanPastRenamingProjectWithResolution() {
57385740
}
57395741
}
57405742

5743+
/*
5744+
* Test for: https://github.com/elastic/elasticsearch/issues/134407
5745+
*
5746+
* Input:
5747+
* OrderBy[[Order[a{f}#2,ASC,ANY]]]
5748+
* \_Project[[aTemp{r}#3 AS a#2]]
5749+
* \_Eval[[null[INTEGER] AS aTemp#3]]
5750+
* \_EsRelation[uYiPqAFD][LOOKUP][a{f}#2]
5751+
*
5752+
* Output:
5753+
* Project[[aTemp{r}#3 AS a#2]]
5754+
* \_OrderBy[[Order[aTemp{r}#3,ASC,ANY]]]
5755+
* \_Eval[[null[INTEGER] AS aTemp#3]]
5756+
* \_EsRelation[uYiPqAFD][LOOKUP][a{f}#2]
5757+
*/
5758+
public void testPushDownOrderByPastRename() {
5759+
FieldAttribute a = getFieldAttribute("a");
5760+
EsRelation relation = relation().withAttributes(List.of(a));
5761+
5762+
Alias aTemp = new Alias(EMPTY, "aTemp", new Literal(EMPTY, null, a.dataType()));
5763+
Eval eval = new Eval(EMPTY, relation, List.of(aTemp));
5764+
5765+
// Rename the null literal to "a" so that the OrderBy can refer to it. Requires re-using the id of original "a" attribute.
5766+
Alias aliasA = new Alias(EMPTY, "a", aTemp.toAttribute(), a.id());
5767+
Project project = new Project(EMPTY, eval, List.of(aliasA));
5768+
5769+
// OrderBy sorts on original `a` attribute; after pushing down it should sort on aTemp.
5770+
OrderBy orderBy = new OrderBy(EMPTY, project, List.of(new Order(EMPTY, a, Order.OrderDirection.ASC, Order.NullsPosition.ANY)));
5771+
5772+
LogicalPlan optimized = new PushDownAndCombineOrderBy().apply(orderBy);
5773+
5774+
var projectOut = as(optimized, Project.class);
5775+
assertThat(projectOut.projections(), equalTo(project.projections()));
5776+
var orderByOutput = as(projectOut.child(), OrderBy.class);
5777+
var orderAttr = as(orderByOutput.order().getFirst().child(), ReferenceAttribute.class);
5778+
5779+
// the actual fix test
5780+
assertThat(orderAttr.name(), equalTo("aTemp"));
5781+
assertThat(orderAttr.id(), equalTo(aTemp.id()));
5782+
5783+
var evalOutput = as(orderByOutput.child(), Eval.class);
5784+
assertThat(evalOutput.fields(), equalTo(eval.fields()));
5785+
assertThat(evalOutput.child(), equalTo(relation));
5786+
}
5787+
57415788
/**
57425789
* Expects
57435790
* <pre>{@code

0 commit comments

Comments
 (0)