Validate user-supplied missing values on unmapped fields #43718
Conversation
|
Pinging @elastic/es-analytics-geo |
There was a problem hiding this comment.
Core changes look good, left a few minor comments/questions.
The only noticeable change should be that if you pass something exceedingly unusual as a missing value for an unmapped field to the terms agg, it will no longer silently convert it to a string and continue. Instead, it will throw an AggregationExecutionException same as if you'd had that value in a mapped field.
Do we have a good feeling for how "breaking" this is? E.g. is there a case where someone was using this behavior (accidentally or otherwise) and now we'll throw an exception? I'm not really sure what you could pass in the REST API that would be non-string/non-numeric... an object I guess? Or is it only an issue for java users that can send in objects which have a valid toString()?
...c/main/java/org/elasticsearch/search/aggregations/support/ValuesSourceAggregatorFactory.java
Show resolved
Hide resolved
...st/java/org/elasticsearch/search/aggregations/bucket/histogram/HistogramAggregatorTests.java
Outdated
Show resolved
Hide resolved
.../src/test/java/org/elasticsearch/search/aggregations/metrics/CardinalityAggregatorTests.java
Outdated
Show resolved
Hide resolved
.../src/test/java/org/elasticsearch/search/aggregations/metrics/CardinalityAggregatorTests.java
Outdated
Show resolved
Hide resolved
|
@polyfractal Ok, as per conversation, I updated the PR so there's zero behavior change, and this just adds hooks and tests. |
|
👍 when CI is happy it LGTM! |
|
@elasticmachine update branch |
…3718) (#43940) Provides a hook for aggregations to introspect the `ValuesSourceType` for a user supplied Missing value on an unmapped field, when the type would otherwise be `ANY`. Mapped field behavior is unchanged, and still applies the `ValuesSourceType` of the field. This PR just provides the hook for doing this, no existing aggregations have their behavior changed.
This PR adds the ability for Aggregation factories to have finer grained control over how missing values are mapped to
ValuesSources. Technically speaking, this addresses a minor bug in the terms aggregation, but more importantly it resolves a design constraint for Range field aggregations. I opened the PR against master however because it doesn't depend on any of the range field aggregation work and touches shared infrastructure for aggregations, so getting it in master sooner will avoid merge conflicts down the road.I've added a bunch of tests, and done my best to preserve the existing behavior wherever possible. The only noticeable change should be that if you pass something exceedingly unusual as a missing value for an unmapped field to the terms agg, it will no longer silently convert it to a string and continue. Instead, it will throw an
AggregationExecutionExceptionsame as if you'd had that value in a mapped field.