Conversation
CoverageTotal: 93.95% ⚪ 0 pp vs Comparing No per-file coverage changes vs 🔇 0 ignored region(s), 175 tolerated region(s)
Patch coveragePatch: 100% (6/6 new lines covered)
|
CoverageTotal: 94.05% ⚪ 0 pp vs Comparing No per-file coverage changes vs 🔇 0 ignored region(s), 174 tolerated region(s)
Patch coveragePatch: 100% (6/6 new lines covered)
|
newhoggy
force-pushed
the
issue-2925-path-reindex-bridge-streams
branch
from
September 18, 2026 11:57
52bbeaf to
cb43a0a
Compare
…ect (#2925) eval_each_generic's lazy Builtin::Path arm sent a document the reindex bridge would not round-trip identically (a number literal past REINDEX_LITERAL_LEN_CAP, or a NaN spelling) to eval_on_owned, which collects every path before a wrapping consumer (limit, first, label/break) ever sees one -- over-running jq's own generator order on exactly the documents that select this route. The cursor-navigable and bridge-identity routes already streamed since #2908; this was the one shape #2925 recorded as still collecting after that work (the sibling shape, a generator in index position, had already closed with #2267). Swaps the entry point to eval_each_owned_bridged instead of eval_on_owned -- the demand-forwarding twin eval.rs's own eval_each already uses for isempty/any/all/IN via bridge_to_each_owned_flow. Not the demoting eval_each_owned: this arm already reroot_markers's the expression against the same root before crossing, and the demoting entry's RootWitness::Owned catch-all would silently re-demote a marker Extends path_results_stream_to_their_consumer_2908 with the over-cap rows (confirmed failing pre-fix, matching jq 1.7.1 post-fix) in place of the single residual case it used to carry, and updates the three doc sites (the arm's own comment, limitations.md's "two shapes still collect" paragraph, and builtin_path's now-stale reachability note) to say both shapes are closed. Fixes #2925
Post-rebase review round on #3122's Reentry refactor found: - A stale doc comment in eval.rs's builtin_path still named eval_each_owned_bridged, which #3122 folded into eval_each_owned before this PR's fix commit was even authored -- carried over unedited through the earlier rebase resolution since eval.rs auto-merged with no conflict marker to catch it at. - eval_generic.rs's fixed arm rebuilt Expr::Builtin(Builtin::Path(..)) via a clone of path_expr, when the match's own scrutinee expr already equals that value and can be passed directly. - The new cap256/cap257 test rows claimed to guard the REINDEX_LITERAL_LEN_CAP boundary, but since the fix makes both routes stream identically end-to-end, the CLI-level pair can't actually tell which route ran (confirmed live: flipping the cap comparison to misroute 256 still passed). The real boundary guard is test_reindex_bridge_identity_predicate_agrees_1909, which already pins it directly against reindex_bridge_is_identity; corrected the comment to say what these two rows actually cover (jq-agreement on either side of the boundary) instead.
newhoggy
force-pushed
the
issue-2925-path-reindex-bridge-streams
branch
from
September 18, 2026 12:45
c47116a to
cde7afa
Compare
newhoggy
added this pull request to the merge queue
Sep 18, 2026
Any commits made after this event will not be merged.
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.
Summary
eval_each_generic's lazyBuiltin::Patharm sent a document the reindex bridge would not round-trip identically (a number literal pastREINDEX_LITERAL_LEN_CAP= 256 chars, or a NaN spelling) toeval_on_owned, which collects every path before a wrapping consumer (limit,first,label/break) ever sees one — over-running jq's own generator order exactly on the documents that select this route ([limit(1; path((.a|stderr),(.b|stderr)))]wrote12instead of jq's1). This was the last of the two shapes jq: two path() shapes still collect after #2908 — the reindex-bridge route and a generator in index position #2925 recorded as still collecting after jq: path()'s own consumer side still collects, so a bound outside path() can't stop the fold (#2694 residual) #2908 — the sibling shape (a generator in index position) had already closed with jq: eager path-resolution vs jq's lazy generator can over/under-fire target side effects when a downstream write fails partway #2267.eval_each_owned_bridgedinstead ofeval_on_owned— the demand-forwarding twineval.rs's owneval_eachalready uses forisempty/any/all/INviabridge_to_each_owned_flow.eval_each_owned: this arm alreadyreroot_markers's the expression against the same root before crossing the bridge (jq: a root-bound$xmatches a rebuilt equal-valued copy — TrackedVar's value witness writes where jq refuses #2642/jq: a $var bound from a navigated position refuses at that same node (.a as $y | .a | ($y.b) = 9) #3037), and the demoting entry'sRootWitness::Ownedcatch-all inmarker_needs_demotionwould silently re-demote a marker jq: a $var bound from a navigated position refuses at that same node (.a as $y | .a | ($y.b) = 9) #3037's promotion just proved is the root's own node — a real behavior difference from the prior 2026-09-16 triage's suggested snippet, which predates theeval_each_owned_bridgedsplit (introduced the next day, jq: #2642 fabrication still live when the bind and the rebuild both run inside eval.rs (input builtins, |= RHS, fold UPDATE) #3036/jq: a $var bound from a navigated position refuses at that same node (.a as $y | .a | ($y.b) = 9) #3037's root-witness work). Verified this matters by tracingmarker_needs_demotion's_ => truecatch-all underRootWitness::Owned.path_results_stream_to_their_consumer_2908with the over-cap rows the old residual-guard comment predicted, confirmed failing on pre-fixmain(assert_eqmismatch"123"vs"1") and passing post-fix, matching jq 1.7.1 on every row including the 256/257-character cap boundary and yq mode.docs/compliance/jq/limitations.md's "two shapes still collect" paragraph (also drops its stale "anyFloat" wording — bridge-identity since jq/yq: a computed float read back out of a container regains literal spelling in tostring (1.0 vs 1) #2902 — and cross-references the separate, still-open#3025value-fidelity gap this fix does not touch), andbuiltin_path's reachability note ineval.rs.Test plan
cargo build --features clicargo test --features cli,simd,regex,serde(full suite: 8955 passed, 0 failed)cargo clippy --all-targets --all-features -- -D warningscargo clippy --all-targets --features std,simd,serde,cli,regex,bench-runner,large-tests,mmap-tests -- -D warningsmain(temporarily reverted just the source fix via a taggedgit stash, reran, restored) before confirming they pass post-fix/usr/bin/jq1.7.1 before writing it into the test tablecargo llvm-cov --features cli,simd,regex,serde --workspace --lcov+omni-dev coverage diffFixes #2925