Repository navigation
fix(api): refuse a bucket-metric buckets_path that reaches no number - #2997
Open
amartya1523 wants to merge 5 commits into
Open
amartya1523 wants to merge 5 commits into
amartya1523 wants to merge 5 commits into
Conversation
max_bucket and its siblings read one number per bucket of the first aggregation in buckets_path, by resolving the rest of the path inside each bucket. When that rest names another multi-bucket aggregation, or a multi-value metric without saying which value, ES fails with "buckets_path must reference either a number value or a single value numeric metric aggregation, got: [LongTerms] at aggregation [x]" (or "... but [x] contains multiple values. Please specify which to use."). XERJ skipped every bucket and answered 200 with a null value. _search now classifies each sibling pipeline's path before running: Object[] when the path goes on below a multi-bucket aggregation, the result class (LongTerms/DoubleTerms/StringTerms by field type, the histogram and range classes) when it stops at one, and the multiple-values error for stats, extended_stats, boxplot and percentiles or percentile_ranks with more than one value. Value keys, bucket properties and anything it cannot classify are left alone. The YAML runner now reads Java's \< and \> escapes as the literal characters they are in ES's patterns; Rust's regex took them as word boundaries, so pipeline.yml's "incorrect name" case, whose message XERJ already returns, could never match. Retires the five aggregations/pipeline.yml catch-lenient entries.
API: nested terms, terms ends (long, keyword, double), and percentiles with several percents (tdigest, hdr, through a filter) are a 400 with ES's reason; metric, single-percent and value-key paths still answer with the value. Runner: the pipeline.yml incorrect-name pattern matches ES's message.
…nner clippy's items_after_test_module rejects a test module ahead of other items.
3 tasks done
…cket-metric-paths
Build + Test failed only on xerj-cluster's log_catchup::restart_replays_the_committed_index_and_in_sync_set timing out; this PR does not touch the cluster crate.
Contributor
Author
|
Status note on the red Build + Test: everything from this PR passes (ES-YAML conformance, Format + Clippy, and the
|
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
A bucket-metric pipeline (
max_bucket,avg_bucket,sum_bucket,stats_bucket, ...) reads one number per bucket of the first aggregation in itsbuckets_path. It gets that number by resolving the rest of the path inside each bucket (BucketHelpers.resolveBucketValue). When the rest names something that isn't a number, ES fails the request:buckets_path(underthe_terms: terms)the_terms>the_terms_2>the_max(path goes on below a secondterms)buckets_path must reference either a number value or a single value numeric metric aggregation, got: [Object[]] at aggregation [the_terms_2]the_terms>the_terms_2(path stops at atermson an integer)..., got: [LongTerms] at aggregation [the_terms_2]the_terms>the_percentiles(percents: [10, 50, 90, 99], tdigest or hdr)..., but [the_percentiles] contains multiple values. Please specify which to use.XERJ skipped every bucket and answered 200 with
"the_bad_max": {"value": null}, which is indistinguishable from "no data". Fourcatch-lenient.txtlines are these cases (aggregations/pipeline.yml):Max pipeline through terms aggMax pipeline on terms aggMax pipeline on percentiles tdigest agg without specifying percentMax pipeline on percentiles hdr agg without specifying percentA fifth entry turned out to be a runner bug, not an engine gap.
Max pipeline on percentiles with incorrect nameexpectsNo aggregation [unknown_aggregation] found for path [the_terms>unknown_aggregation];. XERJ already returns exactly that (find_invalid_pipeline_agg, #1824). The YAML pattern escapes the>as\>, which Java's regex reads as a literal>. The runner compiles patterns with Rust'sregexcrate, where\</\>are start-/end-of-word assertions, so the pattern could never match any body.Fix:
es_compat.rs: new
find_bucket_metric_path_error. It runs insearch_implnext tofind_unsupported_metric_agg_field, where the target indices' mappings are at hand. For every sibling pipeline with a stringbuckets_path, it takes the first element (it must be a multi-bucket aggregation, otherwise the path is left alone) and walks the rest:got: [Object[]] at aggregation [x]got: [<class>] at aggregation [x]. Atermsresult is named by the field's mapped type in the first target index:LongTerms(integer types, date, boolean),DoubleTerms(floating types),StringTerms(keyword, ip, ...).histogram,date_histogram,range,date_rangeandfiltersmap to theirInternal*classes. Anything else isn't classified.stats,extended_stats,boxplot, andpercentiles/percentile_rankswith more than one value) →... but [x] contains multiple values. Please specify which to use.A percentiles with exactly one percent is single-valued (ES 8.4+), and the YAML's passingpercents: [99]cases depend on that.filter,nested, ...) is walked into.p.99,p[99.9],st.max), a bucket property (_count,_key, ...), a single-value metric, and anything unclassified.The error is ES's
aggregation_execution_exceptionroot cause inside a 400search_phase_execution_exception.tests/es-compat-yaml/src/main.rs: new
java_regex_to_rust. It rewrites\</\>to the literal characters before compiling acatch:pattern. Other escapes, including\\, pass through.Evidence
Tests run against main first:
termsend on a keyword (StringTerms) and on a double (DoubleTerms)filterthe_terms>m(max)the_terms>p(single percent)the_terms>p.99the_terms>st.maxpipeline.ymlpattern matches XERJ's message, and\\>stays an escaped backslash.searchbody in the ES-YAML suite. It fires on exactly these fourcatch:requests and on no request that is meant to succeed. Every siblingbuckets_pathinavg_bucket.yml,sum_bucket.yml,percentiles_bucket.yml,stats_bucket.yml,typed_keys.yml,rare_terms.yml, etc. is unaffected.Checks
cargo fmt -p xerj-api -p es-yaml-runner -- --checkcargo test --profile quick -p xerj-api --lib: 432 passed, 0 failedcargo test --profile quick -p es-yaml-runner: 1 passed, 0 failedpipeline.ymlcases now enforce their expectation; their catch-lenient lines are deleted. No othercatch:pattern in the suite uses\<or\>, so the runner change can't change another case.Not run locally:
--releasebuilds,clippyand the integration binaries. Disk on this machine is at 99%; CI runs all of these.Provenance
quickprofile, based on main116d5a676.Object[]andLongTermsare pinned by the YAML._searchis covered;_msearchhas its own path.nulltoday for three valid sibling paths:the_terms>_count,the_terms>t2._bucket_countand a path through a single-bucket agg (the_terms>f>m). They're out of this PR's scope (no YAML case covers them); I can take them as a follow-up.