Move getPointReaderOrNull into AggregatorBase #58769
Conversation
not-napoleon
added
>non-issue
:Analytics/Aggregations
|
Pinging @elastic/es-analytics-geo (:Analytics/Aggregations) |
elasticmachine
added
the
Team:Analytics
| * | ||
| * @param config The config for the values source metric. | ||
| */ | ||
| public Function<byte[], Number> getPointReaderOrNull(ValuesSourceConfig config) { |
There was a problem hiding this comment.
Do you think now'd be a good time to change the name? I always thought something like pointReaderIfSafe or something is a little more clear about why you might get null back.
And it isn't so much "if early termination is applicable" as "if there are no filters and the underlying index is in the same order as the values produced by the config"
There was a problem hiding this comment.
Could you make the method final?
There was a problem hiding this comment.
the underlying index is in the same order as the values produced by the config
Out of curiosity, what in the current logic looks at index ordering?
server/src/main/java/org/elasticsearch/search/aggregations/support/ValuesSourceConfig.java
Show resolved
Hide resolved
| * @param fieldType The field type we want to get a reader for | ||
| * @return null if we can't get a reader (e.g. because the field is the wrong type), otherwise a point reader function. | ||
| */ | ||
| default Function<byte[], Number> getPointReader(MappedFieldType fieldType) { return null; } |
There was a problem hiding this comment.
I'm not convinced this is the right place to add extensibility here. The other option I'm considering, we could make parsePoint a method on MappedFieldType (or, maybe a method that returns a reference to parsePoint or null, depending on if it's implemented for that field). That would let us get rid of the instance type checking, and provide a way for plugins that add fields which use CoreVSTs to add custom point readers.
There was a problem hiding this comment.
I agree that it should be the MappedFieldType's responsibility. It is built by the FieldMapper which is ultimately the only thing that actually knows how the index was built.
|
@elasticmachine update branch |
|
@not-napoleon I edited your description to say that this closes #58769 which I believe it does, in that it creates the abstraction that other aggs can use to read the points. It doesn't link them all in. At least, it didn't when I last looked, but I think that is ok. |
Conflicts: server/src/main/java/org/elasticsearch/search/aggregations/support/ValuesSourceConfig.java
Conflicts: server/src/main/java/org/elasticsearch/search/aggregations/support/ValuesSourceConfig.java server/src/test/java/org/elasticsearch/search/aggregations/metrics/MinAggregatorTests.java
This is a pretty straight forward refactoring to move the point reader optimization that min and max use up to the aggregator base, and move the values source config related logic into values source config. Hopefully that makes it easier to reuse, and also puts the check for if it can be applied in a more visible place.
Closes #55552