test(jq): pin recurse/.. partial-error behavior and add depth-scaling benchmark - #662
Merged
Merged
Conversation
Every existing recurse/`..` golden fixture wraps its filter in `[...]`, which collapses `QueryResult::Partial` back to a bare `Error` before it reaches stdout — so nothing exercises what `resolve_recurse`'s `Err((outputs, e))` early-return actually streams. Adds two cases, unwrapped so the prefix streams: - path_recurse_cond_error_after_output: resolve_recurse's parameterized recurse(f;cond) path - path_recursive_descent_error_after_output: push_recursive_branches' bare `..` path, with the error triggered by a later pipe stage Captured via ./scripts/sync-jq-golden.sh against the pinned jq oracle; succinctly matches jq byte-for-byte on both, including stderr's `<stdin>:0` line-number quirk. De-risk step for #626 (Cow<'a, OwnedValue> refactor of these two functions). Refs #661
No recurse/`..` fixture nests past 3 levels, so nothing would catch a depth-dependent correctness bug — e.g. a lifetime that only breaks past a few stack frames in #626's proposed Cow<'a, OwnedValue> rewrite of push_recursive_branches/resolve_recurse. Drives path(..), path(recurse), and path(recurse(.k; cond)) — covering both functions #626 touches — against a 300-level linear-nesting document, asserting the exact, independently-derived path set. Depth 300 is deliberately beyond what real jq's own parser accepts on this shape (~128), so this is a pure internal correctness pin, not an oracle comparison. Refs #661
No checked-in benchmark reproduces #626's synthetic linear-nesting document and scaling table, so "the fix worked" would have to be eyeballed rather than measured against a baseline. Adds an in-process Criterion bench (not a CLI-spawn A/B — these depth-100-to-400 documents are far below the >=1MB floor the A/B method's process-spawn rule needs) reproducing #626's document — {"k": {"k": ... "pad": {"a":{},"b":{},"c":{}}}} — and query, .. | .[.k]?, at #626's own depths (100/200/300/400) so a before/after run of that fix is directly comparable to its table. A smoke run already reproduces the O(depth^2) signature #626 describes. Makes no timing assertion itself. Closes #661
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
1 participant
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.
Summary
De-risk step for #626 (proposed
Cow<'a, OwnedValue>refactor ofpush_recursive_branches/resolve_recurseinsrc/jq/eval.rs, to eliminate an O(subtree)-per-node clone). Test/bench-only — no production code changes.tests/data/jq-golden/cases/*_error_after_output/cases pinpath(recurse(f;cond))/path(..)streaming their prefix before erroring — every existing recurse/..fixture wraps its filter in[...], which collapsesQueryResult::Partialback to a bareErrorbefore it reaches stdout, so this behavior was previously untested. Captured via./scripts/sync-jq-golden.shagainst the pinned jq oracle; succinctly matches jq byte-for-byte on both.tests/jq_recurse_depth_tests.rsdrivespath(..),path(recurse), andpath(recurse(.k; cond))against a 300-level linear-nesting document, asserting the exact, independently-derived path set. Covers both functions perf(jq): eval_index_expr/eval_slice_bound eagerly materialize a computed key/bound's full subtree before checking whether it's usable #626 touches. No existing fixture nests past 3 levels.benches/jq_recurse_depth_bench.rsreproduces perf(jq): eval_index_expr/eval_slice_bound eagerly materialize a computed key/bound's full subtree before checking whether it's usable #626's synthetic document ({"k": {"k": ... "pad": {"a":{},"b":{},"c":{}}}}) and query (.. | .[.k]?) at perf(jq): eval_index_expr/eval_slice_bound eagerly materialize a computed key/bound's full subtree before checking whether it's usable #626's own depths (100/200/300/400), so a before/after run of that fix is directly comparable to its table. No timing assertion — for manual A/B use.Test plan
./scripts/sync-jq-golden.sh --check— 331 goldens up to date, no driftcargo test --features cli --test jq_golden_tests— new cases pass, known-failures manifest unchangedcargo test --test jq_recurse_depth_tests— 3/3 passcargo bench --bench jq_recurse_depth_bench -- --quick— runs clean, reproduces perf(jq): eval_index_expr/eval_slice_bound eagerly materialize a computed key/bound's full subtree before checking whether it's usable #626's O(depth²) signaturecargo clippy --all-targets --all-features -- -D warningsand thestd,simd,serde,cli,regex,bench-runner,large-tests,mmap-testsfeature-set variant — cleancargo test --features cli(full suite) — cleangit diff --stat -- src/jq/eval.rs— empty, per this issue's explicit constraintCloses #661