Repository navigation
fix(aggs): fail weighted_avg on a multi-valued weight, like ES - #2981
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's WeightedAvgAggregator throws when a document that has a value
carries more than one weight ("Encountered more than one weight for a
single document. Use a script to combine multiple weights-per-doc into
a single value."), and since 8.12 that is a 400. XERJ skipped such a
document and answered 200 with an average over the rest, because the
aggregation evaluator had no way to fail a request.
It does now, through the slot marker the bucket-cap check already uses:
run_weighted_avg embeds the failure, the API answers any embedded
reason other than too_many_buckets with the shard-level
illegal_argument_exception 400 instead of leaking it inline, and
merge_metric_agg lets an embedded failure win over another index's
result. A multi-weight document without a value is still skipped, as
ES only reads the weights of a document it collects.
Retires `aggregations/weighted_avg.yml · Multi weight field not allowed`.
Engine: the marker for a multi-weight document with a value or value.missing, and the skip without one. API: the 400 alone and merged with a clean index in both orders; single weights still average.
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
weighted_avgpairs each document's value with one weight. In ES, a document that has a value and more than one weight makesWeightedAvgAggregator#collectthrow. Since 8.12 that's a 400 (search_phase_execution_exception, root causeillegal_argument_exception):XERJ skipped that document and answered 200 with an average over the rest. The number looks plausible and is wrong for the caller's data. One
catch-lenient.txtline is this case:aggregations/weighted_avg.yml · Multi weight field not allowed.The
run_weighted_avgdoc comment said why: "The aggregation evaluator has no error channel." It does have one, but only for the bucket cap. A runner can embed{"error": reason, "__error_status__": 400}in its result slot.search_implalready scans for that, but it only acts when the reason istoo_many_buckets; any other embedded reason went out inline under a 200.Fix:
aggs.rs,run_weighted_avg). A document with more than one weight now embeds ES's failure, if it has a value orvalue.missinggives it one. ES only reads the weights of a document whose value source advances (docValues.advanceExact(doc) && docWeights.advanceExact(doc)), so a multi-weight document without a value is still skipped. The doc comment now describes this.too_many_bucketswith the existingsearch_shard_validation_errorenvelope (400search_phase_execution_exception→illegal_argument_exception) instead of leaking the marker inline. Today only the bucket-cap paths (aggs.rs,fast_aggs.rs) produce the marker, and they keep their own branch, so they're unchanged.merge_metric_agglets an embedded failure win over another index's result. Before, merging a failure with a cleanweighted_avgfrom another index could rebuild a "valid" average, which would hide the failure in a multi-index search. In ES, one shard throwing fails the search.Unchanged: single-valued weights, a multi-valued value field (each value paired with the document's one weight),
missingon either side, and the all-zero-weightsnull.Evidence
Two new tests, run against main first:
value.missinggives it a value/multi,/multi,singleand/single,multiall answer 400 with the outer type, root-cause type and exact reason. The two multi-index orders cover the merge path from both sides./singlestill averages to 4.Checks
cargo fmt -p xerj-api -p xerj-engine -- --checkcargo test --profile quick -p xerj-engine --lib: 941 passed, 2 failed. The two aresnapshot_path_security_tests::location_inside_data_dir_is_allowedand::explicit_allowlist_permits_an_external_root. They fail the same way with main's code on this macOS machine (re-run withaggs.rsreset to main); they don't touch aggregations. CI is Linux.cargo test --profile quick -p xerj-api --lib: 432 passed, 0 failedweighted_avg.yml · Multi weight field not allowednow enforces it; its catch-lenient line is deleted.Multi value field(multi-valued value, single weight) is unchanged.run_weighted_avgdoc comment that described the old skip is updated.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.WeightedAvgAggregator; the YAML only assertsbad_request._shards.failuresin ES. Here it's a 400, the same astoo_many_bucketsalready behaves, since this path has no partial-failure response.