Skip to content

Fix union-types when aggregating on inline conversion function #110652

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
craigtaverner merged 1 commit into from
Jul 10, 2024
Merged

Fix union-types when aggregating on inline conversion function #110652

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
15 changes: 15 additions & 0 deletions x-pack/plugin/esql/qa/testFixtures/src/main/resources/union_types.csv-spec
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -514,6 +514,21 @@ mc:l | count:l
7 | 2
;

multiIndexTsLongStatsStats
Copy link
Copy Markdown
Contributor Author

@craigtaverner craigtaverner Jul 9, 2024

Choose a reason for hiding this comment

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

This is not the test that tests the fix in the PR; but is a useful test that was used to flush out some related behaviors, so I wanted to keep it. For the test that really matters, look up at line 284, which uses the capability union_types_inline_fix, and was already merged as a result of the previous fix, but had the wrong capability name, and so was ignored and we missed the failure. This PR adds that capability name, enabling this test, and fixes the test.

required_capability: union_types
required_capability: union_types_agg_cast

FROM sample_data, sample_data_ts_long
| EVAL ts = TO_STRING(@timestamp)
| STATS count = COUNT(*) BY ts
| STATS mc = COUNT(count) BY count
| SORT mc DESC, count ASC
;

mc:l | count:l
14 | 1
;

multiIndexTsLongRenameStats
required_capability: union_types

Expand Down
7 changes: 6 additions & 1 deletion x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/action/EsqlCapabilities.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -117,7 +117,12 @@ public enum Cap {
* Fix to GROK validation in case of multiple fields with same name and different types
* https://github.com/elastic/elasticsearch/issues/110533
*/
GROK_VALIDATION;
GROK_VALIDATION,

/**
* Fix for union-types when aggregating over an inline conversion with conversion function. Done in #110652.
*/
UNION_TYPES_INLINE_FIX;
Copy link
Copy Markdown
Contributor Author

@craigtaverner craigtaverner Jul 9, 2024

Choose a reason for hiding this comment

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

The test on line 284 of union_types.csv-spec was already requiring this capability (and therefor ignored). We now enabled this capability and fix the failing test that resulted.


private final boolean snapshotOnly;

Expand Down
13 changes: 8 additions & 5 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 @@ -1088,15 +1088,19 @@ protected LogicalPlan doRule(LogicalPlan plan) {

// In ResolveRefs the aggregates are resolved from the groupings, which might have an unresolved MultiTypeEsField.
// Now that we have resolved those, we need to re-resolve the aggregates.
if (plan instanceof EsqlAggregate agg && agg.expressionsResolved() == false) {
if (plan instanceof EsqlAggregate agg) {
// If the union-types resolution occurred in a child of the aggregate, we need to check the groupings
plan = agg.transformExpressionsOnly(FieldAttribute.class, UnresolveUnionTypes::checkUnresolved);

// Aggregates where the grouping key comes from a union-type field need to be resolved against the grouping key
Map<Attribute, Expression> resolved = new HashMap<>();
for (Expression e : agg.groupings()) {
Attribute attr = Expressions.attribute(e);
if (attr != null && attr.resolved()) {
resolved.put(attr, e);
}
}
plan = agg.transformExpressionsOnly(UnresolvedAttribute.class, ua -> resolveAttribute(ua, resolved));
plan = plan.transformExpressionsOnly(UnresolvedAttribute.class, ua -> resolveAttribute(ua, resolved));
}

// Otherwise drop the converted attributes after the alias function, as they are only needed for this function, and
Expand Down Expand Up @@ -1222,9 +1226,8 @@ protected LogicalPlan rule(LogicalPlan plan) {
return plan.transformExpressionsOnly(FieldAttribute.class, UnresolveUnionTypes::checkUnresolved);
}

private static Attribute checkUnresolved(FieldAttribute fa) {
var field = fa.field();
if (field instanceof InvalidMappedField imf) {
static Attribute checkUnresolved(FieldAttribute fa) {
if (fa.field() instanceof InvalidMappedField imf) {
String unresolvedMessage = "Cannot use field [" + fa.name() + "] due to ambiguities being " + imf.errorMessage();
return new UnresolvedAttribute(fa.source(), fa.name(), fa.qualifier(), fa.id(), unresolvedMessage, null);
}
Expand Down