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.

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: Fix synthetic attribute pruning #111413

Merged
merged 51 commits into from
Sep 4, 2024
Merged
Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
51 commits
Select commit Hold shift + click to select a range
205dfad
Use AggregatorMode instead of AggregateExec.Mode
alex-spies Jul 29, 2024
1cabef2
Make AggregateExec track intermediateAttributes
alex-spies Jul 29, 2024
54e0436
Implement QueryPlan.requiredInputSet
alex-spies Jul 29, 2024
890701e
Simplify and correct ProjectAwayColumns
alex-spies Jul 29, 2024
bb0c926
Small refactor
alex-spies Jul 29, 2024
f22f3a1
Squash some minor mistakes
alex-spies Jul 29, 2024
dbc9199
Update tests
alex-spies Jul 29, 2024
741dbd7
Simplify DependencyConsistency
alex-spies Jul 30, 2024
9e4dab5
Small refactor
alex-spies Jul 30, 2024
d78f073
Tiny refactor
alex-spies Jul 30, 2024
9695190
Start using synthetic=true again
alex-spies Jul 30, 2024
c343812
Merge remote-tracking branch 'upstream/main' into fix-synthetic-attri…
alex-spies Jul 30, 2024
c5e74be
Update docs/changelog/111413.yaml
alex-spies Jul 30, 2024
ac2d6e4
Do not filter out <no-fields> in the Analyzer
alex-spies Jul 30, 2024
54e1847
Merge remote-tracking branch 'upstream/main' into fix-synthetic-attri…
alex-spies Jul 30, 2024
7db76d8
Merge remote-tracking branch 'upstream/main' into fix-synthetic-attri…
alex-spies Aug 14, 2024
01d4c5f
Fix AggregateExec ser/de tests, hash, equals
alex-spies Aug 14, 2024
5443fef
Merge remote-tracking branch 'upstream/main' into fix-synthetic-attri…
alex-spies Aug 19, 2024
ddfdd2c
Add and use new constructor for FieldAttribute
alex-spies Aug 19, 2024
e7c212e
Mark another one as synthetic
alex-spies Aug 19, 2024
3c648b3
Rename requiredInputSet->childrenReferences
alex-spies Aug 19, 2024
4f01e7c
Synthetic as :s in debug output, e.g. field{f:s}#1
alex-spies Aug 19, 2024
ff8b72d
Spotless
alex-spies Aug 19, 2024
e4b258a
Merge remote-tracking branch 'upstream/main' into fix-synthetic-attri…
alex-spies Aug 20, 2024
9ad9e1c
Revert changes to AggregateExec
alex-spies Aug 20, 2024
7ff1991
Revert "Revert changes to AggregateExec"
alex-spies Aug 20, 2024
2eabfc2
Avoid using QueryPlan.references
alex-spies Aug 20, 2024
f1b9b86
Replace references with childrenReferences
alex-spies Aug 20, 2024
63c39a7
Fix a whoopsie
alex-spies Aug 20, 2024
6028664
Improve comments
alex-spies Aug 20, 2024
e193fb1
Add new transport version for AggregateExec
alex-spies Aug 20, 2024
ac44b1b
More tests for ProjectAwayColumns
alex-spies Aug 21, 2024
41e9159
Merge remote-tracking branch 'upstream/main' into fix-synthetic-attri…
alex-spies Aug 21, 2024
febb40f
Improve comment
alex-spies Aug 21, 2024
93cf2cf
Don't make computeReferences public
alex-spies Aug 21, 2024
9dbd5ea
Checkstyle
alex-spies Aug 21, 2024
d4dc892
Reduce noise via static imports
alex-spies Aug 22, 2024
0505dc4
Synthetic label: {f$} instead of {f:s}
alex-spies Aug 22, 2024
6f36f70
Make synth attr prefix inaccessible
alex-spies Aug 22, 2024
ec67cf2
Merge remote-tracking branch 'upstream/main' into fix-synthetic-attri…
alex-spies Aug 22, 2024
259d93f
Make FieldAttribute ctor with type package private
alex-spies Aug 22, 2024
7849978
Merge remote-tracking branch 'upstream/main' into fix-synthetic-attri…
alex-spies Aug 22, 2024
3582136
Merge remote-tracking branch 'upstream/main' into fix-synthetic-attri…
alex-spies Aug 28, 2024
d1f55e3
Refactor fieldNames for METRICS + add test
alex-spies Aug 28, 2024
776ed78
Update comment
alex-spies Aug 28, 2024
77431f6
Undo refactor to fieldNames; rely on index mode
alex-spies Aug 29, 2024
7c3aaf5
Merge remote-tracking branch 'upstream/main' into fix-synthetic-attri…
alex-spies Aug 29, 2024
e557eb4
Merge remote-tracking branch 'upstream/main' into fix-synthetic-attri…
alex-spies Aug 30, 2024
cb69899
Merge remote-tracking branch 'upstream/main' into fix-synthetic-attri…
alex-spies Sep 3, 2024
f65747d
Use default implementation for computeReferences
alex-spies Sep 3, 2024
a7b8715
Rename static helpers in Aggregate and Eval
alex-spies Sep 3, 2024
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
Original file line number Diff line number Diff line change
Expand Up @@ -195,7 +195,13 @@ static TransportVersion def(int id) {
public static final TransportVersion ESQL_PROFILE_SLEEPS = def(8_725_00_0);
public static final TransportVersion ZDT_NANOS_SUPPORT = def(8_726_00_0);
public static final TransportVersion LTR_SERVERLESS_RELEASE = def(8_727_00_0);
public static final TransportVersion ESQL_AGGREGATE_EXEC_TRACKS_INTERMEDIATE_ATTRS = def(8_728_00_0);
public static final TransportVersion ALLOW_PARTIAL_SEARCH_RESULTS_IN_PIT = def(8_728_00_0);
public static final TransportVersion RANK_DOCS_RETRIEVER = def(8_729_00_0);
public static final TransportVersion ESQL_ES_FIELD_CACHED_SERIALIZATION = def(8_730_00_0);
public static final TransportVersion ADD_MANAGE_ROLES_PRIVILEGE = def(8_731_00_0);
public static final TransportVersion REPOSITORIES_TELEMETRY = def(8_732_00_0);
public static final TransportVersion ML_INFERENCE_ALIBABACLOUD_SEARCH_ADDED = def(8_733_00_0);
public static final TransportVersion ESQL_AGGREGATE_EXEC_TRACKS_INTERMEDIATE_ATTRS = def(8_734_00_0);

/*
* STOP! READ THIS FIRST! No, really,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -65,9 +65,11 @@ public FieldAttribute(
this(source, parent, name, field.getDataType(), field, nullability, id, synthetic);
}

// TODO: Should this become private? An explicitly set type will be discarded the moment this is `clone`d, so using this constructor is
// deceptive.
public FieldAttribute(
/**
* Used only for testing. Do not use this otherwise, as an explicitly set type will be ignored the next time this FieldAttribute is
* {@link FieldAttribute#clone}d.
*/
FieldAttribute(
Source source,
FieldAttribute parent,
String name,
Expand Down Expand Up @@ -197,6 +199,11 @@ protected Attribute clone(Source source, String name, DataType type, Nullability
return new FieldAttribute(source, parent, name, field, nullability, id, synthetic);
}

@Override
public Attribute withDataType(DataType type) {
throw new UnsupportedOperationException("FieldAttribute obtains its type from the contained EsField.");
}

@Override
public int hashCode() {
return Objects.hash(super.hashCode(), path, field);
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
/*
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
* or more contributor license agreements. Licensed under the Elastic License
* 2.0; you may not use this file except in compliance with the Elastic License
* 2.0.
*/

package org.elasticsearch.xpack.esql.core.expression;

import org.elasticsearch.xpack.esql.core.tree.Source;
import org.elasticsearch.xpack.esql.core.type.DataType;
import org.elasticsearch.xpack.esql.core.type.EsField;

public class FieldAttributeTestUtils {
public static final FieldAttribute newFieldAttributeWithType(
Source source,
FieldAttribute parent,
String name,
DataType type,
EsField field,
Nullability nullability,
NameId id,
boolean synthetic
) {
return new FieldAttribute(source, parent, name, type, field, nullability, id, synthetic);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -36,10 +36,9 @@ public class AggregateExec extends UnaryExec implements EstimatesRowSize {
private final List<? extends Expression> groupings;
private final List<? extends NamedExpression> aggregates;
/**
* The output attributes of {@link AggregatorMode#INITIAL} aggregations, resp.
* the input attributes of {@link AggregatorMode#FINAL} aggregations.
* The output attributes of {@link AggregatorMode#INITIAL} and {@link AggregatorMode#INTERMEDIATE} aggregations, resp.
* the input attributes of {@link AggregatorMode#FINAL} and {@link AggregatorMode#INTERMEDIATE} aggregations.
*/
// TODO: For INTERMEDIATE, should the input attributes be the same as the output attributes? Currently, they are.
private final List<Attribute> intermediateAttributes;
alex-spies marked this conversation as resolved.
Show resolved Hide resolved

private final AggregatorMode mode;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,6 @@
import org.elasticsearch.common.regex.Regex;
import org.elasticsearch.compute.operator.DriverProfile;
import org.elasticsearch.core.Releasables;
import org.elasticsearch.index.IndexMode;
import org.elasticsearch.index.query.QueryBuilder;
import org.elasticsearch.logging.LogManager;
import org.elasticsearch.logging.Logger;
Expand Down Expand Up @@ -54,7 +53,6 @@
import org.elasticsearch.xpack.esql.plan.logical.Phased;
import org.elasticsearch.xpack.esql.plan.logical.Project;
import org.elasticsearch.xpack.esql.plan.logical.RegexExtract;
import org.elasticsearch.xpack.esql.plan.logical.UnresolvedRelation;
import org.elasticsearch.xpack.esql.plan.physical.EstimatesRowSize;
import org.elasticsearch.xpack.esql.plan.physical.FragmentExec;
import org.elasticsearch.xpack.esql.plan.physical.PhysicalPlan;
Expand Down Expand Up @@ -302,9 +300,9 @@ static Set<String> fieldNames(LogicalPlan parsed, Set<String> enrichPolicyMatchF
references.addAll(enrichRefs);
} else {
references.addAll(p.references());
// TODO: Is this still needed? This branch used to live in UnresolvedRelation.references().
if (p instanceof UnresolvedRelation ur && ur.indexMode() == IndexMode.TIME_SERIES) {
alex-spies marked this conversation as resolved.
Show resolved Hide resolved
references.add(new UnresolvedAttribute(ur.source(), MetadataAttribute.TIMESTAMP_FIELD));
if (p instanceof Aggregate aggregate && aggregate.aggregateType() == Aggregate.AggregateType.METRICS) {
// The METRICS command relies on @timestamp without the user having to mention it.
references.add(new UnresolvedAttribute(aggregate.source(), MetadataAttribute.TIMESTAMP_FIELD));
}
// special handling for UnresolvedPattern (which is not an UnresolvedAttribute)
p.forEachExpression(UnresolvedNamePattern.class, up -> {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,8 @@
import org.elasticsearch.xpack.esql.core.type.EsField;
import org.elasticsearch.xpack.esql.type.AbstractEsFieldTypeTests;

import static org.elasticsearch.xpack.esql.core.expression.FieldAttributeTestUtils.newFieldAttributeWithType;

public class FieldAttributeTests extends AbstractAttributeTestCase<FieldAttribute> {
public static FieldAttribute createFieldAttribute(int maxDepth, boolean onlyRepresentable) {
Source source = Source.EMPTY;
Expand All @@ -26,7 +28,7 @@ public static FieldAttribute createFieldAttribute(int maxDepth, boolean onlyRepr
EsField field = AbstractEsFieldTypeTests.randomAnyEsField(maxDepth);
Nullability nullability = randomFrom(Nullability.values());
boolean synthetic = randomBoolean();
return new FieldAttribute(source, parent, name, type, field, nullability, new NameId(), synthetic);
return newFieldAttributeWithType(source, parent, name, type, field, nullability, new NameId(), synthetic);
}

@Override
Expand All @@ -51,6 +53,6 @@ protected FieldAttribute mutate(FieldAttribute instance) {
case 4 -> nullability = randomValueOtherThan(nullability, () -> randomFrom(Nullability.values()));
case 5 -> synthetic = false == synthetic;
}
return new FieldAttribute(source, parent, name, type, field, nullability, new NameId(), synthetic);
return newFieldAttributeWithType(source, parent, name, type, field, nullability, new NameId(), synthetic);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -70,6 +70,7 @@

import static org.elasticsearch.test.ListMatcher.matchesList;
import static org.elasticsearch.test.MapMatcher.assertMap;
import static org.elasticsearch.xpack.esql.core.expression.FieldAttributeTestUtils.newFieldAttributeWithType;
import static org.hamcrest.Matchers.equalTo;

public class PlanNamedTypesTests extends ESTestCase {
Expand Down Expand Up @@ -151,7 +152,7 @@ static FieldAttribute randomFieldAttributeOrNull() {
}

static FieldAttribute randomFieldAttribute() {
return new FieldAttribute(
return newFieldAttributeWithType(
Source.EMPTY,
randomFieldAttributeOrNull(), // parent
randomAlphaOfLength(randomIntBetween(1, 25)), // name
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1227,6 +1227,26 @@ public void testEnrichOnDefaultField() {
assertThat(fieldNames, equalTo(ALL_FIELDS));
}

public void testMetrics() {
Set<String> fieldNames = EsqlSession.fieldNames(parser.createStatement("""
METRICS k8s bytes=sum(rate(network.total_bytes_in)), sum(rate(network.total_cost)) BY cluster"""), Set.of());
assertThat(
fieldNames,
equalTo(
Set.of(
"@timestamp",
"@timestamp.*",
"network.total_bytes_in",
"network.total_bytes_in.*",
"network.total_cost",
"network.total_cost.*",
"cluster",
"cluster.*"
)
)
);
}

private void assertFieldNames(String query, Set<String> expected) {
Set<String> fieldNames = EsqlSession.fieldNames(parser.createStatement(query), Collections.emptySet());
assertThat(fieldNames, equalTo(expected));
Expand Down