Repository navigation
Conversation
davesnx
left a comment
There was a problem hiding this comment.
Independent review of fea7a04 against main (0f5ca93). Method: hand trace of Execution.compile/fold/stream_cut/leading_key and of Runtime.ml against the pre-PR Interpreter.ml; 70 hand-written differential CLI queries (identity, indices incl. negative and out of range, iteration incl. empty and null, pipes, commas with error order, array/map with mid-failure, select, every binary operator incl. division by zero, multi-output operands falling back, duplicate root keys, -d), all 70 byte-identical in stdout, stderr and exit code to a 0f5ca93 build; 8 controlled-pipe event-input scenarios and 9 selective-reader scenarios (duplicate keys, malformed escapes/surrogates/exponents in discarded data, 500-deep discarded containers, trailing junk, unterminated input, fd lifecycle checked with strace) all matching the documented contract; @runtest @fmt exit 0 (889 source tests, 92 reader tests, Cram incl. source.t); bench_execution.exe --iterations 1000 --warmup 1 --samples 3 passes its equality checks.
Findings (inline): two should-fix, one nit. Nothing indicates wrong query results or data loss.
Not inline, worth weighing: after a query error under --stream-output, the process keeps reading to preserve input-error precedence, so a stalled or hostile producer holds the process and the pipe open indefinitely. docs/COMPILED_QUERY_ENGINE.md ("Errors and source lifetime") discloses and accepts this for the first slice; confirmed empirically. It is easy to hit from ordinary scripts, so a bounded drain or a documented timeout recommendation may be worth a follow-up.
Checked and clean
- Fallback is always whole-expression; the
Operationarm rejects every non-single-result operand shape. stream_cut/leading_keymatch the design doc's barrier table for.rows[]|.id,.rows[]|select(..),[.rows[]],.rows|map(.id)|.[],.rows[-1],.rows[], .rows[].Runtime.mlbodies are byte-identical extractions of the old interpreter logic, including first-duplicate-key lookup and failing on non-list index.- Callback exceptions escape both
Execution.foldandInterpreter.foldwith identity intact; halts map correctly. Core.run_input/run_input_iter: input error wins over query parse error, debug AST only after valid input, one sharedload_input.js/Js.mlroutes throughCore.run_input (String ..), type-compatible; REPL untouched.--debugwith--stream-outputkeeps validation-first order;| head -1exits with 141 and no trace.
| @@ -1,3 +1,7 @@ | |||
| (executable | |||
| (name stream_probe) | |||
There was a problem hiding this comment.
should-fix. stream_probe uses Unix.create_process and Unix.select on anonymous pipes and is pulled into @runtest through source.t on every CI leg, including windows-latest, where Unix.select on pipes is not supported. The sibling pipe test in source/jotason/test/Test_read.ml:1101 guards with if not Sys.win32; this stanza has no (enabled_if (<> %{os_type} Win32)), and the Cram dependency will also need the same condition. The PR body's "the parser pipe test skips its Unix-only path on Windows" covers Test_read.ml only.
There was a problem hiding this comment.
Fixed in 389b6b1: stream_probe gets (enabled_if (<> %{os_type} "Win32")), and the Cram stanza is split with applies_to so basic, errors, functions and null run everywhere while source carries the same guard and the probe dependency.
| in | ||
| Core.run ~debug ~colorize ~verbose ~raw:raw_output ~summarize:false | ||
| query json | ||
| if stream_output then ( |
There was a problem hiding this comment.
should-fix. This is the first mode where valid JSON lines and error prose share stdout. Errors go through print_error_message, which is print_endline (stdout), and the exit code stays 0:
$ query-json -c --stream-output '.[] | .id' mixederr.json # [{"id":1},{"id":2},"bad"]
1
2
error[type_mismatch]: Cannot index string with string "id"
in: "bad"
exit=0 (stderr empty)
In atomic mode a failing query prints no result bytes, so consumers never saw a mix. A JSON-lines consumer of --stream-output now gets corrupted input with no exit-code signal. README says "the error starts on a new line" but not that it lands on stdout. Suggest writing the terminal error to stderr and exiting non-zero for this mode (or everywhere), or at minimum documenting that stdout can end in non-JSON text after partial output.
There was a problem hiding this comment.
Fixed in 389b6b1: in --stream-output mode the terminal error now goes to stderr and the process exits 1, so stdout only ever carries results; atomic mode is unchanged (same text on stdout, exit 0), which is a separate decision. README states the new contract and the Cram cases assert it. Repro: stdout 1,2; stderr carries the type_mismatch error; exit 1.
| |> Runtime_events.Timestamp.to_int64 |> Int64.to_float | ||
| |> fun ns -> ns /. 1e9 | ||
|
|
||
| let deadline () = now () +. 5.0 |
There was a problem hiding this comment.
nit. A fixed 5 s deadline with three probes back to back is a flakiness risk on a loaded CI runner. Consider a larger bound or one retry on the specific timeout failure.
There was a problem hiding this comment.
Fixed in 389b6b1: deadline raised to 30 s with the failure text updated; it only bounds the failure case.
The interpreter evaluated every query against a complete
Json.tand returned a complete result list, so a query over a large array could not print anything before EOF, retained the whole input, and allocated a final list of all results. This adds an execution layer that removes those three costs for a conservative query subset while leaving every existing call path and its output unchanged.What changes
Execution.preparecompiles paths, indices, iteration, pipes, commas, array construction,map,select,emptyand binary operators into a plan; anything else falls back to the interpreter for the whole query. Compiled and interpreted execution share one privateRuntimemodule for literals, operators, truthiness, key access and error text, so results and diagnostics cannot drift.Execution.foldandCore.run_iterdeliver each complete result to a callback without building the final list.Core.runkeeps its atomic string result.Json.Inputreads String, File, Channel and Value sources with one validating reader. For a plan that starts with one unconditional root key, only that member is retained and the rest of the document is validated in discard mode. Duplicate-key, missing-key and non-object-root behavior stay exactly as before.Core.run_input_iterexecutes each complete child as soon as it is parsed, before EOF; other queries validate the whole input first. Query failures stop output but still drain and validate the input, so a later input error keeps precedence. There is no separate delivery option, and the compiled/interpreted choice is visible only to tests throughExecution.For_test.Core.run_input_itercall with a line-per-result sink. The default holds every line and writes them at once after the query succeeds, so stdout stays all-or-nothing;--stream-outputflushes each line as it is produced and enables output before EOF. A query with no results prints one empty line in both modes. The JS library uses the validated atomicCore.run_input. The REPL is unchanged.source/Execution.mliandsource/Core.mli;benchmarks/README.mddescribes the measurement procedure.Measured on this tree's release build, Linux x86-64 (AMD EPYC 9755), OCaml 5.5.0~beta1,
-c, versus a0f5ca93build, jq 1.7, jaq 3.1.1, gojq 0.12.19. Fixture: a 21.4 MB array of 97,080 objects.Time to first output line when the producer writes 20 MB, stalls 3 s, then writes the rest (
.[] | .id, 3 runs each):query-json --stream-outputquery-jsondefault, and0f5ca93--stream --unbufferedPeak RSS,
.[] | .name.english(median of 3,/usr/bin/time %M):--stream-output8,240 KB; default and0f5ca93200,740 KB; jq 1.7 249,856 KB; jaq 307,520 KB; gojq 172,032 KB.Wall time, same query, file argument (hyperfine, 10 runs, warmup 3):
--stream-output366 ± 20 ms; default 707 ± 42 ms;0f5ca93709 ± 12 ms; jq 1.7 479 ± 13 ms; jaq 305 ± 3 ms; jq 1.7--streamwith thefromstream(1|truncate_stream(inputs))idiom 4.52 ± 0.09 s. jq's streaming mode matches the latency and memory but costs 12x in time; this path streams at whole-input speed with no query rewrite.On the repository's
benchmarks/bench.shcases without--stream-output, this tree is within noise of0f5ca93on every case (for example big.16.9 vs 16.9 ms, huge.2.162 vs 2.161 s), so default behavior costs nothing.In-process benchmark (
benchmarks/bench_execution.ml, results checked for exact equality againstInterpreter.executebefore timing, five paired samples): fold of 16,384 outputs 262,470 vs 774,810 ns/op and 2.23 vs 3.02 MB/op; early root-member selection on an 87 KB input 18.2 % less time and 60.8 % less allocation; first rendered callback on a wrapped 575 KB input 18.8 µs withWhen_readyvs 4.57 ms withAfter_validation.Semantics kept, checked by differential tests against the interpreter (
source/test/Test_execution.ml) and Cram tests with a controlled pipe (cli/test/source.t,cli/test/stream_probe.ml): result order, comma and pipe evaluation order, first-duplicate-key lookup, negative indices, truthiness,selectyielding per truthy result, input errors winning over query parse errors, debug AST only after valid input, callback exceptions escaping with identity and backtrace, and 100,000 ordered outputs.Review notes. With
--stream-outputa late query error can leave an already emitted prefix; this is why streaming is opt-in and documented in the README. After a query error the input is drained to preserve error precedence, so an open producer can hold the process. Windows and macOS were not exercised locally; the parser pipe test skips its Unix-only path on Windows.Checks at
fea7a04: releasedune build @allexit 0;dune build @runtest @fmtexit 0 (889 source tests, 92 reader tests, CLI Cram incl. the streaming probes);git diff --checkclean.Risk: two-way door. A merge revert restores the interpreter-only path, the previous
CoreAPI and the CLI without--stream-output; nothing outside the repository changes. This branch editscli/cli.ml,source/Core.ml,source/Interpreter.ml,source/jotason/Read.mllandjs/Js.ml, which #71 and #72 also edit; whichever of the three merges last needs a rebase.