SCRIPTING: Move Aggregation Script Context to its own class #33820
Conversation
original-brownbear
added
>non-issue
WIP
:Core/Infra/Scripting
|
Pinging @elastic/es-core-infra |
| } | ||
|
|
||
| /** Return the score of the current document. */ | ||
| public double getScore() { |
There was a problem hiding this comment.
I don't think we should have score accessible as score and _score?
There was a problem hiding this comment.
That's what I was going to ask you, but yea I think score can go away here right?
| * <p> | ||
| * @param value per-document value, typically a String, Long, or Double | ||
| */ | ||
| public void setNextAggregationValue(Object value) { |
There was a problem hiding this comment.
At minimum this should be typed (we should have separate script classes for each underlying doc values type), but I would much rather this be built into the script itself, so instead of setting the value, an iterate type call is made, similar to the doc values api.
There was a problem hiding this comment.
@rjernst as it turns out this isn't so trivial :(
Determining the exact type of the script upstream would require a lot of refactoring starting with org.elasticsearch.search.aggregations.support.ValuesSourceConfig (did some initial cleanups towards that in #34323).
Maybe we should do that in a next step?
| /** | ||
| * Return the result as a long. This is used by aggregation scripts over long fields. | ||
| */ | ||
| public long runAsLong() { |
There was a problem hiding this comment.
We shouldn't need these methods if we make typed versions of scripts, where execute would then return the correct type. Additionally, don't we always return double for all numeric fields (ie runAsLong isn't actually used)?
There was a problem hiding this comment.
It's used in: org.elasticsearch.search.aggregations.support.ValuesSource.Numeric.WithScript.LongValues.
but you're right typed versions of this context should be possible, will try tomorrow :)
|
|
||
| public AggregationScript(Map<String, Object> params, SearchLookup lookup, LeafReaderContext leafContext) { | ||
| this.params = new ParameterMap(new HashMap<>(params), DEPRECATIONS); | ||
| this.leafLookup = lookup.getLeafSearchLookup(leafContext); |
There was a problem hiding this comment.
I didn't think doc or _score works in value aggregation scripts? Looking at the user of this, I don't see where setDocument is called.
There was a problem hiding this comment.
I don't know about the score (though a scorer is set for this script in e.g org.elasticsearch.search.aggregations.support.values.ScriptBytesValues#setScorer) but the document is set in many places like org.elasticsearch.search.aggregations.support.values.ScriptLongValues#advanceExact
There was a problem hiding this comment.
Thanks, I see the calls to setDocument now. It still looks like the scorer is not set anywhere, but it also looks quite deceptive so I fear there is something more complex going on.
There was a problem hiding this comment.
I agree this at least helps us get rid of SearchScript uses, so let's get this in with the generic Object return values and such, and then work on refactoring to have both typed and value/non-value specific versions (I still think _value scripts should not provide _score or doc). LGTM.
|
@rjernst thanks, I'll still have to fix the expression case here but that should be pretty quick+easy. Will try to hurry that up :) |
|
@rjernst alright enabled expressions here too the same way I did it for the terms set script. See if you're still ok with it ? :) |
| this.params.putAll(leafLookup.asMap()); | ||
| } | ||
|
|
||
| protected AggregationScript() { |
There was a problem hiding this comment.
@rjernst like in the terms set case (same code there) this is used by expressions because they don't use the leaf lookup (just to avoid the null checks and logic in the constructor).
* master: Do not update number of replicas on no indices (elastic#34481) Core: Remove two methods from AbstractComponent (elastic#34336) Use RoleRetrievalResult for better caching (elastic#34197) Revert "Search: Fix spelling mistake in Javadoc (elastic#34480)" Search: Fix spelling mistake in Javadoc (elastic#34480) Docs: improve formatting of Query String Query doc page (elastic#34432) Tests: Handle epoch date formatters edge cases (elastic#34437) Test: Fix running with external cluster Fix handling of empty keyword in terms aggregation (elastic#34457) [DOCS] Fix tag in SecurityDocumentationIT [Monitoring] Add additional necessary mappings for apm-server (elastic#34392) SCRIPTING: Move Aggregation Script Context to its own class (elastic#33820) MINOR: Remove Deadcode in ExpressionTermSetQuery (elastic#34442) HLRC: Get SSL Certificates API (elastic#34135)
* SCRIPTING: Move Aggregation Script Context to its own class
WIP