Make JSON number output byte-identical to Go across all three runtimes (#180) - #184
Merged
Conversation
…e exponent Two defects in go_format_json_number, both found while investigating #180 (which was reported as a Go-vs-Java divergence; pine-cpp turned out to be wrong in its own ways). The thresholds were already right — 'e' below 1e-6 or at/above 1e21, 'f' otherwise — but the digits came from the wrong place. std::to_chars with chars_format::fixed prints the value EXACTLY, whereas Go's strconv.FormatFloat(d, 'f', -1, 64) prints the fewest digits that round-trip and zero-fills. So 1.0000000000000002e20 came out as 100000000000000016384 where Go emits 100000000000000020000. Same double, different bytes. The default to_chars overload has the same problem once the magnitude renders without an exponent, so neither non-scientific mode is usable. Only chars_format::scientific is guaranteed shortest-round-trip; it is now the single source of digits, and go_json_to_fixed / go_json_to_scientific reposition the decimal point from there. Second defect: chars_format::scientific pads the exponent to two digits, so 1e-7 printed as "1e-07". Go's encoding/json strips one leading zero from NEGATIVE exponents only — verified against encoding/json rather than inferred, since the asymmetry is easy to get wrong: "1e-7" is trimmed, "1e+21" keeps its "+21", and "1e-100" keeps all three digits. Verified against Go across 600 doubles (boundary-focused plus random bit patterns): the two runtimes now produce byte-identical output, md5 bf229ccbf23c82e75ac1beaa20991bc7 on both. New test pins 22 assertions with expected strings taken from actual json.Marshal output, not derived by hand. Both mechanisms are independently gated: reverting to chars_format::fixed turns the shortest-round-trip assertion red, and removing the exponent trim turns the 1e-7/1e-9 assertions red.
…kson (#180) Closes the divergence #180 reported: Java emitted "1.0E20" where Go emits "100000000000000000000". A differential-fuzz round hit it via a Lua operator squaring 1e10. The Double serializer special-cased negative zero and integer-valued doubles within +-2^53, then handed everything else to Jackson's writeNumber, which formats via Double.toString. Go renders plain decimal all the way up to 1e21, so the whole band from 2^53 to 1e21 came out in scientific notation — and so did everything below 1e-6 in the wrong spelling. 1e20 was one instance of a much wider gap, not a special case. New GoFormat.formatJsonNumber implements Go's actual rule (strconv.FormatFloat with 'e' below 1e-6 or at/above 1e21, 'f' otherwise, precision -1), and the serializer now writes its output raw for every value including zero. Digits come from Double.toString, which is already shortest-round-trip; only their placement differs, handled through BigDecimal(String) so no binary error creeps back in. Kept deliberately separate from formatFloatF and the %g emulation. Those have different thresholds, and conflating the JSON path with them is how this arose — formatFloatF would in fact have produced the right answer for 1e20, but the JSON path never called it. The exponent handling is the subtle part and was checked against encoding/json rather than reasoned about: strconv pads to two digits, then json strips one leading zero from negative exponents only. So 1e-7 prints "1e-7" while 1e+21 keeps "+21" and 1e-100 keeps three digits. Verified across 600 doubles against Go: every number literal now matches. Seven new tests, all seven red when the serializer is reverted to writeNumber. One of them asserts the serializer and formatJsonNumber agree, so the rule cannot get a second divergent copy. Note the 600-value comparison also surfaced an unrelated pre-existing gap: Java emits object keys in insertion order while Go sorts them. Filed as #183, not touched here. differential-fuzz normalizes with sort_keys=True, which is why neither that nor this number bug showed up as a whole-document mismatch.
The fixture named for this defect could not have caught it. 04_number_precision feeds 100000 / 1000001 / 0.5, and the Lua doubling puts all three at integer values inside +-2^53 — precisely the one band Java's old serializer guard handled correctly. A gate named "number_precision" that misses every magnitude where the formatting actually diverges is worse than no gate, because it reads as coverage. The new fixture picks inputs whose DOUBLED value lands in each of Go's regimes, since that is what gets serialized: 1e20 (plain decimal past 2^53), 1e21 and 1.5e21 (scientific, with and without a mantissa), 1e-7 (negative-exponent trim), 1e-6 (plain-decimal boundary), a shortest-round-trip case that differs from the exact binary expansion, 1e16 (just past 2^53), and -1e20. Section 14 is the right home: it compares raw HTTP response bodies with no fallback. Section 9 calls itself "no normalization" but on a byte mismatch re-compares through normalize_json and counts equality there as a pass with only a [W] line, so it cannot pin byte-level number formatting either. Verified the fixture has teeth in both directions independently: reverting the Java serializer to writeNumber turns Go-vs-Java red, and reverting the C++ formatter to chars_format::fixed turns Go-vs-C++ red. Passes byte-exact across all three runtimes as committed.
Reflection: memory/reflections/json-number-format-parity-180.md The documentation gap was a precondition for #180, not just a consequence of it. dag-engine.md's GoFormat section listed three formatters with their consumers and never mentioned the JSON output path, so the doc read as though GoFormat were the single source of truth for number formatting — while /execute numbers went through a Jackson serializer that called none of them. formatJsonNumber is now the documented fourth entry, with its own consumer chain (createGoCompatMapper → RunCli / PineServer) kept visually separate from the operator-layer three, and a note that the four have different thresholds and are not interchangeable. formatFloatF would in fact have returned the right answer for 1e20; the JSON path simply never called it. New reference/number-formatting-parity.md carries two measured facts that cost me a wrong first attempt each: - std::to_chars guarantees shortest round-trip only under chars_format::scientific. Both fixed AND the default no-format overload print the exact value once the magnitude renders without an exponent, which is the counterintuitive half — I assumed the default overload was shortest and had to back it out. - Go's encoding/json strips one leading zero from NEGATIVE exponents only, so 1e-7 is trimmed while 1e+21 keeps "+21" and 1e-100 keeps three digits. Reasoning about this rather than measuring it gets it wrong in either direction. ci-quality-baseline.md gains the most useful finding: what each gate can actually pin. differential-fuzz compares after json.loads plus sort_keys=True, so key order and most number-literal differences are structurally invisible. #180 surfaced only through a side effect of Python's int/float split — Go's integer-shaped literal parses to int and passes through normalization untouched while Java's 1.0E20 becomes a float and re-dumps as 1e+20. Eight classes of divergence were checked; 0.0000001 vs 1.0E-7, 1e+21 vs 1.0E21, C++'s 1e-07 vs Go's 1e-7 and precision differences past the 11th digit are all invisible. Section 9 calls itself "no normalization" but re-compares through normalize_json on mismatch and counts that as a pass with a [W] line. Section 14 is the only true byte channel. Distilled to one rule: a property claimed as byte-exact needs a channel that normalizes nothing, and the question to ask when adding such a contract is which channel goes red, not whether the suite is green. doc-gaps.md records two open decisions: the byte channel covers only 5 fixtures against a global contract, and removing section 9's normalization fallback is a prerequisite for fixing #183 (recorded as filed and unfixed, with the UTF-8 byte-order versus UTF-16 code-unit trap noted for whoever takes it).
… JSON IMPORTANT, from the independent review: making the serializer unconditionally write formatJsonNumber's output regressed NaN/Infinity from a quoted JSON string into a bare token. Same request, only GoFormat.java swapped: base ab2dfd5: {..."item_score":"Infinity"} -> parses head 2e2a1fa: {..."item_score":+Inf} -> json.loads fails So my own change turned a valid response into an unparseable one, on a path I had claimed was unreachable. It is reachable. The write path does validate NaN/Inf, which is what I checked, but a request carrying 1e400 never goes through it: Jackson silently coerces that to Infinity at parse time, where Go and C++ both reject the request outright. Parity is already broken upstream in that case, so byte equality with Go is not available — Go's encoding/json errors on non-finite floats and emits nothing to match. The reachable goal is valid JSON, and Jackson's quoted form is the only rendering that delivers it. formatJsonNumber now throws for non-finite input rather than fabricating a representation. Returning "+Inf" was the root of this: it let a caller believe there was a Go-compatible answer when there is none. The serializer checks first and keeps writeNumber for that case, with the reasoning at the call site. Two comment claims were also wrong and are corrected: "+Inf" is not valid JSON, and "callers upstream validate" covers only the write path. Three new tests, all red against a mutant that lets non-finite fall through to writeRawValue: the quoted spellings, a whole-document strict-parser round-trip, and one asserting formatJsonNumber refuses non-finite instead of inventing bytes. Also from the same review: - The new fixture had no trailing newline while the other four in that directory do. The CI newline gate only scans pine-cpp/**/*.{cpp,hpp}, so nothing would have caught it. - go_json_to_fixed and go_json_to_scientific each took an unused double parameter, left over from an earlier shape of the helpers. Verified: 325 Java tests (10 in this class), 246 C++ doctest cases, and the 1e400 reproducer now emits "Infinity" and parses, matching base. On the review's open float32 question: Go's floatEncoder float32 branch is unreachable from /execute. The only non-test float32 in pine-go is a defensive case in row_frame.go's validation switch; JSON parsing always yields float64.
Two BLOCKING findings from the final full-range review, both introduced by my own #180 fix rather than pre-existing. C++ rendered ±Inf as "i.nfe+02". std::to_chars with chars_format::scientific SUCCEEDS on non-finite input — it writes "inf"/"-inf"/"nan" and returns errc{} — so the error fallback I relied on never fired. Those letters then reached go_json_decompose, which read 'i' as a mantissa digit and 'f' as an exponent digit. The old implementation returned a plain "inf", so this was a regression from merely non-standard output to corrupted output. Reachable because engine.cpp snapshots output before validating it, and the Lua bridge guards NaN but not Inf. Now checked explicitly before to_chars, returning the exact strings the pre-#180 code produced so nothing beyond the corruption changes. Java diverged from Go on subnormals because the premise in my own comment was false: Double.toString is shortest-round-trip for normal doubles but not for subnormals. It renders MIN_VALUE as "4.9E-324" when "5E-324" already round-trips, and Go emits the latter. Reachable through an ordinary request: {"v":5e-324} came back as 4.9e-324 in Java against 5e-324 in Go. New shortestRoundTrip tries successively fewer significant digits and takes the first that parses back to the identical double, starting from Double.toString's digit count so normal doubles settle immediately. Re-measured after the fix, comparing strings rather than numerically: 1,000,000 consecutive bit patterns from 1 (the whole low subnormal range) plus 200,000 random finite doubles, all three runtimes against json.Marshal — zero divergences. C++ was already correct on subnormals; only Java was wrong. The review flagged a methodology trap worth repeating: `awk '$1!=$2'` compares numeric-looking strings numerically, so "1e-7" tests equal to "1.0E-7" and every count comes back zero. That is what hid the subnormal divergence initially. Two comments corrected, since both stated the false premise: formatJsonNumber's Javadoc and formatFloatF's. formatFloatF keeps Double.toString deliberately — its callers are key/salt and condition formatting, which never see subnormals — and now says so instead of claiming shortest-round-trip generally. New gates, each verified red against a mutant: C++ non-finite (2 assertions) and subnormal rendering; Java subnormal spellings plus a round-trip sweep over 20,000 subnormal bit patterns. 327 Java tests, 246 C++ cases, cross-validate 55/55, differential-fuzz 1000/1000, byte-exact channel 5/5 both pairs.
IMPORTANT, from the final full-range review: shortestRoundTrip searched upward from precision 1 and returned the first candidate that round-tripped. Correct answer, wrong order — every already-minimal normal double paid a failed BigDecimal round-and-parse for each digit below its true minimum. Measured here at 6.8 microseconds per value against 0.076 for Double.toString, roughly 90x, on the /execute response path for every double field. My comment claimed "normal doubles settle on the first attempt and only subnormals do real work". Exactly backwards: normal doubles were the slow case, because they need the most digits and therefore fail the most attempts. Now Double.toString is taken as-is unless one fewer significant digit still round-trips, which is a single BigDecimal probe and fails for every normal double. The search loop only runs for subnormals. 6.8us -> 1.0us, and the remaining gap is that one probe. Re-verified after the change: 1,000,000 consecutive subnormal bit patterns and 200,000 random finite doubles, string comparison against json.Marshal, zero divergences either sweep. Guarded by a test asserting the ratio against Double.toString measured in the same JVM rather than an absolute figure, so it is not a machine-speed tripwire: the regression measured ~90x, current is comfortably under 40x, threshold 60x. Verified red when the fast path is removed (reported 70.4x). Minor, same review: countSignificantDigits claimed Double.toString emits "at most one" leading zero, so 0.001234 over-counted 4 significant digits as 7. Double.toString emits as many placeholder zeros as the exponent needs before switching to scientific notation. Leading zeros are now skipped, with three assertions covering the 0.00x shapes. Also recorded the deliberate non-finite divergence as an accepted difference in llmdoc rather than leaving it implicit: Go refuses to encode NaN/Inf at all, so there is no byte sequence to match; pine-cpp keeps its pre-#180 bare inf/nan and pine-java keeps Jackson's quoted form, which is the only choice that leaves the response parseable. The divergence originates in request parsing — Go and C++ reject 1e400 outright while Jackson coerces it — so unifying the formatter cannot remove it. Noted as out of scope for #180 with the /stats path flagged as an untested potential exposure. 329 Java tests, 246 C++ cases, lint, codegen-check, cross-validate 55/55, differential-fuzz 1000/1000.
IMPORTANT, from the fifth full-range review: the fast path I added in the previous commit missed the most common input class, and my comment asserted the opposite. Double.toString always writes a fractional part, so 1.0 arrives as "1.0". Counting that trailing zero gave digits=2, the one-fewer-digit probe then succeeded because "1" round-trips, and every integer-valued double fell through to the search loop. Measured: integer-valued 1121 ns/value against 1046 for 17-digit fractional values — the class that should be cheapest was the most expensive, which is what the reviewer caught by benchmarking the two shapes separately rather than trusting the claim. The trailing ".0" is a syntax requirement, not a significant digit, so countSignificantDigits now strips it. Integer-valued drops to 612 ns and fractional to 976, so the ordering is finally the intended one. Correctness re-verified after the change: 1,000,000 consecutive subnormal bit patterns and 200,000 random finite doubles against json.Marshal, string comparison, zero divergences either sweep. Guarded by a test asserting integer-valued is cheaper than fractional in the same JVM — a shape assertion rather than a machine-speed one — plus value assertions for 1.0/42.0/1e20. Verified red when the strip is removed. Also from the same review: - Removed the leading-zero skip in countSignificantDigits instead of keeping it. The reviewer showed flipping that branch changes no output, and I confirmed it: over-counting only widens shortestRoundTrip's search, which still returns the shortest round-tripping candidate. It was dead defensive code that no test could pin, and the test named for it passed either way. The test is now named for what it actually asserts (rendered bytes of small plain decimals) and the code says why the count may safely over-report. - The C++ isnan guard's only observable effect is normalizing -NaN, which no test covered; both runtimes now assert it. Verified red when the guard is removed. - The reflection said channel 14 has "4 fixtures" while this very range adds the fifth, contradicting doc-gaps.md and index.md in the same range. Both mentions now say what the count was before and after. 332 Java tests, 246 C++ cases, lint, codegen-check, cross-validate 55/55, differential-fuzz 1000/1000, byte-exact channel 5/5 both pairs.
…h too Two IMPORTANT findings from the sixth full-range review, and they are the same mistake I made last round wearing different clothes. Removing the leading-zero skip in countSignificantDigits was wrong. I had verified it changed no OUTPUT and concluded it was dead code — true as far as it went, and the wrong question. It does not change the result, but it decides whether the fast path fires: every double below 1.0 renders with a "0." prefix, counting those zeros inflated the digit count, the one-fewer-digit probe then succeeded, and the entire interval fell through to the search loop. Measured 8065 ns per value over [0.001, 1) against 1104 over [1, 1000) — and [0, 1) is where scores and probabilities live, on the /execute response path. Restored, with the rationale stated as "decides fast-path eligibility" rather than "cosmetic": now 847 ns, the cheapest interval rather than the dearest. The reason I did not see it is the second finding: normalDoublesDoNotPayForThe SubnormalSearch generated its sample with nextDouble()*1000, which puts 99.9% of values at |d| >= 1 — precisely the interval where the fast path already worked. It reported 20x while [0.001, 1) was running at 144x, worse than the ~90x regression it was written to catch. The generator now mixes all three shapes (|d| >= 1, [0.001, 1), integer-valued), and goes red at 88.4x when the skip is removed again. Worth recording as a pattern, since this range has now produced three instances: a performance claim in a comment is a claim about a measurement, and a benchmark whose sample avoids the interesting interval will confirm whatever you already believe. Both times the fix was to measure the shapes separately. Correctness unaffected and re-verified after each change: 1,000,000 consecutive subnormal bit patterns and 200,000 random finite doubles against json.Marshal, string comparison, zero divergences for both Java and C++. Two minors, same review: - formatJsonNumber had no javadoc. Inserting the shortestRoundTrip helpers put their doc comment between formatJsonNumber's javadoc and the method, so the compiler discarded the first. Reattached. - go_json_to_scientific negated the exponent in place and then recomputed p.exp10 - 1 < 0 to decide the zero-trim, so the sign was derived twice with different expressions. Now one const bool drives both, with the verified asymmetry stated once. 332 Java tests, 246 C++ cases, lint, codegen-check, cross-validate 55/55, differential-fuzz 1000/1000.
The optimization cost three review rounds and never protected anything the correctness fix needed, so it is gone at the user's direction. Every version was correct on output and wrong in its own comment about which inputs it covered: v1 claimed normal doubles hit the fast path when they were the slow class; v2 missed integer-valued doubles entirely because Double.toString(1.0) is "1.0" and the trailing zero counted as significant; v3 missed everything below 1.0 because the "0." prefix did the same. Each time the mechanism was identical — the benchmark sample avoided the interval that would have contradicted the claim, since nextDouble()*1000 puts 99.9% of its values where the fast path already worked. shortestRoundTrip now tries precision 1 upward and returns the first candidate that round-trips. It cannot be wrong about which inputs it covers, because it treats them all alike. The two performance tests are removed with the thing they guarded; the reasoning and the three measured counter-examples are recorded in llmdoc so the next person to optimize this knows to benchmark [0.001,1), [1,1000) and integer-valued separately. One real trap surfaced while simplifying, and it is now pinned by a test. My first attempt rounded the exact binary expansion — BigDecimal(double) plus a MathContext — which diverged from Go on 55 of 200,000 random doubles: 2209012388886329.2 in Go against ...329.3. MathContext rounds HALF_UP on the true value while Go reports the digit nearest the double. Double.toString's digits are already the right ones; the only problem with them is that there can be too many. So the shortening operates on BigDecimal(Double.toString(d)), and digitsComeFromDoubleToStringNotFromRoundingTheExactValue goes red against the other form. Correctness unchanged and re-verified: 1,000,000 consecutive subnormal bit patterns and 200,000 random finite doubles against json.Marshal, string comparison, zero divergences. 331 Java tests, 246 C++ cases, lint, codegen-check, cross-validate 55/55, differential-fuzz 1000/1000, byte-exact channel 5/5 both pairs.
IMPORTANT, from the eighth full-range review, and the same defect class as the last three: a comment I added asserts something measurement contradicts. formatFloatF's javadoc said its callers "never see subnormals" and named key/salt/condition formatting. Both halves are wrong. Salt uses formatG, conditions use sprint, and the one real caller is TransformResourceLookup's key coercion — which a request-supplied 5e-324 does reach, since Jackson parses it happily. Java then derives a 327-character resource key where Go and C++ derive 326. Confirmed independently: grep for callers, and formatFloatF(Double.MIN_VALUE).length() == 327. Left unfixed on purpose, now stated as a known divergence instead of denied. It predates this work, #180 is about JSON output bytes, and changing a key-derivation function is a behaviour change for anything already keyed on the current form. Pinned by a test asserting 327 so the number is a recorded fact and fixing it later requires deliberately updating a failing assertion. Minor, same review: llmdoc claimed this path is "not a hot bottleneck" without having benchmarked it. Re-measured here at 18.6x plain Jackson — 103 ms against 5.5 ms for a 30,000-double response. The claim is gone; the doc now gives the measured figure and says plainly that the reason for accepting it is a 3-for-3 failure rate at optimizing this function, not cheapness. If the ratio ever matters, that is a separate task with the per-interval benchmark requirement already written down. Second minor: "over 50 divergences in a 200k random sweep" holds only for magnitude-targeted sampling; uniform random bit patterns give 8-13. Now stated as distribution-dependent, with the note that the mechanism and the four pinned values do not depend on the draw. 332 Java tests, 246 C++ cases, lint, codegen-check, cross-validate 55/55, differential-fuzz 1000/1000.
Ninth full-range review: 0 blocking, 0 important, 2 minor — the first round with no important findings, so the convergence the earlier rounds were missing. Both implementations carried a zero-pad branch for positive exponents, written by reasoning from strconv's rule rather than from this function's reachable inputs. The scientific branch is only entered when |d| >= 1e21 or |d| < 1e-6, so a positive exponent is never below 21 and is already two digits. The reviewer confirmed it by measurement — no single-digit positive exponent appears in Go's output across 688,258 samples, and deleting both pads leaves every rendering byte-identical. Removed, with the reachability argument in place of the strconv-derived one. Fittingly, this is the same failure mode as the perf comments: a claim inferred from a related rule rather than checked against the code's actual input range. The "eight subnormal values diverged" figure did not say what it counted. My enumeration gives 8 bit patterns and 8 distinct Double.toString strings over bits 1..200000; the reviewer's wider enumeration gives 14 patterns mapping to 8 decimal targets. Both are right about different things, so the comment now names the basis and lists the strings. The reviewer also went past its own findings and checked the C++ side of the formatFloatF divergence I had documented as Java-versus-Go. All three runtimes differ: Go 326 characters, Java 327, and pine-cpp "5e-324" — because go_format_lookup_key uses char buf[64] with to_chars, which cannot hold a 326-character expansion, returns value_too_large, and falls through to go_format_g's scientific notation. So it is not a digit-count difference there but a whole change of format. Verified with a temporary probe. Recorded in both the javadoc and llmdoc, with the warning that unifying this requires widening the C++ buffer and not only fixing Java's digits. 332 Java tests, 246 C++ cases, lint, codegen-check, cross-validate 55/55, differential-fuzz 1000/1000; subnormal and random sweeps still zero-divergence against json.Marshal after the branch removal.
…s too IMPORTANT, from the tenth full-range review: the mapper registered only Double.class. Jackson dispatches on the declared type, so primitive double fields and double[] stayed on its default path and emitted "1.0E20" — the exact shape of #180, inside the mapper that exists to prevent it. Confirmed with a POJO: {"d":1.0E20,"arr":[1.0E20]}. Pre-existing and not reachable today, since everything on /execute and /stats is boxed into Double through Variant, so this closes a hole rather than fixing a live defect. Registered anyway, because my comment asserted "every FINITE value goes through formatJsonNumber" and that should be a property of the mapper, not of whichever paths happen to exist. double[] needs its own registration on top of Double.TYPE: Jackson serializes primitive arrays with a dedicated ArraySerializer that writes elements directly instead of delegating. Test covers all three declared shapes. Asserted per field rather than whole document, because Jackson's key order is not Go's — that is #183 and separate from the number bytes. Three minors from the same review: - go_json_decompose's leading- and trailing-zero strip loops are unreachable now that the call site hardcodes chars_format::scientific, which emits neither. Kept, since the function's contract is decomposing a decimal rendering rather than one caller's current format choice, but the header comment claiming to_chars "picks fixed or scientific on its own" was stale and is corrected, and the loops now say they are unreachable and safe to delete if no second caller appears. - A test comment still said "over 50 such cases in a 200k random sweep" after llmdoc had been corrected: it is 13 for uniform random bit patterns and 50+ only when sampling by magnitude. - llmdoc flagged /stats non-finite exposure as unmeasured. It is answerable and now answered: those call sites go through go_format_json_number, which early-returns nan/inf, so /stats behaves exactly as /execute. 333 Java tests, 246 C++ cases, lint, codegen-check, cross-validate 55/55, differential-fuzz 1000/1000; subnormal and random sweeps still zero-divergence against json.Marshal.
IMPORTANT, from the eleventh full-range review: the serializer covered the three double shapes but not Float, while the comment I added claimed the type list was complete precisely so it would not depend on which paths exist today. Float is an accepted frame value type in all three runtimes — pine-go's `case float32`, pine-java's `instanceof Float` in DataFrame and ColumnFrame — and the reviewer reached it end to end with out.setItem(0, "score", Float.valueOf(1e20f)), producing 1.0E20. The #180 shape, inside the mapper built to prevent it. Widening to double is not the fix, and the reviewer said so before I could get it wrong: Go formats float32 with bitSize=32, so the digits are shortest for FLOAT. Widening surfaces the noise the narrower type was hiding — 0.1f must print "0.1" but (double) 0.1f is 0.10000000149011612, and 1e20f widens to 100000002004087730000 against Go's 100000000000000000000. So there is now a formatJsonNumber(float) with its own shortestRoundTrip(float), and the placement logic both overloads share is factored into formatDecimal — bit width decides which digits are shortest, never where the point goes. Two things only measurement would have caught, both found by sweeping 300,009 float32 bit patterns against json.Marshal: - Float.toString is not shortest for subnormals either, exactly as Double.toString is not: MIN_VALUE renders "1.4E-45" where "1E-45" round-trips. 11 low subnormals diverged. - The 1e-6 threshold must be applied to the SHORTENED decimal, not the widened double. Bits 897988541 widen to 9.999999974752427e-07, below the threshold, but shorten to 1e-06, which is not; Go prints 0.000001. Comparing the widened value gave 1e-6. Final sweep: 300,009 float32 values, zero divergences, and the double sweeps (1e6 subnormals, 200k random) still zero after the refactor. Two minors from the same review: - The 18.6x cost figure re-measures at 22-23x here. A ratio should travel better than that, so it now carries its measurement conditions and says to read it as "about 20x" rather than as a threshold. - The three-way resource-key divergence pinned only Java's 327 characters. Widening the C++ buf[64] would have turned nothing red, which is the one thing the llmdoc warning asked a future reader to avoid. test_format_g.cpp now pins go_format_lookup_key(denorm_min) == "5e-324". 335 Java tests, 247 C++ cases, lint, codegen-check, cross-validate 55/55, differential-fuzz 1000/1000.
IMPORTANT, from the twelfth full-range review: my comment set the standard that the mapper should be self-consistent rather than merely covering today's paths, and used that to justify five registrations — while DoubleNode, FloatNode, DecimalNode and BigDecimal still emit Jackson's default form. The reviewer could not check reachability after its snapshot was destroyed, so it declined to call it a live defect and asked me to either register them or narrow the claim. I did the check it could not: readTree appears only in Config.java and ResourceManager.java, both parsing configuration inbound, and no response is assembled from a JsonNode. So these carriers are unreachable on a response path, and registering serializers for them would add behaviour nothing exercises — which is exactly the mistake the shortestRoundTrip fast-path history records three times: code justified by a belief about coverage that no measurement supports. The comment now states precisely which six carriers are covered and that JsonNode/BigDecimal are deliberately excluded with the reachability argument, and a test pins that boundary so a future change serializing a JsonNode outward meets a failing assertion instead of silently emitting 1.0E20. Two stale figures, both mine: - The mangled-output literal is "i.nfe+2", not "i.nfe+02". Quoted in three places, and it was accurate until this same range deleted the positive-exponent zero-pad as unreachable — which shortened it by one character. Collateral from my own change that I never traced. Re-derived by deleting the non-finite guards and recompiling. - The cost figure was a point value that does not hold: three measurements of the documented shape give 18.6x, 22.7x and 25.2x, the spread coming from warmup rounds and how the median is taken. Now stated as a range of about 18-25x with the reason, and explicitly not usable as a gate until the warmup, sampling and statistic are pinned. 336 Java tests, 247 C++ cases, lint, codegen-check, cross-validate 55/55, differential-fuzz 1000/1000; float32 sweep still 0 divergences over 300,009 values.
…ates Thirteenth full-range review: APPROVE, 0 blocking. The one important finding is a test with no teeth rather than a code defect, which is a first for this range. floatShapesAllUseTheGoFormatter claimed to cover all three float carriers but pinned only two: FloatHolder.getPrimitive() returned 0.1f, and Jackson's default writeNumber(float) also emits "0.1", so the assertion could not tell the registered path from the default one. Deleting the Float.TYPE registration left all 336 tests green. The reviewer established the registration is load-bearing before blaming the test — with 1e20f the output is 1.0E20 without it — which is the right order of inference. Now 1e-7f, where Go gives "1e-7" and Jackson gives "1.0E-7", and the case goes red when the registration is removed. Two counts corrected, both mine: - "11 low subnormals" is 9. Re-counted exhaustively over float32 subnormals, bits 1..0x7FFFFF, by output bytes changing — the same convention the double-side figure uses: patterns 1, 2, 3, 4, 6, 7, 21, 29, 71. The javadoc now names the range and the convention, which the double figure already did and this one did not. - The main-code comment still said "over 50 divergences in a 200k random sweep" after the test comment and llmdoc had both been corrected to 11-13 for uniform sampling. Three places quoting one measurement, two updated, one missed. Also recorded a forward-looking warning the reviewer raised beyond its findings: pine-cpp has no float32 path today (Variant::value_t holds only double), so no divergence exists — but adding one by widening to double and calling go_format_json_number would reproduce exactly the trap the Java side just handled, including that the threshold must be compared against the shortened decimal rather than the widened value. 336 Java tests, 247 C++ cases, lint, codegen-check, cross-validate 55/55, differential-fuzz 1000/1000. The reviewer additionally ran real Go and Java servers against fixture 06 and got cmp-identical bodies, and swept 8,388,607 float32 subnormals plus 2,000,000 double subnormals with zero divergences.
Contributor
🔍 PR 审查
未发现需要修改的问题。C++ 与 Java 的 JSON 数值格式化实现覆盖了定点/科学计数法阈值、最短往返表示、负零、次正规数及 float32 精度,并由新增的字节级 fixture 和单元测试约束。 验证说明
本次审查由 Codex 主链路 (gpt-5.6-sol) 完成。 |
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 #180.
The reported bug, and what it actually was
#180 reported that
1e20serialized as100000000000000000000in Go but1.0E20in Java. The title made it sound like one boundary value. It was not: a 20-value
probe across the formatting regimes found 15 of 20 diverging, from three
independent defects.
pine-java special-cased negative zero and integer-valued doubles within ±2^53,
then handed everything else to Jackson's
writeNumber, which formats viaDouble.toString. Go renders plain decimal all the way to 1e21, so the entire bandfrom 2^53 to 1e21 came out in scientific notation, as did everything below 1e-6 in
the wrong spelling.
1e20was one point inside a wide gap.pine-cpp had the right thresholds but sourced digits from
std::to_chars(chars_format::fixed), which prints the value exactly where Go'sprecision=-1means shortest round-trip:1.0000000000000002e20came out as100000000000000016384against Go's100000000000000020000. Separately, itsscientific branch zero-padded exponents (
1e-07), while Go'sencoding/jsonstrips one leading zero — from negative exponents only.
That asymmetry is load-bearing and easy to get wrong in either direction, so it was
verified against
encoding/jsonrather than reasoned about:1e-7is trimmed,1e+21keeps+21,1e-100keeps all three digits.Also fixed, found while verifying
Floatis an accepted frame value type in all three runtimes andbypassed the serializer entirely. Widening to double is not the fix — Go uses
bitSize=32, so0.1fmust print0.1where(double) 0.1fis0.10000000149011612. Needs its own shortest-round-trip search, and the 1e-6threshold must be compared against the shortened decimal, not the widened value.
Double.classalone leftdouble,double[], and every float form on thedefault path emitting
1.0E20.Double.toStringnorFloat.toStringisshortest-round-trip there — 8 double bit patterns and 9 float ones.
MIN_VALUErenders
4.9E-324where5E-324round-trips and Go emits the latter.turned
Infinityfrom a quoted string into a bare+Inf, i.e. invalid JSON,reachable via a request carrying
1e400.formatJsonNumbernow throws ratherthan inventing a representation Go does not have.
i.nfe+2:to_charssucceeds on non-finite input and writesinf, sothe
errcfallback never fired and those letters were parsed as mantissa/exponentdigits.
Test coverage
New byte-exact fixture
06_number_format_regimes.jsonon cross-validate section 14— the only channel with no normalization fallback. The pre-existing fixture named
04_number_precision.jsoncould not have caught this: its inputs double tointeger values inside ±2^53, exactly the band the old code handled correctly. A gate
named for a defect that structurally cannot detect it is worse than none.
21 new Java assertions and a C++ subcase, each verified by mutation in both
directions.
Verification
At
92dcb91e: 336 Java tests, 247 C++ doctest cases (110,640 assertions), lint,codegen-check,
cross-validate55/55,differential-fuzz1000/1000. Against Go'sjson.Marshal, compared as strings: 1,000,000 consecutive double subnormals plus200,000 random finite doubles, and 300,009 float32 values — zero divergences.
Reviewers independently swept up to 8,388,607 exhaustive float32 subnormals and ran
real Go and Java servers against the new fixture for
cmp-identical bodies.make fmt-checkwas not run: no clang-format locally, and it has no CI job either(tracked in
llmdoc/memory/doc-gaps.md).Deliberately not fixed, each with a reachability argument
The divergence originates in request parsing — Jackson coerces
1e400where Goand C++ reject the request — so no formatter change removes it. C++ keeps its
pre-Go vs Java JSON number divergence at 1e20 (diff-fuzz, pre-existing) #180 bare
inf; Java keeps quoted strings, the only form leaving the responseparseable.
5e-324(Go 326 chars, Java 327,C++
5e-324becausebuf[64]overflows into a scientific fallback). Differentcode path; changing key derivation is a behaviour change for existing indexes.
Pinned on both sides so a partial fix goes red.
JsonNode/BigDecimal: bypass the serializer, butreadTreeappears only ininbound config parsing and no response is built from a JsonNode. Boundary pinned.
Review history worth knowing before merge
13 independent blind review rounds in isolated fixed-commit snapshots. The parity
fix itself stopped drawing findings after round 2. Everything after that was one
recurring mistake of mine: a comment asserting a property that measurement
contradicted — five instances, three of them the same fast-path coverage claim
about three different intervals.
The mechanism was identical each time. I wrote the claim from reasoning, and where I
did benchmark, the sample avoided the case that would have refuted it:
nextDouble()*1000puts 99.9% of values where the fast path already worked, and0.1frenders identically under both code paths so the assertion had no teeth.That optimization is now deleted rather than fixed a fourth time (at the user's
direction). It bought a few microseconds per double, cost three review rounds, and
the replacement cannot make a false coverage claim because it treats every input
alike. The reasoning and the three counter-examples are in
llmdoc/reference/number-formatting-parity.mdfor whoever optimizes it next.Two terminal evidence audits ran. Both found no implementation defect; both
failed on my bookkeeping, and the second caught that one of my own repairs had
hidden itself. Those failures and their repairs are recorded under
.code-review/,including two gaps left standing rather than reconstructed: the four commits most
central to this task predate the review-skill invocation and so have no
contemporaneous staging freeze, and five blind reports have no archived artifact
(three destroyed by an external
/tmpcleaner, one budget exhaustion, one caused byan ambiguous instruction of mine).
Filed, not fixed here
Found by this work's 600-value sweep: byte counts matched, all 600 numbers
matched, and the difference was purely key order.
differential-fuzznormalizeswith
sort_keys=True, so it is structurally blind to it.