fix(engine): bail to brute for multi-field suffixes at any depth - #129
Merged
Conversation
`dv_col` strips only `.keyword`, so `<field>.raw` — the classic Logstash
multi-field — found no column and failed CLOSED on the columnar path:
predicates became a segment-wide `SegPred::Never` and aggregation targets
skipped the segment entirely, so a panel returned a small plausible number
assembled from the memtable alone instead of an error.
There is no single "the brute path" to be identical to. Which resolver a
name is measured against depends on where in the request it appears, and
they disagree with each other. Both halves below are measured on the brute
leg of `tests/fast_aggs_multifield_suffix.rs` over a 12 000-flushed +
200-memtable corpus, not read off the source:
* a TOP-LEVEL QUERY FILTER runs through `index.rs::get_field_value`,
whose multi-field fallback is `get_field_value(source, &field[..idx])`
— RECURSIVE. It strips an unbounded number of trailing segments, so
`extension.raw`, `extension.raw.raw`, `extension.keyword.raw`,
`extension.raw.keyword` and `extension.raw.raw.raw` ALL return 3 200;
* an agg TARGET and a `filters`/`filter` leaf run through
`aggs::get_nested_field`, which strips exactly ONE segment: 3 200 and
4 buckets for `extension.raw`, but 0 and `[]` for every doubled form;
* `extract_array_path_values`, beside it, strips `.keyword` only.
The previous revision of this fix modelled the reference as
`get_nested_field` alone and probed only the IMMEDIATE parent. Measured on
that revision with the new test, doubly-suffixed names still failed closed
on the columnar path — fast vs brute, identical corpus, same process:
extension.raw.raw term query 0 vs 3 200
extension.keyword.raw term query 0 vs 3 200
extension.raw.keyword term query 0 vs 3 200
extension.raw.raw.raw term query 0 vs 3 200
`dotted_suffix_diverges_from_brute` now walks the whole ancestor chain and
reports the UNION of what any resolver resolves. Resolution itself stays
narrow — widening `dv_col` would make the fast path CLAIM shapes it cannot
prove byte-identical (bool rendering, composite key kinds and typed term
keys all key off the requested name), and per the above there is nothing
single to widen TO. Bailing is right against all three resolvers, because
after a bail the request is answered by the very one it would have been
measured against. Three gates, one predicate:
* `seg_field_kind` -> `Err` — every agg TARGET executor gates on this
first, which covers the sites that `continue` past a column-less
segment rather than bailing;
* `field_needs_brute_fallback` — `exec_agg`'s early guard;
* `resolve_pred` -> `None` instead of `Never` — every predicate site
(`filters`, `filter`, top-level query, adjacency).
A dotted name with NO ancestor column is still answered on the fast path:
`get_field_value`'s recursion bottoms out and finds nothing either, and a
column only ever exists for a scalar leaf, which is the same condition
(`is_leaf`) that resolver puts on its own fallback — so a genuinely absent
nested sub-path under an object parent does not bail. Asserted directly,
so the fix cannot degenerate into "every dotted field goes brute".
Also covered:
* `bool_fields.contains(field)` is left an exact-name lookup. Measured:
a `terms` agg on a boolean field named `<field>.keyword` already agrees
with brute (`key:1/"true" 6 200`, `key:0/"false" 6 000`) because
`is_bool` false fails the numeric-column gate and the request bails.
That gate is the ONLY arm that could render 0/1 where brute renders
false/true, so normalising the name there would add the wrong-rendering
risk, not remove it. Locked in by a regression test.
* graph.rs `kw_col`/`num_col` scoped out and documented: all call sites
pass literal §2.2 envelope names and the module has no mapping-driven
or user-supplied field-name path, so no multi-field suffix reaches them.
The invariant under test is fast-path/brute-path agreement, not hardcoded
counts: one corpus, one process, every shape run on the columnar path and
again with it disabled, and the two maps compared whole across `.raw`,
`.keyword`, `.sort`, `.en`, `.raw.raw`, `.keyword.raw`, `.raw.keyword`, a
triple `.raw.raw.raw`, and an absent field. The absolute assertions exist
only so a change that empties BOTH paths cannot pass, and they pin the two
brute resolvers apart at the numbers above.
Verified per the regression rule: reverting ONLY the ancestor walk in
src/fast_aggs.rs and keeping every test fails
`fast_and_brute_agree_across_multi_field_suffix_shapes` plus the three
rewritten unit tests, and passes with it restored. Full `-p xerj-engine`
suite green (571 tests), fmt and clippy `-D warnings` clean.
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.
Closes #120
dv_colstrips only.keyword, but a top-level query filter is resolved byindex.rs::get_field_value, whose multi-field fallback calls itself on the parent — so it strips an unbounded number of trailing segments. A first cut of this fix modelled the wrong resolver (aggs::get_nested_field, one strip) and probed a single parent withrsplit_once, so every doubly-suffixed name (extension.raw.raw,extension.keyword.raw) still failed closed on the columnar path.Measured, fast vs brute (
XERJ_DISABLE_FAST_AGGS=1), same corpus:termqueryextension.raw.rawextension.keyword.rawextension.raw.raw.rawdotted_suffix_diverges_from_brutenow walks the whole ancestor chain and bails when any ancestor owns a column. Resolution is deliberately not widened — the module contract is that the fast path answers only shapes it can prove byte-identical, and there is no single brute behaviour to widen to (three resolvers strip different amounts). After a bail, the request is answered by the very resolver it would have been measured against.The regression test (
fast_aggs_multifield_suffix.rs) asserts fast and brute agree across ten suffix shapes and a genuinely absent field, in one process, on both execution paths.Corrected before merge
Verification caught overclaims in the doc comment and commit message — the same pattern this branch's earlier revision was blocked for, so worth being explicit:
flatten_to_stringsas the resolver that strips.keywordonly. It isextract_array_path_values;flatten_to_stringsdoes no name resolution at all. Fixed in three places.geo.city) also has no ancestor column and is wrongly kept on the fast path. That is a separate, pre-existing divergence, now filed as fast_aggs: predicates and targets on nested-OBJECT fields (geo.city) diverge from brute #128 and called out honestly in the comment rather than papered over.Scope
This closes the multi-field-suffix divergence #120 is about. The general nested-object divergence (#128) is a different root — the columnar path has no column for any non-top-level field — and needs a schema-driven bail like #104, not the ancestor-column probe.