-
Notifications
You must be signed in to change notification settings - Fork 25.7k
Use query source to detect non-null docs #133287
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
Conversation
ed87ed5 to
882b679
Compare
882b679 to
1a5aecb
Compare
dnhatn
left a comment
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.
To the reviewers: except for the comments, most of the changes add a new parameter to the column reader.
| /** | ||
| * Returns the set of fields that are guaranteed to be dense after the source query. | ||
| */ | ||
| static Set<String> nullsFilteredFieldsAfterSourceQuery(QueryBuilder sourceQuery) { |
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.
Here, we extract fields from the query that can be considered nullsFiltered when reading values. The nullsFiltered flag passed to each FieldInfo.
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.
Is this done only for TS or for FROM too? For the latter, I wonder if ignoring nulls is desired.
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.
It should work for both, but it will be a no-op. Optimizations can be enabled at the codec level, and only possible with the TSDB codec now.
| ); | ||
| } | ||
| if (fields[field].info.nullsFiltered() && block.mayHaveNulls()) { | ||
| assert IntStream.range(0, block.getPositionCount()).noneMatch(block::isNull) |
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.
This can be expensive, so it is only enabled with assertions.
| } | ||
| for (ColumnAtATimeWork r : columnAtATimeReaders) { | ||
| target[r.idx] = (Block) r.reader.read(loaderBlockFactory, docs, offset); | ||
| target[r.idx] = (Block) r.reader.read(loaderBlockFactory, docs, offset, operator.fields[r.idx].info.nullsFiltered()); |
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.
We pass the nullsFiltered from FieldInfo to column reader.
|
Pinging @elastic/es-analytical-engine (Team:Analytics) |
|
Pinging @elastic/es-storage-engine (Team:StorageEngine) |
| case TermsQueryBuilder q -> Set.of(q.fieldName()); | ||
| case RangeQueryBuilder q -> Set.of(q.fieldName()); | ||
| case ConstantScoreQueryBuilder q -> nullsFilteredFieldsAfterSourceQuery(q.innerQuery()); | ||
| // TODO: support SingleValueQuery |
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.
Should we also find and ignore coalence? Or is this part of default?
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.
Looks good!
One note: what happens if we have multiple field metrics? For instance:
TS metrics
| WHERE cpu_usage IS NOT NULL OR memory_usage IS NOT NULL
| STATS max(avg_over_time(cpu_usage)), max(avg_over_time(memory_usage)) BY tbucket(1 hour)
I'd think we don't necessarily get dense values here? If so, is this covered?
We won't be able to infer |
|
Thanks Kostas! |

One of the slowest parts of time-series queries is reading metric values - it accounts for 30% of the profiler time for the following query:
This is because the
metrics.system.memory.utilizationfield is sparse, requiring iteration over its DISI to find value indices when reading values. This change adds a flag namednullsFilteredto the column reader, signaling that all target docs have values for the field. This enables optimizations such as skipping value index lookups with DISI and performing bulk copying.We can safely do this because if the filter
WHERE metrics.system.memory.utilization IS NOT NULLis pushed down to Lucene, then every document returned from the Lucene operator will have a value for themetrics.system.memory.utilizationfield.I was able to make changes that reduce the execution time for reading sparse metric values to be comparable with reading dense fields (like timestamp). To keep this PR small, I will open the codec-related changes in a separate PR.