Min score for time series #96878
Min score for time series #96878
Conversation
Enables min score on time series aggregation.
elasticsearchmachine added needs:triage 
| @martijnvg might need some more checkstyle cleanup, but should essentially be ready for review. |
tmgordeeva added Team:Analytics 
| Pinging @elastic/es-analytics-geo (Team:Analytics) |
| Scorer scorer = weight.scorer(leaf); | ||
| if (scorer != null) { | ||
| LeafWalker leafWalker = new LeafWalker(leaf, scorer, bucketCollector, () -> tsidOrd[0]); | ||
| LeafWalker leafWalker = new LeafWalker(leaf, scorer, minimumScore, bucketCollector, () -> tsidOrd[0]); |
There was a problem hiding this comment.
What about wrapping the scorer here with a MinScoreScorer wrapper?
The DocIdSetIterator it produces in the LeafWalker constructor only returns documents that have at least the minimum score.
This way LeafWalker class doesn't need to be changed and there shouldn't be any overhead for users not using the minimum score feature?
| this.timestampReverse = TIME_SERIES_SORT[1].getOrder() == SortOrder.DESC; | ||
| } | ||
| | ||
| public void setMinimumScore(float minimumScore) { |
There was a problem hiding this comment.
This does need to be invoked from AggregationPhase otherwise it minimum score functionality can't be used?
...rc/test/java/org/elasticsearch/search/aggregations/support/TimeSeriesIndexSearcherTests.java Show resolved Hide resolved
| @martijnvg should we ready to go now! I'll be around for a while on slack if there's any last minute changes we need. |
This looks almost ready. Can you address this comment: https://github.com/elastic/elasticsearch/pull/96878/files#r1234034640 I think we just need to wrap the scorer before we create the |
| dir.close(); | ||
| } | ||
| | ||
| static class StaticScoreFunctionBuilder extends ScoreFunctionBuilder<StaticScoreFunctionBuilder> { |
There was a problem hiding this comment.
And this inner class can be removed? Because it is no longer used?
| Hi @tmgordeeva, I've created a changelog YAML for you. |
| private final SortedNumericDocValues timestamps; // TODO can we have this just a NumericDocValues? | ||
| private final BytesRefBuilder scratch = new BytesRefBuilder(); | ||
| | ||
| private final Scorer scorer; |
There was a problem hiding this comment.
I think this field is no longer used and can be removed?
There was a problem hiding this comment.
Oops, I missed this comment - I'll make a tiny cleanup PR later.
3 participants
Enables min score on time series aggregation.