ESQL load all dimensions when doing a Time Series query #132687
Conversation
not-napoleon
added
>enhancement
WIP
v9.2.0
:StorageEngine/ES|QL
…ension-filter' into fieldcaps-dimension-filter
…ension-filter' into fieldcaps-dimension-filter
| // Detect if we are in TS mode | ||
| List<LogicalPlan> relations = parsed.collect(UnresolvedRelation.class::isInstance); | ||
| for (LogicalPlan i : relations) { | ||
| if (((UnresolvedRelation) i).isTimeSeriesMode()) { |
There was a problem hiding this comment.
This cast should be completely safe, since the collect above should only have returned UnresolvedRelation instances.
There was a problem hiding this comment.
You can avoid casting with this:
boolean shouldCollectAllDimensions =
parsed.collectFirstChildren(c -> c instanceof UnresolvedRelation ur && ur.isTimeSeriesMode()).isEmpty() == false;
…ension-filter' into fieldcaps-dimension-filter
| final String field = entry.getKey(); | ||
| if (fieldNameFilter.test(field) == false) { | ||
| MappedFieldType ft = entry.getValue(); | ||
| if (fieldNameFilter.test(field) == false && ((ft.isDimension() && includeDimensions) == false)) { |
There was a problem hiding this comment.
Can you move includeDimensions before ft.isDimension() since it's much cheaper?
| l.onResponse(result.withIndexResolution(indexResolution)); | ||
| }) | ||
| }), | ||
| false |
There was a problem hiding this comment.
we need to use true for time-series mode here?
| // Detect if we are in TS mode | ||
| List<LogicalPlan> relations = parsed.collect(UnresolvedRelation.class::isInstance); | ||
| for (LogicalPlan i : relations) { | ||
| if (((UnresolvedRelation) i).isTimeSeriesMode()) { |
There was a problem hiding this comment.
You can avoid casting with this:
boolean shouldCollectAllDimensions =
parsed.collectFirstChildren(c -> c instanceof UnresolvedRelation ur && ur.isTimeSeriesMode()).isEmpty() == false;
| Set<String> wildcardJoinIndices, | ||
| InferenceResolution inferenceResolution | ||
| InferenceResolution inferenceResolution, | ||
| boolean collectAllDimensions |
There was a problem hiding this comment.
I think you meant to add this flag to PreAnalyzer.PreAnalysis instead? We already have an index mode in PreAnalyzer.PreAnalysis; shouldn't that be sufficient?
There was a problem hiding this comment.
yeah, upon review I don't think we need this at all. As you say, the index mode in the PreAnalyzer.PreAnalysis should be enough.
|
|
||
| private static boolean checkIncludeDimensions(String[] filters) { | ||
| for (String filter : filters) { | ||
| if ("+dimension".equals(filter)) { |
There was a problem hiding this comment.
Nit: +dimensions reads better to me? +dimension implies that there is only one.
There was a problem hiding this comment.
Although having said that, we already have +parent that can return multiple parents, so maybe it's fine.
There was a problem hiding this comment.
Yeah, I had been following the "convention" from +parent, but I don't feel strongly one way or the other.
IMHO, it should be an enum. I have a strong dislike of magic strings. I have a mind to refactor that on a slow Friday. |
…ension-filter' into fieldcaps-dimension-filter
3 participants
Fetch all dimensions when running a time series query
Note on CCS - this is adding a filter to field caps, which will not be available on older versions. This will cause the CCS field caps request to fail for those clusters. However, since the
TScommand this supports will not be released until after this is merged, that will only happen on clusters that weren't going to be able to run the query anyway. This seems acceptable to me, but worth mentioning.