Repository navigation
Conversation
davesnx
left a comment
There was a problem hiding this comment.
Independent review of 7eef191 against main (0f5ca93), commit by commit. Method: code reading of every changed function, a 0f5ca93 release build for differential checks, 50+ hand-built comparison queries plus a 53-query sweep over benchmarks/{small,medium,big}.json and 1M-int, 1M-float, 21 MB-array and 100k-key fixtures, and dune build @runtest @fmt on source/ and cli/ (842 runtime tests, 99 reader tests, Cram; exit 0).
Result of the sweep: 50 of 53 queries byte-identical to main. The three differences are exactly the documented index("")/rindex("")/find_indices("") change; on main the last two loop forever, so this is a hang fix as much as a value change.
Findings (inline): three should-fix, two nits. No wrong results or data loss found.
Checked and clean
Literal_strand all callers: boundary math, needle longer than haystack,start > limit, overlapping matches infind_indices/rindex(same as the oldResemantics), empty needle per caller, byte-level UTF-8 behavior unchanged (café/münchen/日本語 fixture).assoc_merge/merge_left: first-duplicate-wins on the right in both paths, left duplicates kept in place, append order, threshold boundary at 31/32/33/34/65 keys,*throughdeep_merge,[@tail_mod_cons]stack safety on a 100k-key self-merge (mainexceeded a 90 s timeout).- Integer fast paths: promotion Int -> Int64 -> Big_int at every boundary; the old
json_of_integer_zalso normalized in-range Int64 back to Int, sotype/uniqueresults are unchanged. Decimal.of_lexeme_optfast path: 18-digit bound, sign,scale >= 0gate, trailing-zero trim,repr,-0,0.000,1e-7, 19-digit slow path; round trips identical.sort_by/unique/unique_by/min_by/max_by: Hashtbl vsList.memequality onnan,-0.0, differing-reprdecimals; stability; key evaluated once; error propagation.yield_range_list/last_*: step 0, negative steps, huge ranges underfirst/limit(no overflow now, correct values),laston empty; theassert falseis unreachable by construction.- Regex cache: invalid pattern memoized as
Nonewith the old passthrough behavior; compiledRe.resafely reused. Write.ml/Core.ml: float formatting identical acrossfloats.json, extreme magnitudes and negative zero; NaN/Infinity stillnull; colored-Coutput identical per constructor; thesummarize && not colorizebranch is reachable viaError.mlcontext rendering and is byte-identical.cli.mlcolor precedence-c>-C>NO_COLOR> isatty, incl. error messages.
| @@ -1,5 +1,7 @@ | |||
| # Unreleased | |||
|
|
|||
| - [BREAKING] **Color output now follows `jq`'s rules**: colored output is on only when stdout is a terminal (or `-C`/`--color` forces it), off when piped/redirected or when `NO_COLOR` is set (`-c`/`--no-color` still forces it off; `-C` wins over `NO_COLOR`) | |||
There was a problem hiding this comment.
should-fix. The PR carries two behavior changes but only this one has a CHANGES entry. The other, from "Search and split strings without compiling a regex per call": index("") returns null (was 0), and rindex("")/find_indices("") return null/[] where main looped forever. Release notes built from this file would miss a user-visible change and a hang fix. Suggest a second bullet in the same style.
There was a problem hiding this comment.
Fixed in ecae5a8: added a second [BREAKING] bullet under Unreleased for index("")/rindex("")/find_indices(""), noting that the last two previously looped forever.
| Above [merge_threshold] keys on the right we index it in a hashtable so | ||
| each left key resolves in O(1) instead of rescanning the right list; | ||
| below it, a linear scan is cheaper (no hashing/allocation overhead). *) | ||
| let merge_threshold = 32 |
There was a problem hiding this comment.
should-fix. The hash-indexed branch below this threshold check has no unit coverage: the merge tests in Test_runtime.ml use objects of two or three keys, so every test runs the linear path. Verified by hand that 31/32/33/34/65-key right-hand sides with duplicate keys on both sides match main, so it is correct today, but nothing protects it. Add a + and a * case with a right-hand object of exactly 32 and of more than 32 keys, including a duplicate key.
There was a problem hiding this comment.
Fixed in ecae5a8: object_merge_threshold_boundary builds right-hand objects of exactly 32 and 40 entries (a key shared with the left, a key duplicated on the right) and checks + and * against the expected merged order and values, so both the linear and the hash-indexed paths are covered. 847 runtime tests now.
| recompiled on every input value that flows through a `map`/pipe. Cache | ||
| the compiled regex (or the fact that it fails to compile) keyed by | ||
| pattern text, bounded so a pathological program can't grow it forever. *) | ||
| let regex_cache : (string, Re.re option) Hashtbl.t = Hashtbl.create 16 |
There was a problem hiding this comment.
should-fix. The 64-entry Hashtbl.reset eviction is never exercised: the new tests use two distinct patterns. Add a case that runs 65+ distinct literal patterns through sub/gsub and then reuses the first one, asserting the match still succeeds after the reset.
There was a problem hiding this comment.
Fixed in ecae5a8: regex_cache_eviction runs 65 distinct literal patterns through gsub in one query and then reuses the first one, forcing the 64-entry reset and a recompile.
|
|
||
| -C forces color on even when stdout is not a terminal | ||
|
|
||
| $ query-json -C '.first.name' mock.json | od -c | head -2 |
There was a problem hiding this comment.
nit. First use of od in this repo's Cram tests, and make test also runs on the windows-latest job. That job runs under Cygwin, whose coreutils include od, so this should be fine; worth confirming on the first CI run rather than assuming.
There was a problem hiding this comment.
Fixed in ecae5a8: the Cram case now uses grep -c "$(printf '\033')" and expects 1, so it no longer depends on od.
| for `indent` separate small Buffer.add_string calls per line: depths | ||
| repeat constantly across a large document, so this amortizes to a | ||
| handful of allocations total. *) | ||
| let indent_cache = ref [| "" |] |
There was a problem hiding this comment.
nit. This cache only grows. Harmless for the one-shot CLI, but Write.ml is also compiled into the JS library, where a long-lived process rendering arbitrary depths keeps the largest indent string array forever. Rendering a 10,000-deep array is byte-identical to main, so this is about memory, not output. A cap (for example, compute directly above depth 256) would remove the concern.
There was a problem hiding this comment.
Fixed in ecae5a8: indent_for caches depths up to 64 and builds deeper indents directly with String.make. A new writer test renders a 100-deep nesting and a 200-deep array is byte-identical to main with color neutralised.
… empty-needle change
A comparison against jq 1.7/1.8.1, jaq 3.1.1 and gojq 0.12.19 on 2026-09-19 showed query-json losing on several common operations because of work repeated per element, not because of the language design. This PR removes that repeated work in ten places, one commit each, with output kept byte-identical to
mainexcept for the two behavior changes listed at the end.mainsub,gsubsplit,contains,inside,index,rindex,find_indicesRe.compile (Re.str needle)per call for literal needlesLiteral_str)sort_by,min_by,max_by,unique,unique_byList.memscan for seen valuesList.stable_sort, hash setrange,limit,first,last,nthlastwalks instead of reversing+and*@copy;List.assocper key was quadratic when both sides are large[@tail_mod_cons]pass; hash index above 32 right-hand keys+and-Int + Intwent through the exactDecimal/ZpathZ.of_stringper number with a fractionint%.6gvia the primitiveif colorizeper tokenMeasurements.
hyperfine -N --warmup 2 --min-runs 10, release builds, Linux x86-64 (AMD EPYC 9755), OCaml 5.5.0~beta1,-c(no color) unless stated. Base is0f5ca93; each row was measured on this branch right after its commit landed. jq is 1.7 from Debian.[.[] | .name.english | gsub("a"; "4")] | length, 21 MB arraymap(.name.english | contains("an")) | length, 21 MB arraysort_by(.base.HP) | length, 21 MB array (97,080 objects)[range(1000000)] | lengthfirst(range(100000000))[limit(10; range(100000000))]. + {"z": 1} | length, 100k-key object. + . | length, 100k-key objectadd, array of 1M intslength, array of 1M floats., 97 MBbenchmarks/huge.json.[] | .name.english, 21 MB array (97,080 results).colored (-Con this branch), 97 MBOn the repository's own
benchmarks/bench.shcases (10 runs, warmup 3) the rendering-heavy rows move and the parse-dominated rows do not: big.16.4 -> 14.3 ms (jq 16.8), huge.2146 -> 1644 ms (jq 2767), hugekeys1208 -> 1273 ms (jq 1222, within run-to-run noise). Float parsing stays about 1.8x behind jq: the remaining cost is theDecimalrecord per number and cons-then-reverse array building, which is a representation change and out of scope here.Parity. For every query above and the edge cases in the commits,
cmpof-coutput against a0f5ca93build is identical, including decimal results (12.99 + 10is still22.99), integer overflow boundaries, mixed-typesort, duplicate keys in merges, and the empty-separator forms ofsplitand/.Behavior changes, each in its own commit so it can be dropped:
Colorize only on a TTY and write ANSI codes directly: color is on only when stdout is a terminal or-C/--coloris given, off when piped or whenNO_COLORis non-empty;-c/--no-colorstill wins. Documented in README and CHANGES; Cram covers both the piped default and-C.Search and split strings without compiling a regex per call:index(""),rindex("")andfind_indices("")returnnull/null/[]as jq does. Onmain,index("")returned0andrindex("")/find_indices("")looped forever.Regression coverage added: lazy
limit(0; ...),first(empty),nthout of range, 10k-deepdescend/paths; invalid-pattern passthrough and cache-key collision forsub/gsub; empty-needleindex/rindex/find_indices;from_entriesand+with duplicate keys; overflow boundaries for+/-; float formatting and nested empty containers in the writer; escapes, surrogates and a 100k-char string in the reader.Checks at
7eef191:dune build @runtest @fmt(842 runtime tests, 99 reader tests, CLI Cram) exit 0, release@allexit 0,git diff --checkclean; the same three checks were run on every commit in the range.Where to start reading:
source/Interpreter.mlfor the interpreter changes (Literal_str,Operators.assoc_merge,yield_range_list,sort_by),source/jotason/Write.mlandsource/Core.mlfor rendering,cli/cli.mlfor the color decision.Risk: two-way door. A merge revert restores every previous behavior, including color-on-pipe and the
index("")result; no data, published API or release artifact changes.