Skip to content

fix(engine): bail to brute for multi-field suffixes at any depth - #129

Merged
xerj-org merged 1 commit into
mainfrom
fix/issue-120-v2
Aug 2, 2026
Merged

fix(engine): bail to brute for multi-field suffixes at any depth#129
xerj-org merged 1 commit into
mainfrom
fix/issue-120-v2

Conversation

@xerj-org

@xerj-org xerj-org commented Aug 2, 2026

Copy link
Copy Markdown
Owner

Closes #120

dv_col strips only .keyword, but a top-level query filter is resolved by index.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 with rsplit_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:

term query before after
extension.raw.raw fast 0 / brute 3,200 3,200 / 3,200
extension.keyword.raw fast 0 / brute 3,200 3,200 / 3,200
extension.raw.raw.raw fast 0 / brute 3,200 3,200 / 3,200

dotted_suffix_diverges_from_brute now 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:

  • The comment named flatten_to_strings as the resolver that strips .keyword only. It is extract_array_path_values; flatten_to_strings does no name resolution at all. Fixed in three places.
  • The comment claimed a "genuinely absent nested sub-path finds no ancestor column, reports false, and stays on the fast path as a real empty answer" — implying the nested case is safe. It is not universally. Doc-value columns exist only for top-level scalar fields, so a present nested-object value (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.

`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.
@xerj-org
xerj-org merged commit 1f349f9 into main Aug 2, 2026
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

fast_aggs: multi-field suffixes other than .keyword still silently under-return

1 participant