Repository navigation
fix(api): reject the percentile aggregations on a non-numeric field - #2989
Open
amartya1523 wants to merge 2 commits into
Open
amartya1523 wants to merge 2 commits into
amartya1523 wants to merge 2 commits into
Conversation
ES registers percentiles, percentile_ranks and median_absolute_deviation
for numeric values only (plus date and boolean for the percentiles), so
one on a keyword or ip field fails every shard with "Field [f] of type
[t] is not supported for aggregation [percentiles]" and the search is a
400. XERJ answered 200 with every percentile null, which reads as an
empty index rather than the wrong field.
The check that already does this for avg/sum/min/max/stats/
extended_stats now covers the three, with the same rules: only types ES
never accepts, and only when the field has that type in every target.
Retires the two percentiles_{hdr,tdigest}_metric.yml Invalid params
entries; every other step of both cases already answered 400.
Replays every step of both Invalid params cases; keyword and ip fields are a 400 with ES's reason for percentiles, percentile_ranks and median_absolute_deviation. Numeric, date and mixed-index targets still answer.
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
AI-assisted change, run and reviewed by @amartya1523.
What this changes, and why
percentileson a keyword field answered 200 with every percentilenull:That reads as "this index has no values", not "this is the wrong field". ES registers the percentile aggregations for numeric values only:
percentilesandpercentile_ranksalso take date and boolean,median_absolute_deviationis numeric only. So on a keyword or ip field,ValuesSourceRegistry#getAggregatorfails every shard and the search is a 400:Two
catch-lenient.txtlines are this case:aggregations/percentiles_hdr_metric.yml · Invalid params testaggregations/percentiles_tdigest_metric.yml · Invalid params testEach YAML case has several steps (bad
number_of_significant_value_digits, negativecompression, empty/null/non-numericpercents, and a keyword field). The new test replays every step against main: all the others already answer 400; only the keyword-field step, the last one in each case, failed.Fix (es_compat.rs):
find_unsupported_metric_agg_fieldalready does this foravg,sum,min,max,statsandextended_stats(#2913). It now coverspercentiles,percentile_ranksandmedian_absolute_deviationtoo, with the same rules:search_shard_validation_errorenvelopeNo new function: the change is the list of aggregation names and the doc comment.
Evidence
New test module
percentile_family_field_type_tests(es_compat.rs), run against main first:First test: all twelve steps of the two YAML cases return 400. Then
percentileson keyword and ip,percentile_rankson keyword, andmedian_absolute_deviationon keyword each assert ES's exact reason.Second test:
percentileson an integer (hdr) and on a datepercentile_rankson a doublemedian_absolute_deviationon an integerpercentilesover/test_1,other, whereothermaps the field as alongAll still answer 200.
Checks
cargo fmt -p xerj-api -- --checkcargo test --profile quick -p xerj-api --lib: 432 passed, 0 failed, including the existingmetric_agg_field_type_testsInvalid params testcases now enforce it; their catch-lenient lines are deleted. The other percentile cases use numeric fields.Not run locally:
--releasebuilds,clippyand the integration binaries. Disk on this machine is at 99%; CI runs all of these.Provenance
quickprofile, based on main83c9f0d12.PercentilesAggregationBuilder,PercentileRanksAggregationBuilderandMedianAbsoluteDeviationAggregationBuilder.boxplot(x-pack analytics) is numeric-only in ES too, but it has no YAML case here, so I left it out of this PR. I can add it if you want.