Skip to content
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

ESQL: continue resolving attributes for Eval #99601

Merged
merged 3 commits into from
Sep 19, 2023
Merged

ESQL: continue resolving attributes for Eval #99601

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
f1bc083
Continue resolving attributes for eval expressions
astefan Sep 14, 2023
5c356e1
Merge branch 'main' of https://github.com/elastic/elasticsearch into …
astefan Sep 14, 2023
e88a326
Update docs/changelog/99601.yaml
astefan Sep 14, 2023
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
6 changes: 6 additions & 0 deletions docs/changelog/99601.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
pr: 99601
summary: "ESQL: continue resolving attributes for Eval"
area: ES|QL
type: bug
issues:
- 99576
33 changes: 33 additions & 0 deletions x-pack/plugin/esql/qa/testFixtures/src/main/resources/eval.csv-spec
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -166,3 +166,36 @@ emp_no:i | sum:i
10002 | 3230647
10003 | 3230970
;

chainedEvalReusingPreviousValue
from employees | sort emp_no | eval x1 = concat(first_name, "."), x2 = concat(x1, "."), x3 = concat(x2, ".") | keep x*, first_name | limit 5;

x1:keyword | x2:keyword | x3:keyword |first_name:keyword
Georgi. |Georgi.. |Georgi... |Georgi
Bezalel. |Bezalel.. |Bezalel... |Bezalel
Parto. |Parto.. |Parto... |Parto
Chirstian. |Chirstian.. |Chirstian... |Chirstian
Kyoichi. |Kyoichi.. |Kyoichi... |Kyoichi
;

chainedEvalReusingPreviousValue2
from employees | sort emp_no | eval x1 = concat(first_name, "."), x2 = concat(x1, last_name), x3 = concat(x2, gender) | keep x*, first_name, gender | limit 5;

x1:keyword | x2:keyword | x3:keyword |first_name:keyword|gender:keyword
Georgi. |Georgi.Facello |Georgi.FacelloM |Georgi |M
Bezalel. |Bezalel.Simmel |Bezalel.SimmelF |Bezalel |F
Parto. |Parto.Bamford |Parto.BamfordM |Parto |M
Chirstian. |Chirstian.Koblick|Chirstian.KoblickM|Chirstian |M
Kyoichi. |Kyoichi.Maliniak |Kyoichi.MaliniakM |Kyoichi |M
;

chainedEvalReusingPreviousValue3
from employees | sort emp_no | eval x1 = concat(first_name, "."), x2 = concat(x1, last_name), x3 = concat(x2, x1) | keep x*, first_name | limit 5;

x1:keyword | x2:keyword | x3:keyword |first_name:keyword
Georgi. |Georgi.Facello |Georgi.FacelloGeorgi. |Georgi
Bezalel. |Bezalel.Simmel |Bezalel.SimmelBezalel. |Bezalel
Parto. |Parto.Bamford |Parto.BamfordParto. |Parto
Chirstian. |Chirstian.Koblick|Chirstian.KoblickChirstian.|Chirstian
Kyoichi. |Kyoichi.Maliniak |Kyoichi.MaliniakKyoichi. |Kyoichi
;
12 changes: 8 additions & 4 deletions x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/analysis/Analyzer.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -323,13 +323,17 @@ protected LogicalPlan doRule(LogicalPlan plan) {
return resolveEnrich(p, childrenOutput);
}

return plan.transformExpressionsUp(UnresolvedAttribute.class, ua -> resolveAttribute(ua, childrenOutput));
return plan.transformExpressionsUp(UnresolvedAttribute.class, ua -> maybeResolveAttribute(ua, childrenOutput));
}

private Attribute resolveAttribute(UnresolvedAttribute ua, List<Attribute> childrenOutput) {
private Attribute maybeResolveAttribute(UnresolvedAttribute ua, List<Attribute> childrenOutput) {
if (ua.customMessage()) {
return ua;
}
Comment on lines 330 to 332
Copy link
Contributor Author

Choose a reason for hiding this comment

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

In essence, skip this part (quickly returning from resolving an Attribute) and let eval have another chance at resolving expressions that use attributes from previous eval expressions.

return resolveAttribute(ua, childrenOutput);
}

private Attribute resolveAttribute(UnresolvedAttribute ua, List<Attribute> childrenOutput) {
Attribute resolved = ua;
var named = resolveAgainstList(ua, childrenOutput);
// if resolved, return it; otherwise keep it in place to be resolved later
Expand Down Expand Up @@ -441,7 +445,7 @@ private LogicalPlan resolveRename(Rename rename, List<Attribute> childrenOutput)
// remove attributes overwritten by a renaming: `| keep a, b, c | rename a as b`
projections.removeIf(x -> x.name().equals(alias.name()));

var resolved = resolveAttribute(ua, childrenOutput);
var resolved = maybeResolveAttribute(ua, childrenOutput);
if (resolved instanceof UnsupportedAttribute || resolved.resolved()) {
var realiased = (NamedExpression) alias.replaceChildren(List.of(resolved));
projections.replaceAll(x -> x.equals(resolved) ? realiased : x);
Expand Down Expand Up @@ -490,7 +494,7 @@ private LogicalPlan resolveRename(Rename rename, List<Attribute> childrenOutput)
private LogicalPlan resolveEnrich(Enrich enrich, List<Attribute> childrenOutput) {

if (enrich.matchField().toAttribute() instanceof UnresolvedAttribute ua) {
Attribute resolved = resolveAttribute(ua, childrenOutput);
Attribute resolved = maybeResolveAttribute(ua, childrenOutput);
if (resolved.equals(ua)) {
return enrich;
}
Expand Down
19 changes: 19 additions & 0 deletions x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/analysis/AnalyzerTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@

import java.util.List;
import java.util.Map;
import java.util.stream.IntStream;

import static org.elasticsearch.xpack.esql.EsqlTestUtils.as;
import static org.elasticsearch.xpack.esql.analysis.AnalyzerTestUtils.analyze;
Expand Down Expand Up @@ -1254,6 +1255,24 @@ public void testEnrichExcludesPolicyKey() {
assertThat(e.getMessage(), containsString("Unknown column [id]"));
}

public void testChainedEvalFieldsUse() {
var query = "from test | eval x0 = pow(salary, 1), x1 = pow(x0, 2), x2 = pow(x1, 3)";
int additionalEvals = randomIntBetween(0, 5);
for (int i = 0, j = 3; i < additionalEvals; i++, j++) {
query += ", x" + j + " = pow(x" + (j - 1) + ", " + i + ")";
}
assertProjection(query + " | keep x*", IntStream.range(0, additionalEvals + 3).mapToObj(v -> "x" + v).toArray(String[]::new));
}

public void testMissingAttributeException_InChainedEval() {
var e = expectThrows(VerificationException.class, () -> analyze("""
from test
| eval x1 = concat(first_name, "."), x2 = concat(x1, last_name), x3 = concat(x2, x1), x4 = concat(x3, x5)
| keep x*
"""));
assertThat(e.getMessage(), containsString("Unknown column [x5], did you mean any of [x1, x2, x3]?"));
}

private void verifyUnsupported(String query, String errorMessage) {
verifyUnsupported(query, errorMessage, "mapping-multi-field-variation.json");
}
Expand Down