fix(jq): keep a computed float bare across the reindex bridge - #2933
Conversation
CoverageTotal: 93.49% ⚪ 0.01 pp vs Comparing No per-file coverage changes vs 🔇 0 ignored region(s), 91 tolerated region(s)
Patch coveragePatch: 98.62% (214/217 new lines covered)
Uncovered new lines (3)
Indirect coverage changes🔴 2 lines lost coverage, 🟢 1 lines gained coverage on unchanged code. Indirect changes
|
CoverageTotal: 93.39% ⚪ 0.01 pp vs Comparing No per-file coverage changes vs 🔇 0 ignored region(s), 92 tolerated region(s)
Patch coveragePatch: 98.62% (214/217 new lines covered)
Uncovered new lines (3)
Indirect coverage changes🔴 2 lines lost coverage, 🟢 1 lines gained coverage on unchanged code. Indirect changes
|
`to_json_for_reindex` spelled a bare `Float` with a display formatter (`format_float_yq` / `jq_bare_float_display`), and the reparse could not tell that text from a document literal, so a computed float came back as a `NumberLiteral` and every literal-preserving rule echoed the bridge's spelling: `[0.5+0.5] | .[0] | tostring` was `1.0` in yq mode (yq `1`), and `[2*1e16] | .[0] | tostring` was `"2E+16"` in jq mode (jq `"2e+16"`). The bridge now writes a finite bare `Float` as a token with a doubled exponent marker (`1e0e0`), the same design as the NaN/Infinity sentinels: unparseable as an ordinary number so no literal can collide, digit-leading so the semi-index scans it as a number. `from_number_bytes` and `JsonNumber::as_f64` decode it back to a bare `Float`; `number_literal()` and `as_i64` already refuse it. `to_json_yq` switches to yq's threshold-aware `format_float_yq`, which the old literal accident had been masking, and `reindex_bridge_is_identity` admits a finite `Float` now that the round trip is an identity on it. Also closes #1134 (every intervening stage before `tostring`) and #1144 (`join`/`@csv`/interpolation on a constructed array), and removes the recorded `[(0.5+0.5)] | .[0] == 1` divergence.
Three regressions the token exposed, each on a route that had been relying on the bridge re-spelling a computed float: - `@yaml`/`@props` spelled a bare `Float` with plain `Display` under a comment claiming the bridge never yields one (`[1e17*1] | .[0] | @yaml` gave `100000000000000000`); both arms now take yq's computed-float rule. - A path-context pipe over an owned container (`reduce 1 as $x ([1e10*2]; .[0] | [tostring, key])`) hands the bridge's document to the *generic* materializer, whose literal-first chain fell through to `from_document_float` and re-baked the value as `"20000000000.0"`. A `DocumentValue::bridge_computed_float` hook, consulted before `number_literal()`, keeps it bare there and in `tonumber`. - yq's `--input-format json` DOM route printed a whole float as `a: 100` only because the jq-mode bridge collapsed `Float(100.0)` to an integer literal; real yq's JSON decoder types it `!!int`, so #978's canonicalizer (`from_number_literal_plain`) now does the same, with Go's saturating `int64` round-trip as the boundary. Fixes the pre-existing `--eval-all` divergence on the same route and moves `--argjson`'s rendering with it. Also: `reindex_bridge_is_identity` admits every bare `Float` (the sentinels make NaN/infinite ones identities too); `yq_float_fidelity_fixup` removed, now a dead round trip; tests build tokens with the encoder instead of hard-coding spellings; the container `tostring`/`tojson` rendering and the `tojson` trailing-newline divergences the verification surfaced are recorded in the yq limitations doc.
Stale docs: eval_path_context_pipe_owned/eval_path_context_pipe_detached's doc comments, two of their own tests, and the jq limitations record still claimed a bare Float takes the cheap detached route reindex_bridge_is_identity now admits as an identity -- only NaN and an over-cap NumberLiteral still do. pure_value_matrix's comment repeated the same stale claim. Reuse: extracted sign_len(), collapsing a sign-stripping micro-pattern that had been hand-rolled four independent times in validate.rs (is_valid_number, strip_redundant_leading_zeros, has_leading_dot, parse_computed_float_token). Trimmed to_json_for_reindex's doc comment, which repeated computed_float_token's own bug-history narrative verbatim, to a pointer instead. Efficiency: from_number_bytes now tries is_valid_number before parse_computed_float_token, not after -- semantically identical (the token's doubled exponent marker makes is_valid_number reject every token unconditionally) but skips the token check's contains(&b'e') scan entirely for the common case, including a genuine literal that happens to end in the token's own suffix (5e0, 120e0). Hardening: computed_float_token's finiteness precondition is now a real assert!, not a debug_assert! -- every current caller already guarantees it, but a future one that didn't would otherwise splice unparseable text into reindexed JSON and corrupt the document silently in release builds. Simplification: test_reindex_round_trip_keeps_float_and_literal_apart_2902 no longer hand-rolls a JsonIndex/StandardJson::Array walk; a per-value loop through to_json_for_reindex + from_number_bytes covers the identical property with far less scaffolding, matching the two sibling tests beside it. Coverage: added a negative computed float through a container for both modes (only the sign-stripping unit tests exercised the negative case before; nothing walked the full arithmetic -> array -> reindex -> cursor pipeline end to end for one).
397ceea to
b2d2be6
Compare
Closes #2902. Closes #1134. Closes #1144.
Summary
to_json_for_reindexspelled a bareFloatwith a display formatter, and the reparse could not tell that text from a document literal, so a computed float came back as aNumberLiteraland every literal-preserving rule (yq: scientific-notation YAML literals corrupted by any non-identity navigation #1008/yq: tostring/@json on a computed whole-number float skips the scientific-notation threshold #1054/jq: a computed float past the ordinary range is spelled out in full (.a * 1e100 prints 101 digits) where jq 1.7.1 prints 1e+100 #2456) echoed the bridge's spelling:[0.5+0.5] | .[0] | tostringwas1.0in yq mode (real yq1), and one threshold up jq mode had the same defect —[2*1e16] | .[0] | tostringwas"2E+16"(real jq"2e+16").Floatas a token with a doubled exponent marker (1e0e0) — the same design as the existing NaN/Infinity sentinels: unparseable as an ordinary number so no document literal (nor any jq-lenient spelling this crate preserves) can collide, and digit-leading so the semi-index scans it as a number.OwnedValue::from_number_bytesandJsonNumber::as_f64decode it back to a bareFloat;number_literal()/as_i64already refuse it;"1e0e0" | tonumberstill errors.to_json_yqswitches to yq's threshold-awareformat_float_yq—(1e10*2) | tojsonwas right onmainonly because theToJsonbuiltin itself crosses the bridge and got the literal accident.reindex_bridge_is_identity(perf(jq): any path-context query pays an unconditional O(document size) to_owned_checked conversion, dwarfing #1850's own fix #1909) admits a finiteFloatnow that the round trip is an identity on it.EXPR | . | tostring,(tostring),as $x | $x | tostring,map(... | tostring)) and every yq: join's array-constructed/computed elements still lose the decimal-point distinction via the reindex bridge #1144 shape (join/@csv/@tsv/interpolation on a constructed array) now matches the reference, and the recorded[(0.5+0.5)] | .[0] == 1divergence indocs/compliance/yq/limitations.mdis gone.Test plan
/usr/bin/jq1.7.1 and yq v4.53.3 — every row matches (the only remaining diff,[0.5+0.5] | tostringflow-vs-block, ismain's pre-existing container-tostringstyle and unrelated)!!float 2document floats unchanged through a container (and[.a] | .[0] == 2is nowtrue, as in yq)f64/i64, bit-exact round trip incl.-0.0/subnormal/f64::MAX, decoder rejects1e0/sentinels/malformed;from_number_bytesandJsonNumberdecode; serialize-and-reparse keepsFloat/NumberLiteral/Intapart in both modescomputed_float_through_container_2902(yq) andtest_computed_float_through_container_keeps_computed_spelling_2902(jq) CLI modules, every expectation captured from the pinned binaries; yq: computed-float formatting diverges between @uri/@html/@urid/@base64d (numeric_display_string) and tostring/join (to_json_yq) #1124 known-gap test flipped to"1"; yq-mode skip removed fromeval_owned_pure_agrees_with_the_reindex_bridgecargo test --features cli,simd,regex,serde --workspace,--no-default-features,cargo fmt --check,cargo clippy --all-targets --all-features -D warnings,RUSTDOCFLAGS=-D warnings cargo doc --no-deps --all-featuresall green locallyReview round (second commit)
/code-reviewfound three routes that had been relying on the bridge's re-spelling, all fixed and pinned:@yaml/@propsspelled a bareFloatwith plainDisplayunder a comment claiming the bridge never yields one —[1e17*1] | .[0] | @yamlgave100000000000000000(yq1e+17); both arms now take yq's computed-float rule.reduce 1 as $x ([1e10*2]; .[0] | [tostring, key])) hands the bridge's document to the generic materializer, whose literal-first chain fell through tofrom_document_floatand re-baked"20000000000.0". NewDocumentValue::bridge_computed_floathook, consulted beforenumber_literal()into_owned_at_depth/to_owned_cursor_at_depth/tonumber.--input-format jsonDOM route (--slurp/--eval-all) printeda: 100only because the jq-mode bridge collapsedFloat(100.0)to an integer literal. Real yq's JSON decoder types a whole float as!!int(.c | type→!!intfor1.0,9223372036854775808.0→9223372036854775807,1e19stays!!float), so yq: JSON-sourced literal spelling leaks decimal-point preservation, unlike real yq #978's canonicalizer (from_number_literal_plain) now does the same — which also fixes the pre-existing--eval-alldivergence on that route.--argjson(a succinctly extension, no oracle) moves with it:1.0→1,1.e5→100000; recorded in the yq limitations doc.Also from review:
reindex_bridge_is_identityadmits every bareFloat;yq_float_fidelity_fixup(now a dead round trip per float-bearing[..]/,) removed; tests build tokens with the encoder rather than hard-coding spellings; the pre-existing containertostring/tojsonrendering andtojsontrailing-newline divergences observed during verification are now recorded indocs/compliance/yq/limitations.md.