-
Notifications
You must be signed in to change notification settings - Fork 25.6k
ESQL load all dimensions when doing a Time Series query #132687
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
Changes from 17 commits
e2c2a7c
4a38b0b
33f975c
fb35040
980a99a
0c11a2a
5ff350c
d65202f
74fa6a8
a5c830b
d0dcf7c
f041e71
5292071
0c9014e
083594d
794b96c
a157a64
dc6c618
ada3b48
9e2946d
4bca905
da094a4
2bcc34c
3b5037f
1616c05
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -161,6 +161,7 @@ static Map<String, IndexFieldCapabilities> retrieveFieldCaps( | |
| boolean includeEmptyFields | ||
| ) { | ||
| boolean includeParentObjects = checkIncludeParents(filters); | ||
| boolean includeDimensions = checkIncludeDimensions(filters); | ||
|
|
||
| Predicate<MappedFieldType> filter = buildFilter(filters, types, context); | ||
| boolean isTimeSeriesIndex = context.getIndexSettings().getTimestampBounds() != null; | ||
|
|
@@ -169,10 +170,10 @@ static Map<String, IndexFieldCapabilities> retrieveFieldCaps( | |
| Map<String, IndexFieldCapabilities> responseMap = new HashMap<>(); | ||
| for (Map.Entry<String, MappedFieldType> entry : context.getAllFields()) { | ||
| final String field = entry.getKey(); | ||
| if (fieldNameFilter.test(field) == false) { | ||
| MappedFieldType ft = entry.getValue(); | ||
| if (fieldNameFilter.test(field) == false && ((ft.isDimension() && includeDimensions) == false)) { | ||
| continue; | ||
| } | ||
| MappedFieldType ft = entry.getValue(); | ||
| if ((includeEmptyFields || ft.fieldHasValue(fieldInfos)) | ||
| && (fieldPredicate.test(ft.name()) || context.isMetadataField(ft.name())) | ||
| && (filter == null || filter.test(ft))) { | ||
|
|
@@ -234,6 +235,15 @@ private static boolean checkIncludeParents(String[] filters) { | |
| return true; | ||
| } | ||
|
|
||
| private static boolean checkIncludeDimensions(String[] filters) { | ||
| for (String filter : filters) { | ||
| if ("+dimension".equals(filter)) { | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nit: There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Although having said that, we already have There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, I had been following the "convention" from |
||
| return true; | ||
| } | ||
| } | ||
| return false; | ||
| } | ||
|
|
||
| private static boolean canMatchShard( | ||
| ShardId shardId, | ||
| QueryBuilder indexFilter, | ||
|
|
@@ -267,7 +277,8 @@ private static Predicate<MappedFieldType> buildFilter(String[] filters, String[] | |
| } | ||
|
|
||
| for (String filter : filters) { | ||
| if ("parent".equals(filter) || "-parent".equals(filter)) { | ||
| // These "filters" are handled differently, in that they are not ANDed with the field name pattern | ||
| if ("parent".equals(filter) || "-parent".equals(filter) || "+dimension".equals(filter)) { | ||
| continue; | ||
| } | ||
| Predicate<MappedFieldType> next = switch (filter) { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -423,7 +423,8 @@ private void preAnalyzeLookupIndex( | |
| patternWithRemotes, | ||
| fieldNames, | ||
| null, | ||
| listener.map(indexResolution -> receiveLookupIndexResolution(result, localPattern, executionInfo, indexResolution)) | ||
| listener.map(indexResolution -> receiveLookupIndexResolution(result, localPattern, executionInfo, indexResolution)), | ||
| false | ||
| ); | ||
| } | ||
|
|
||
|
|
@@ -641,7 +642,8 @@ private void preAnalyzeMainIndices( | |
| requestFilter, | ||
| listener.delegateFailure((l, indexResolution) -> { | ||
| l.onResponse(result.withIndexResolution(indexResolution)); | ||
| }) | ||
| }), | ||
| false | ||
|
||
| ); | ||
| } | ||
| } else { | ||
|
|
@@ -782,11 +784,17 @@ public record PreAnalysisResult( | |
| EnrichResolution enrichResolution, | ||
| Set<String> fieldNames, | ||
| Set<String> wildcardJoinIndices, | ||
| InferenceResolution inferenceResolution | ||
| InferenceResolution inferenceResolution, | ||
| boolean collectAllDimensions | ||
|
||
| ) { | ||
|
|
||
| public PreAnalysisResult(EnrichResolution enrichResolution, Set<String> fieldNames, Set<String> wildcardJoinIndices) { | ||
| this(null, new HashMap<>(), enrichResolution, fieldNames, wildcardJoinIndices, InferenceResolution.EMPTY); | ||
| public PreAnalysisResult( | ||
| EnrichResolution enrichResolution, | ||
| Set<String> fieldNames, | ||
| Set<String> wildcardJoinIndices, | ||
| boolean collectAllDimensions | ||
| ) { | ||
| this(null, new HashMap<>(), enrichResolution, fieldNames, wildcardJoinIndices, InferenceResolution.EMPTY, collectAllDimensions); | ||
| } | ||
|
|
||
| PreAnalysisResult withInferenceResolution(InferenceResolution newInferenceResolution) { | ||
|
|
@@ -796,7 +804,8 @@ PreAnalysisResult withInferenceResolution(InferenceResolution newInferenceResolu | |
| enrichResolution(), | ||
| fieldNames(), | ||
| wildcardJoinIndices(), | ||
| newInferenceResolution | ||
| newInferenceResolution, | ||
| collectAllDimensions() | ||
| ); | ||
| } | ||
|
|
||
|
|
@@ -807,7 +816,8 @@ PreAnalysisResult withIndexResolution(IndexResolution newIndexResolution) { | |
| enrichResolution(), | ||
| fieldNames(), | ||
| wildcardJoinIndices(), | ||
| inferenceResolution() | ||
| inferenceResolution(), | ||
| collectAllDimensions() | ||
| ); | ||
| } | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -8,7 +8,6 @@ | |
| package org.elasticsearch.xpack.esql.session; | ||
|
|
||
| import org.elasticsearch.common.regex.Regex; | ||
| import org.elasticsearch.index.IndexMode; | ||
| import org.elasticsearch.xpack.esql.analysis.EnrichResolution; | ||
| import org.elasticsearch.xpack.esql.core.expression.Alias; | ||
| import org.elasticsearch.xpack.esql.core.expression.Attribute; | ||
|
|
@@ -59,22 +58,31 @@ public class FieldNameUtils { | |
| public static PreAnalysisResult resolveFieldNames(LogicalPlan parsed, EnrichResolution enrichResolution) { | ||
|
|
||
| // we need the match_fields names from enrich policies and THEN, with an updated list of fields, we call field_caps API | ||
| var enrichPolicyMatchFields = enrichResolution.resolvedEnrichPolicies() | ||
| Set<String> enrichPolicyMatchFields = enrichResolution.resolvedEnrichPolicies() | ||
| .stream() | ||
| .map(ResolvedEnrichPolicy::matchField) | ||
| .collect(Collectors.toSet()); | ||
|
|
||
| // get the field names from the parsed plan combined with the ENRICH match fields from the ENRICH policy | ||
| List<LogicalPlan> inlinestats = parsed.collect(InlineStats.class::isInstance); | ||
| Set<Aggregate> inlinestatsAggs = new HashSet<>(); | ||
| for (var i : inlinestats) { | ||
| for (LogicalPlan i : inlinestats) { | ||
| inlinestatsAggs.add(((InlineStats) i).aggregate()); | ||
| } | ||
|
|
||
| boolean shouldCollectAllDimensions = false; | ||
| // Detect if we are in TS mode | ||
| List<LogicalPlan> relations = parsed.collect(UnresolvedRelation.class::isInstance); | ||
| for (LogicalPlan i : relations) { | ||
| if (((UnresolvedRelation) i).isTimeSeriesMode()) { | ||
|
||
| shouldCollectAllDimensions = true; | ||
| } | ||
| } | ||
|
|
||
| if (false == parsed.anyMatch(p -> shouldCollectReferencedFields(p, inlinestatsAggs))) { | ||
| // no explicit columns selection, for example "from employees" | ||
| // also, inlinestats only adds columns to the existent output, its Aggregate shouldn't interfere with potentially using "*" | ||
| return new PreAnalysisResult(enrichResolution, IndexResolver.ALL_FIELDS, Set.of()); | ||
| return new PreAnalysisResult(enrichResolution, IndexResolver.ALL_FIELDS, Set.of(), shouldCollectAllDimensions); | ||
| } | ||
|
|
||
| Holder<Boolean> projectAll = new Holder<>(false); | ||
|
|
@@ -86,7 +94,7 @@ public static PreAnalysisResult resolveFieldNames(LogicalPlan parsed, EnrichReso | |
| }); | ||
|
|
||
| if (projectAll.get()) { | ||
| return new PreAnalysisResult(enrichResolution, IndexResolver.ALL_FIELDS, Set.of()); | ||
| return new PreAnalysisResult(enrichResolution, IndexResolver.ALL_FIELDS, Set.of(), shouldCollectAllDimensions); | ||
| } | ||
|
|
||
| var referencesBuilder = new Holder<>(AttributeSet.builder()); | ||
|
|
@@ -162,7 +170,7 @@ public static PreAnalysisResult resolveFieldNames(LogicalPlan parsed, EnrichReso | |
| } | ||
| } else { | ||
| referencesBuilder.get().addAll(p.references()); | ||
| if (p instanceof UnresolvedRelation ur && ur.indexMode() == IndexMode.TIME_SERIES) { | ||
| if (p instanceof UnresolvedRelation ur && ur.isTimeSeriesMode()) { | ||
| // METRICS aggs generally rely on @timestamp without the user having to mention it. | ||
| referencesBuilder.get().add(new UnresolvedAttribute(ur.source(), MetadataAttribute.TIMESTAMP_FIELD)); | ||
| } | ||
|
|
@@ -221,7 +229,7 @@ public static PreAnalysisResult resolveFieldNames(LogicalPlan parsed, EnrichReso | |
| parsed.forEachDownMayReturnEarly(forEachDownProcessor.get()); | ||
|
|
||
| if (projectAll.get()) { | ||
| return new PreAnalysisResult(enrichResolution, IndexResolver.ALL_FIELDS, Set.of()); | ||
| return new PreAnalysisResult(enrichResolution, IndexResolver.ALL_FIELDS, Set.of(), shouldCollectAllDimensions); | ||
| } | ||
|
|
||
| // Add JOIN ON column references afterward to avoid Alias removal | ||
|
|
@@ -235,12 +243,17 @@ public static PreAnalysisResult resolveFieldNames(LogicalPlan parsed, EnrichReso | |
|
|
||
| if (fieldNames.isEmpty() && enrichPolicyMatchFields.isEmpty()) { | ||
| // there cannot be an empty list of fields, we'll ask the simplest and lightest one instead: _index | ||
| return new PreAnalysisResult(enrichResolution, IndexResolver.INDEX_METADATA_FIELD, wildcardJoinIndices); | ||
| return new PreAnalysisResult( | ||
| enrichResolution, | ||
| IndexResolver.INDEX_METADATA_FIELD, | ||
| wildcardJoinIndices, | ||
| shouldCollectAllDimensions | ||
| ); | ||
| } else { | ||
| fieldNames.addAll(subfields(fieldNames)); | ||
| fieldNames.addAll(enrichPolicyMatchFields); | ||
| fieldNames.addAll(subfields(enrichPolicyMatchFields)); | ||
| return new PreAnalysisResult(enrichResolution, fieldNames, wildcardJoinIndices); | ||
| return new PreAnalysisResult(enrichResolution, fieldNames, wildcardJoinIndices, shouldCollectAllDimensions); | ||
| } | ||
| } | ||
|
|
||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you move
includeDimensionsbeforeft.isDimension()since it's much cheaper?