Repository navigation
fix(api): reject avg/sum/min/max/stats on a non-numeric field like ES - #2913
Merged
Merged
Conversation
Motivation:
`{"aggs": {"a": {"avg": {"field": "string_field"}}}}` on a keyword field
answered 200 with `"value": null` (`sum` answered `-0.0`), so a metric
pointed at the wrong field looked like an empty index. ES 8.13 fails
every shard with `Field [string_field] of type [keyword] is not
supported for aggregation [avg]` (ValuesSourceRegistry#getAggregator)
and answers 400. Four catch-lenient lines are this case
(`{avg,max,min,sum}_metric.yml · Aggregating wrong datatype test`).
Root cause:
`find_invalid_agg_params` (xerj-org#1861) deliberately made only mapping-free
checks; nothing on the search path compared a metric's `field` with
the mapped type.
Fix:
`find_unsupported_metric_agg_field`, called in `search_impl` after the
field-alias rewrite, walks the aggregation tree (sub-aggregations
included) and resolves each `avg`/`sum`/`min`/`max`/`stats`/
`extended_stats` field through the participating indices' mappings,
following object `properties` and multi-field `fields` (`name.raw`).
It refuses only types ES never accepts for these aggregations (keyword,
constant_keyword, wildcard, ip, geo_point, version), and only when
every target index maps the field that way: an unmapped field, or one
that is numeric in at least one target, still answers 200 as ES does
(a partial-shard success there). The error uses the existing
`search_shard_validation_error` envelope (search_phase_execution_exception
wrapping illegal_argument_exception).
Evidence:
- ES-YAML suite against a node built from this change (with its two
sibling scroll fixes): `1453 passed · 0 failed · 10 skipped`, no stale
catch-lenient entries; the four lines above are deleted.
Runs through the ES router: avg on a keyword, max on an ip, sum on a
keyword multi-field, stats through a field alias and min under a terms
bucket each answer 400 with ES's reason; avg on a numeric field, on an
unmapped field, across a keyword/long pair of indices, and value_count
on a keyword still answer 200.
On main the first test fails with the 200 it used to return:
{"a":{"avg":{"field":"s"}}} -> {..."aggregations":{"a":{"value":null}}}
left: 200 right: 400
This was referenced Nov 3, 2026
Owner
|
Xerj Merger made the fix: rejects avg/sum/min/max/stats/extended_stats on a non-numeric mapped field with ES's 400 error instead of answering 200 with null. Diff reviewed, Build + Test green on fc8a3c6 (cargo-audit was red only from RUSTSEC-2026-0436, fixed on main by #2912). Thanks @amartya1523! |
4 tasks done
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
A numeric metric aggregation on a non-numeric field answered 200 with a null value. ES 8.13 fails every shard with
Field [f] of type [t] is not supported for aggregation [avg](ValuesSourceRegistry#getAggregator) and answers 400. A typo'd or wrong field therefore looked like an empty index, andsumon a keyword even returned-0.0.Four
catch-lenient.txtlines are this case:{avg,max,min,sum}_metric.yml · Aggregating wrong datatype test. #1861 left it out on purpose because the check needs the mapping ("avgon a keyword field ... is left alone").Fix:
find_unsupported_metric_agg_fieldruns insearch_implafter the field-alias rewrite, so an alias is judged by its target.avg/sum/min/max/stats/extended_stats, it resolvesfieldin each participating index's mapping, following objectpropertiesand multi-fieldfields(name.raw).search_shard_validation_errorenvelope:search_phase_execution_exceptionwrappingillegal_argument_exception.Evidence
Wire, node built from main
b78004832vs this branch (indext:skeyword,ipip,name.rawkeyword multi-field,ninteger; indexu:slong):New test module
metric_agg_field_type_tests(es_compat.rs), run against main first:Checks
cargo fmt --allquickprofile (cargo build --profile quick -p xerj-server, per docs/FAST_BUILDS.md). Not--release; see Not run.cargo test --profile quick -p xerj-api --lib: 417 passed, 0 failed (all three sibling fixes applied)1453 passed · 0 failed · 10 skipped · 1463 total, 65 cases catch-lenient (was 74), no stale-entry notes. Run withes-yaml-runner --dir tests/es-compat-yaml/yamlagainst aquicknode built from mainb78004832plus this PR and its two siblings.Aggregating wrong datatype testcases now enforce it; their catch-lenient lines are deleted.quick): the 31xerj-api/testsfiles that use aggregations or scroll. 30 passed.cluster_metadata_replicatesfailed a different replica-timing test on each of two runs (a_restarted_replica_…thena_late_member_copies_…, a 503replica_not_acknowledgedtimeout). Both failures are write-acknowledgement timeouts, not search responses.Not run:
--releasebuilds (disk on this machine is at 99%, and fat LTO would not fit; I used thequickprofile that FAST_BUILDS.md documents for verification).cargo clippylocally (CI runs it). The other ~80xerj-apiintegration test binaries, for the same disk reason.Provenance
quickprofile, based on mainb78004832.statsandextended_statsreject the same types asavg(ES registers all six on NUMERIC/DATE/BOOLEAN). The YAML suite only exercises the four.textis left alone on purpose: ES rejects it with the fielddata error, which this message would misreport. Range andflattenedfields are not refused; ES rejects them too, so they stay as they were.