-
Notifications
You must be signed in to change notification settings - Fork 24.3k
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
Aggregation Execution Context add timestamp provider #85850
Aggregation Execution Context add timestamp provider #85850
Conversation
Pinging @elastic/es-analytics-geo (Team:Analytics) |
@elasticmachine ok to test |
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.
LGTM!
This is a good improvement, that will definitely be used by the downsampling operation, which loads timestamps. I expect this to also improve performance.
@elasticmachine update branch |
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.
LGTM, except I don't think we need IOException there. I suspect we can convert CheckedSupplier to Supplier in case of tsid as well. I think it is CheckedSupplier for purely historical reasons.
public AggregationExecutionContext( | ||
LeafReaderContext leafReaderContext, | ||
CheckedSupplier<BytesRef, IOException> tsidProvider, | ||
CheckedSupplier<Long, IOException> timestampProvider |
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.
Why is this a checked supplier? Under what condition can it ever throw IOException?
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.
LGTM, except I don't think we need IOException there. I suspect we can convert CheckedSupplier to Supplier in case of tsid as well. I think it is CheckedSupplier for purely historical reasons.
Yeah, I also think Supplier is better, I have converted CheckedSupplier to Supplier for timestamp and tsid.
@elasticmachine update branch |
Thank you for this contribution @weizijun |
* master: (104 commits) fix: ordering terms aggregation on top metrics null values (elastic#85774) Fix up whitespace error introduced in elastic#85948 More docs re. removing cluster.initial_master_nodes (elastic#85948) [Test] Remove API key methods from HLRC (elastic#85802) Remove references to bootstrap.system_call_filter (elastic#85964) Move docker cgroup override to SystemJvmOptions (elastic#85960) Add connection accounting tests (elastic#85966) Remove MacOS from platform support testing matrix Remove custom KnnVectorFieldExistsQuery (elastic#85945) Relax data path deprecations from critical to warn (elastic#85952) Remove hppc from some "common" classes (elastic#85957) Move docker env var settings handling out of bash (elastic#85913) Remove hppc from task manager (elastic#85889) [ML] rename trained model allocations to assignments (elastic#85503) Remove hppc from multi*shard request and responses (elastic#85888) Consolidating logging initialization in cli launcher (elastic#85920) Convert license tools to use unified cli entrypoint (elastic#85919) Add noop detection to node shutdown actions (elastic#85914) Adjust SQL expended test output TSDB: Add timestamp provider to AggregationExecutionContext (elastic#85850) ... # Conflicts: # server/src/main/java/org/elasticsearch/search/aggregations/AggregationExecutionContext.java
As LeafWalker already get the timestamp from doc values, It can add a timestampProvider in the AggregationExecutionContext.
It's useful in the time serise downsampling sense. downsampling collect can get the timestamp from the AggregationExecutionContext.
And It will have other more use case.