Skip to content

[ES|QL] Resolve groupings in aggregate before resolving references to groupings in the aggregations #127524

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
merged 10 commits into from
May 6, 2025
Merged

[ES|QL] Resolve groupings in aggregate before resolving references to groupings in the aggregations #127524

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
f5377c3
resolve groupings before resolving references to groupings in the agg…
fang-xing-esql Apr 29, 2025
ca1ebee
Update docs/changelog/127524.yaml
fang-xing-esql Apr 29, 2025
e8f8c46
groupings referenced in aggregations explicitly
fang-xing-esql Apr 29, 2025
9f014a0
fix typo
fang-xing-esql Apr 29, 2025
d57caf7
refactor
fang-xing-esql Apr 30, 2025
827787f
Merge branch 'main' into resolve-aggregate-grouping
fang-xing-esql Apr 30, 2025
ef989e9
add CsvTests
fang-xing-esql May 1, 2025
c00d62c
Merge branch 'main' into resolve-aggregate-grouping
fang-xing-esql May 1, 2025
7175648
clean up outdated comments
fang-xing-esql May 1, 2025
16de00e
Merge branch 'main' into resolve-aggregate-grouping
fang-xing-esql May 5, 2025
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/127524.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
pr: 127524
summary: Resolve groupings in aggregate before resolving references to groupings in
the aggregations
area: ES|QL
type: bug
issues: []
48 changes: 35 additions & 13 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 @@ -20,6 +20,7 @@
import org.elasticsearch.xpack.esql.analysis.AnalyzerRules.ParameterizedAnalyzerRule;
import org.elasticsearch.xpack.esql.common.Failure;
import org.elasticsearch.xpack.esql.core.capabilities.Resolvables;
import org.elasticsearch.xpack.esql.core.capabilities.Unresolvable;
import org.elasticsearch.xpack.esql.core.expression.Alias;
import org.elasticsearch.xpack.esql.core.expression.Attribute;
import org.elasticsearch.xpack.esql.core.expression.EmptyAttribute;
Expand Down Expand Up @@ -173,13 +174,13 @@ public class Analyzer extends ParameterizedRuleExecutor<LogicalPlan, AnalyzerCon
),
new Batch<>(
"Resolution",
new ResolveRefs(),
/*
* ImplicitCasting must be before ResolveRefs. Because a reference is created for a Bucket in Aggregate's aggregates,
* resolving this reference before implicit casting may cause this reference to have customMessage=true, it prevents further
* attempts to resolve this reference.
*/
Copy link
Member

@costin costin Apr 30, 2025

Choose a reason for hiding this comment

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

  1. why does the order matter now vs before.
  2. the comment no longer hold true (looks like it was a harbinger of things to come).

Copy link
Member Author

@fang-xing-esql fang-xing-esql May 1, 2025

Choose a reason for hiding this comment

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

The reason that ImplicitCasting was put before ResolveRefs is exactly because that resolveAggregate was attempting to resolve the references to the groupings, before the groupings are resolved, and it causes the references to the groupings marked unresolvable too early. This comment is outdated and removed. Thanks for the reminder!

new ImplicitCasting(),
new ResolveRefs(),
new ResolveUnionTypes() // Must be after ResolveRefs, so union types can be found
),
new Batch<>("Finish Analysis", Limiter.ONCE, new AddImplicitLimit(), new AddImplicitForkLimit(), new UnionTypesCleanup())
Expand Down Expand Up @@ -574,7 +575,7 @@ private Aggregate resolveAggregate(Aggregate aggregate, List<Attribute> children
}
}

