Extract Empty/Script/Missing ValuesSource behavior to an interface #48320
Conversation
|
Pinging @elastic/es-analytics-geo (:Analytics/Aggregations) |
not-napoleon
changed the base branch from
feature/extensible-values-source
to
master
server/src/main/java/org/elasticsearch/search/aggregations/support/BuiltinValuesSourceType.java
Outdated
Show resolved
Hide resolved
| @Override | ||
| public ValuesSource getEmpty() { | ||
| // TODO: Implement this or get rid of ANY | ||
| throw new UnsupportedOperationException("ValuesSourceType.ANY is still a special case"); |
There was a problem hiding this comment.
Should be BuiltinValuesSourceType.ANY right?
Not sure about the error message, but don't have a better suggestion at the moment :/
There was a problem hiding this comment.
Yeah, this was my main hesitation on merging this to master. ANY is still handled in the "old style" with special cases scattered through ValuesSourceConfig. I'm pretty sure we'll never hit this error message, but given the complexity it's hard to be absolutely sure.
server/src/main/java/org/elasticsearch/search/aggregations/support/BuiltinValuesSourceType.java
Outdated
Show resolved
Hide resolved
server/src/main/java/org/elasticsearch/search/aggregations/support/BuiltinValuesSourceType.java
Outdated
Show resolved
Hide resolved
| public ValuesSource getField(FieldContext fieldContext, AggregationScript.LeafFactory script) { | ||
| final IndexFieldData<?> indexFieldData = fieldContext.indexFieldData(); | ||
| ValuesSource dataSource; | ||
| if (indexFieldData instanceof IndexOrdinalsFieldData) { |
There was a problem hiding this comment.
Not important right now, but I wonder if we should try to encode some of these attributes with properties instead, in a future iteration? hasOrdinals(), isNumeric(), isRange() (or hasHistogram() in reference to Ignacio's PR) etc, that way we can avoid doing instanceof checks?
Might not be worth it, just a thought since instanceof's always sorta make me uneasy :) And it might make things more pluggable in the future since we won't have hardcoded classes in Core, just attributes
There was a problem hiding this comment.
I'm +1 on this, but it seems out of scope for this PR. I can open an issue for it.
There was a problem hiding this comment.
#48864 opened to track this
|
|
||
| @Override | ||
| public ValuesSource getScript(AggregationScript.LeafFactory script, ValueType scriptValueType) { | ||
| throw new AggregationExecutionException("value source of type [" + this.value() + "] is not supported by scripts"); |
There was a problem hiding this comment.
I wonder if we should make this exception the same as the other getFoo() methods, whatever we decide to keep it consistent?
There was a problem hiding this comment.
See comment above. Changing exception types in this PR is too many moving parts IMHO.
server/src/main/java/org/elasticsearch/search/aggregations/support/ValuesSource.java
Show resolved
Hide resolved
server/src/main/java/org/elasticsearch/search/aggregations/support/ValuesSourceType.java
Show resolved
Hide resolved
server/src/main/java/org/elasticsearch/search/aggregations/support/ValuesSourceType.java
Outdated
Show resolved
Hide resolved
server/src/main/java/org/elasticsearch/search/aggregations/support/BuiltinValuesSourceType.java
Outdated
Show resolved
Hide resolved
| @@ -307,7 +367,7 @@ public boolean needsScores() { | |||
|
|
|||
| @Override | |||
| public SortedBinaryDocValues bytesValues(LeafReaderContext context) throws IOException { | |||
| return new ValuesSource.WithScript.BytesValues(delegate.bytesValues(context), script.newInstance(context)); | |||
| return new Bytes.WithScript.BytesValues(delegate.bytesValues(context), script.newInstance(context)); | |||
There was a problem hiding this comment.
@polyfractal As per above comment, this is one of the rough edges. We're in a Numeric subclass, but we have this bytesValues method which calls into a different subclass now. I'm just guessing, but I suspect this is why WithScript wasn't a Bytes subclass originally.
|
@elasticmachine run elasticsearch-ci/2 |
|
@elasticmachine run elasticsearch-ci/packaging-sample-matrix |
…nterface (elastic#48320) This is a pure code rearrangement refactor. Logic for what specific ValuesSource instance to use for a given type (e.g. script or field) moved out of ValuesSourceConfig and into CoreValuesSourceType (previously just ValueSourceType; we extract an interface for future extensibility). ValueSourceConfig still selects which case to use, and then the ValuesSourceType instance knows how to construct the ValuesSource for that case. Conflicts: server/src/main/java/org/elasticsearch/search/aggregations/bucket/histogram/DateHistogramAggregationBuilder.java server/src/main/java/org/elasticsearch/search/aggregations/bucket/histogram/HistogramAggregationBuilder.java x-pack/plugin/analytics/src/test/java/org/elasticsearch/xpack/analytics/cumulativecardinality/CumulativeCardinalityAggregatorTests.java
…nterface (#48320) (#49330) This is a pure code rearrangement refactor. Logic for what specific ValuesSource instance to use for a given type (e.g. script or field) moved out of ValuesSourceConfig and into CoreValuesSourceType (previously just ValueSourceType; we extract an interface for future extensibility). ValueSourceConfig still selects which case to use, and then the ValuesSourceType instance knows how to construct the ValuesSource for that case.
As part of the extensible values source work, this PR extracts some of the hard-coded ValuesSource types from ValuesSourceConfig.
Related to #42949