Skip to content

Commit d1fd990

Browse files
authored
ESQL: Replace any Attribute type when pushing down past Project (elastic#135295) (elastic#136275)
This will allow any attribute type be replaced in node that's being pushed down past a `Project`. So far only `ReferenceAttribute`s were replaced. Some nodes reference other types, like `FieldAttribute` (in `OrderBy`), which would otherwise be pushed down unchanged, and thus possibly wrongly (such as when the `Project` renames _to_ the field attribute name, thus showdowing it). Closes elastic#134407
1 parent b2b5ab2 commit d1fd990

File tree

6 files changed

+83
-5
lines changed

6 files changed

+83
-5
lines changed

docs/changelog/135295.yaml

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,6 @@
1+
pr: 135295
2+
summary: Replace any Attribute type when pushing down past Project
3+
area: ES|QL
4+
type: bug
5+
issues:
6+
- 134407

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

Lines changed: 19 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -5610,3 +5610,22 @@ id_int:integer | name_str:keyword | extra1:keyword | other1:keyword | other2:int
56105610
13 | Mia | thud | xi | 14000
56115611
14 | Nina | foo2 | omicron | 15000
56125612
;
5613+
5614+
// https://github.com/elastic/elasticsearch/issues/134407
5615+
SortOnFieldNullifiedDueToJoinKeyMissingInSomeOfTheIndices
5616+
required_capability: join_lookup_v12
5617+
required_capability: fixed_pushdown_past_project_with_attributes_resolution
5618+
5619+
from sample_data,languages_mixed_numerics,apps
5620+
| rename language_code_byte as language_code
5621+
| lookup join languages_lookup on language_code
5622+
| rename id as language_code // `id` can be missing locally, so "joined" `language_code` is eval'd to null hereafter
5623+
| lookup join languages_lookup on language_code
5624+
| sort language_code_short DESC NULLS LAST, name ASC NULLS LAST, language_name
5625+
| limit 2
5626+
;
5627+
5628+
@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
5629+
null |null |null |null |32767.0 |32767.0 |32768.0 |32767 |32767 |32767.0 |32767 |null |null |null |null
5630+
null |null |null |null |128.0 |128.0 |128.0 |128 |128 |128.0 |128 |null |null |null |null
5631+
;

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

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -544,6 +544,11 @@ public enum Cap {
544544
*/
545545
FIXED_PUSHDOWN_PAST_PROJECT,
546546

547+
/**
548+
* When resolving renames, consider all {@code Attribute}s in the plan, not just the {@code ReferenceAttribute}s.
549+
*/
550+
FIXED_PUSHDOWN_PAST_PROJECT_WITH_ATTRIBUTES_RESOLUTION,
551+
547552
/**
548553
* Adds the {@code MV_PSERIES_WEIGHTED_SUM} function for converting sorted lists of numbers into
549554
* a bounded score. This is a generalization of the

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

Lines changed: 5 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -30,9 +30,11 @@ public final class RuleUtils {
3030
private RuleUtils() {}
3131

3232
/**
33-
* Returns a tuple of two lists:
34-
* 1. A list of aliases to null literals for those data types in the {@param outputAttributes} that {@param shouldBeReplaced}.
35-
* 2. A list of named expressions where attributes that match the predicate are replaced with their corresponding null alias.
33+
* @return a tuple of two lists:
34+
* <ol>
35+
* <li>A list of aliases to null literals for those data types in the {@code outputAttributes} that {@code shouldBeReplaced}.</li>
36+
* <li>A list of named expressions where attributes that match the predicate are replaced with their corresponding null alias.</li>
37+
* </ol>
3638
*
3739
* @param outputAttributes The original output attributes.
3840
* @param shouldBeReplaced A predicate to determine which attributes should be replaced with null aliases.

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;
@@ -5737,6 +5739,51 @@ public void testPushShadowingGeneratingPlanPastRenamingProjectWithResolution() {
57375739
}
57385740
}
57395741

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

0 commit comments

Comments
 (0)