if (Resolvables.resolved(groupings) == false || (Resolvables.resolved(aggregates) == false)) {
if (Resolvables.resolved(groupings) == false || Resolvables.resolved(aggregates) == false) {
ArrayList<Attribute> resolved = new ArrayList<>();
for (Expression e : groupings) {
Attribute attr = Expressions.attribute(e);
Expand All @@ -585,17 +586,38 @@ private Aggregate resolveAggregate(Aggregate aggregate, List<Attribute> children
List<Attribute> resolvedList = NamedExpressions.mergeOutputAttributes(resolved, childrenOutput);

List<NamedExpression> newAggregates = new ArrayList<>();
for (NamedExpression ag : aggregate.aggregates()) {
var agg = (NamedExpression) ag.transformUp(UnresolvedAttribute.class, ua -> {
Expression ne = ua;
Attribute maybeResolved = maybeResolveAttribute(ua, resolvedList);
if (maybeResolved != null) {
changed.set(true);
ne = maybeResolved;
}
return ne;
});
newAggregates.add(agg);
// If the groupings are not resolved, skip the resolution of the references to groupings in the aggregates, resolve the
// aggregations that do not reference to groupings, so that the fields/attributes referenced by the aggregations can be
// resolved, and verifier doesn't report field/reference/column not found errors for them.
boolean groupingResolved = Resolvables.resolved(groupings);
int size = groupingResolved ? aggregates.size() : aggregates.size() - groupings.size();
for (int i = 0; i < aggregates.size(); i++) {
NamedExpression ag = aggregates.get(i);
Copy link
Member

@costin costin Apr 30, 2025

Choose a reason for hiding this comment

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

Please use better naming than agg and ag (e.g. maybeResolvedAgg).

if (i < size) {
var agg = (NamedExpression) ag.transformUp(UnresolvedAttribute.class, ua -> {
Expression ne = ua;
Attribute maybeResolved = maybeResolveAttribute(ua, resolvedList);
if (groupingResolved) {
if (maybeResolved != null) {
changed.set(true);
ne = maybeResolved;
}
} else {
// An item in aggregations can reference to groupings explicitly, if groupings are not resolved yet and
// maybeResolved is not resolved, return the original UnresolvedAttribute, so that it has a another chance
// to get resolved in the next iteration.
// For example STATS c = count(emp_no), x = d::int + 1 BY d = (date == "2025-01-01")
if (maybeResolved instanceof Unresolvable == false) {
changed.set(true);
ne = maybeResolved;
}
}
Copy link
Member

@costin costin Apr 30, 2025

Choose a reason for hiding this comment

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

maybeResolveAttribute never returns null
Furthermore the if/else can be simplified, e.g.

if (groupingResolved || maybeResolved.resolved() == false) {
   changed.set(true);
  ne = maybeResolved;
}

return ne;
});
newAggregates.add(agg);
} else {
newAggregates.add(ag); // Groupings are not resolved
}
Copy link
Member

@costin costin Apr 30, 2025

Choose a reason for hiding this comment

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

Same here - instead of doing an if/else perform the condition only on updating agg and always update it similar to the previous code style:

var agg = ...
if (groupingResolved) {
   agg = 
}

newAggregates.add(agg);

}

// TODO: remove this when Stats interface is removed
Expand Down
150 changes: 149 additions & 1 deletion 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 @@ -47,8 +47,11 @@
import org.elasticsearch.xpack.esql.expression.function.fulltext.MatchOperator;
import org.elasticsearch.xpack.esql.expression.function.fulltext.MultiMatch;
import org.elasticsearch.xpack.esql.expression.function.fulltext.QueryString;
import org.elasticsearch.xpack.esql.expression.function.grouping.Bucket;
import org.elasticsearch.xpack.esql.expression.function.scalar.convert.ToInteger;
import org.elasticsearch.xpack.esql.expression.function.scalar.string.Concat;
import org.elasticsearch.xpack.esql.expression.function.scalar.string.Substring;
import org.elasticsearch.xpack.esql.expression.predicate.operator.arithmetic.Add;
import org.elasticsearch.xpack.esql.expression.predicate.operator.comparison.Equals;
import org.elasticsearch.xpack.esql.expression.predicate.operator.comparison.GreaterThan;
import org.elasticsearch.xpack.esql.index.EsIndex;
Expand Down Expand Up @@ -80,6 +83,7 @@
import org.elasticsearch.xpack.esql.session.IndexResolver;

import java.io.IOException;
import java.time.Period;
import java.util.ArrayList;
import java.util.List;
import java.util.Map;
Expand Down Expand Up @@ -110,6 +114,9 @@
import static org.elasticsearch.xpack.esql.analysis.AnalyzerTestUtils.randomValueOtherThanTest;
import static org.elasticsearch.xpack.esql.analysis.AnalyzerTestUtils.tsdbIndexResolution;
import static org.elasticsearch.xpack.esql.core.tree.Source.EMPTY;
import static org.elasticsearch.xpack.esql.core.type.DataType.DATETIME;
import static org.elasticsearch.xpack.esql.core.type.DataType.DATE_PERIOD;
import static org.elasticsearch.xpack.esql.type.EsqlDataTypeConverter.dateTimeToString;
import static org.hamcrest.Matchers.contains;
import static org.hamcrest.Matchers.containsString;
import static org.hamcrest.Matchers.empty;
Expand Down Expand Up @@ -368,7 +375,7 @@ public void testNoProjection() {
DataType.INTEGER,
DataType.KEYWORD,
DataType.TEXT,
DataType.DATETIME,
DATETIME,
DataType.TEXT,
DataType.KEYWORD,
DataType.INTEGER,
Expand Down Expand Up @@ -3776,6 +3783,147 @@ public void testResolveCompletionOutputField() {
assertThat(getAttributeByName(esRelation.output(), "description"), not(equalTo(completion.targetField())));
}

public void testResolveGroupingsBeforeResolvingImplicitReferencesToGroupings() {
var plan = analyze("""
FROM test
| EVAL date = "2025-01-01"::datetime
| STATS c = count(emp_no) BY d = (date == "2025-01-01")
""", "mapping-default.json");

var limit = as(plan, Limit.class);
var agg = as(limit.child(), Aggregate.class);
var aggregates = agg.aggregates();
assertThat(aggregates, hasSize(2));
Alias a = as(aggregates.get(0), Alias.class);
assertEquals("c", a.name());
Count c = as(a.child(), Count.class);
FieldAttribute fa = as(c.field(), FieldAttribute.class);
assertEquals("emp_no", fa.name());
ReferenceAttribute ra = as(aggregates.get(1), ReferenceAttribute.class); // reference in aggregates is resolved
assertEquals("d", ra.name());
List<Expression> groupings = agg.groupings();
assertEquals(1, groupings.size());
a = as(groupings.get(0), Alias.class); // reference in groupings is resolved
assertEquals("d", ra.name());
Equals equals = as(a.child(), Equals.class);
ra = as(equals.left(), ReferenceAttribute.class);
assertEquals("date", ra.name());
Literal literal = as(equals.right(), Literal.class);
assertEquals("2025-01-01T00:00:00.000Z", dateTimeToString(Long.parseLong(literal.value().toString())));
assertEquals(DATETIME, literal.dataType());
}

public void testResolveGroupingsBeforeResolvingExplicitReferencesToGroupings() {
var plan = analyze("""
FROM test
| EVAL date = "2025-01-01"::datetime
| STATS c = count(emp_no), x = d::int + 1 BY d = (date == "2025-01-01")
""", "mapping-default.json");

var limit = as(plan, Limit.class);
var agg = as(limit.child(), Aggregate.class);
var aggregates = agg.aggregates();
assertThat(aggregates, hasSize(3));
Alias a = as(aggregates.get(0), Alias.class);
assertEquals("c", a.name());
Count c = as(a.child(), Count.class);
FieldAttribute fa = as(c.field(), FieldAttribute.class);
assertEquals("emp_no", fa.name());
a = as(aggregates.get(1), Alias.class); // explicit reference to groupings is resolved
assertEquals("x", a.name());
Add add = as(a.child(), Add.class);
ToInteger toInteger = as(add.left(), ToInteger.class);
ReferenceAttribute ra = as(toInteger.field(), ReferenceAttribute.class);
assertEquals("d", ra.name());
ra = as(aggregates.get(2), ReferenceAttribute.class); // reference in aggregates is resolved
assertEquals("d", ra.name());
List<Expression> groupings = agg.groupings();
assertEquals(1, groupings.size());
a = as(groupings.get(0), Alias.class); // reference in groupings is resolved
assertEquals("d", ra.name());
Equals equals = as(a.child(), Equals.class);
ra = as(equals.left(), ReferenceAttribute.class);
assertEquals("date", ra.name());
Literal literal = as(equals.right(), Literal.class);
assertEquals("2025-01-01T00:00:00.000Z", dateTimeToString(Long.parseLong(literal.value().toString())));
assertEquals(DATETIME, literal.dataType());
}

public void testBucketWithIntervalInStringInBothAggregationAndGrouping() {
var plan = analyze("""
FROM test
| STATS c = count(emp_no), b = BUCKET(hire_date, "1 year") + 1 year BY yr = BUCKET(hire_date, "1 year")
""", "mapping-default.json");

var limit = as(plan, Limit.class);
var agg = as(limit.child(), Aggregate.class);
var aggregates = agg.aggregates();
assertThat(aggregates, hasSize(3));
Alias a = as(aggregates.get(0), Alias.class);
assertEquals("c", a.name());
Count c = as(a.child(), Count.class);
FieldAttribute fa = as(c.field(), FieldAttribute.class);
assertEquals("emp_no", fa.name());
a = as(aggregates.get(1), Alias.class); // explicit reference to groupings is resolved
assertEquals("b", a.name());
Add add = as(a.child(), Add.class);
Bucket bucket = as(add.left(), Bucket.class);
fa = as(bucket.field(), FieldAttribute.class);
assertEquals("hire_date", fa.name());
Literal literal = as(bucket.buckets(), Literal.class);
Literal oneYear = new Literal(EMPTY, Period.ofYears(1), DATE_PERIOD);
assertEquals(oneYear, literal);
literal = as(add.right(), Literal.class);
assertEquals(oneYear, literal);
ReferenceAttribute ra = as(aggregates.get(2), ReferenceAttribute.class); // reference in aggregates is resolved
assertEquals("yr", ra.name());
List<Expression> groupings = agg.groupings();
assertEquals(1, groupings.size());
a = as(groupings.get(0), Alias.class); // reference in groupings is resolved
assertEquals("yr", ra.name());
bucket = as(a.child(), Bucket.class);
fa = as(bucket.field(), FieldAttribute.class);
assertEquals("hire_date", fa.name());
literal = as(bucket.buckets(), Literal.class);
assertEquals(oneYear, literal);
}

public void testBucketWithIntervalInStringInGroupingReferencedInAggregation() {
var plan = analyze("""
FROM test
| STATS c = count(emp_no), b = yr + 1 year BY yr = BUCKET(hire_date, "1 year")
""", "mapping-default.json");

var limit = as(plan, Limit.class);
var agg = as(limit.child(), Aggregate.class);
var aggregates = agg.aggregates();
assertThat(aggregates, hasSize(3));
Alias a = as(aggregates.get(0), Alias.class);
assertEquals("c", a.name());
Count c = as(a.child(), Count.class);
FieldAttribute fa = as(c.field(), FieldAttribute.class);
assertEquals("emp_no", fa.name());
a = as(aggregates.get(1), Alias.class); // explicit reference to groupings is resolved
assertEquals("b", a.name());
Add add = as(a.child(), Add.class);
ReferenceAttribute ra = as(add.left(), ReferenceAttribute.class);
assertEquals("yr", ra.name());
Literal oneYear = new Literal(EMPTY, Period.ofYears(1), DATE_PERIOD);
Literal literal = as(add.right(), Literal.class);
assertEquals(oneYear, literal);
ra = as(aggregates.get(2), ReferenceAttribute.class); // reference in aggregates is resolved
assertEquals("yr", ra.name());
List<Expression> groupings = agg.groupings();
assertEquals(1, groupings.size());
a = as(groupings.get(0), Alias.class); // reference in groupings is resolved
assertEquals("yr", ra.name());
Bucket bucket = as(a.child(), Bucket.class);
fa = as(bucket.field(), FieldAttribute.class);
assertEquals("hire_date", fa.name());
literal = as(bucket.buckets(), Literal.class);
assertEquals(oneYear, literal);
}

@Override
protected IndexAnalyzers createDefaultIndexAnalyzers() {
return super.createDefaultIndexAnalyzers();
Expand Down
Loading