perf(cubesql): Migrate streaming to columnar JSON batches#11235
Conversation
The SQL API streaming path shipped rows across the JS->Rust native bridge as an array of row objects and rebuilt each `RecordBatch` cell-by-cell through per-cell Neon/NAPI downcasts (`JsValueObject::get`). This mirrors the row-oriented transport the `/load` path already replaced with a columnar JSON `Buffer`.
github-actions
Bot
added
driver:postgres
|
Claude finished @ovr's task in 3m 57s —— View job Code review — verdict: LGTM, small cleanups suggested (0 high, 1 medium inline, ~4 low)Full review (click to expand)
OverallNice, focused refactor. The end-to-end story is coherent:
Serialization contract between FindingsMedium — hardcoded sentinel ( Low — Low — Low — Low — TestsCoverage for the new DocsNothing user-facing — |
ovr
force-pushed
the
perf/sqlapi-streaming-columnar-v1
branch
from
10aa6e4 to
a3a5467
Compare
|
Claude finished @ovr's task in 4m 18s —— View job Claude PR Review — 0 high, 0 medium, 3 lowNice, focused perf win. The row→columnar consolidation on the streaming path mirrors the Details — checklist and findings
Findings (all low)
Also noted (no action needed)
|
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
|
|
||
| let mut response = JsonValueObject::new(data); | ||
| let mut response = LiteralRowsValueObject { | ||
| row_count: limit as usize, |
There was a problem hiding this comment.
Low — negative-limit behavior change. request.limit is Option<i32>; the old path used for _ in 0..limit, which for a negative limit (e.g. Some(-1)) produced an empty range. The new limit as usize wraps to usize::MAX, so LiteralRowsValueObject::len() returns ~2^64 and the per-column loop in build_column_custom_builder! will try to append that many NULLs (OOM / hang). The pre-clamp at scan.rs:510 only guards against > query_limit, not negatives.
Cheap defensive fix:
| row_count: limit as usize, | |
| let mut response = LiteralRowsValueObject { | |
| row_count: usize::try_from(limit).unwrap_or(0), | |
| }; |
Separately: the doc comment on LiteralRowsValueObject claims column() is "never actually invoked" because every field is a MemberField::Literal, but the old JsonValueObject explicitly errored if a MemberField::Member slipped through this path (rows were Value::Null, as_object() returned None). The new impl silently returns a NULL column instead. If this invariant ever breaks, the new behavior masks the bug; consider returning an error from column() to keep the loud-fail semantics.
| * (~11 ms on the same payload). On a real server it should be 3-6× slower, | ||
| * so avoiding the JsValue walk matters even more there. | ||
| */ | ||
| export function rowsToColumnarBuffer(rawData: any): Buffer { |
There was a problem hiding this comment.
Low — rowsToColumnar derives the schema from rows[0] only. Object.keys(rows[0]) at line 39 fixes the column set from row 0. If later rows in the stream buffer ever include additional keys, those columns are silently dropped before crossing the bridge. Postgres cursors return uniform row shape today, so this is likely safe — but this invariant now matters per streaming chunk (not just per full result), so worth a short doc note here or in rowsToColumnar making it explicit.
| }, | ||
| }; | ||
|
|
||
| query.response_format = |
There was a problem hiding this comment.
Low — unconditional format override. This clobbers whatever response_format the caller set on query. Given the deserialization target is now V1LoadResponse<V1LoadResultDataColumnar>, this is intentional and correct, but worth a one-line comment explaining "we force columnar because the response type below assumes it" so a future reader doesn't try to make the format caller-controlled.
1 participant
The SQL API streaming path shipped rows across the JS->Rust native bridge as an array of row objects and rebuilt each
RecordBatchcell-by-cell through per-cell Neon/NAPI downcasts (JsValueObject::get). This mirrors the row-oriented transport the/loadpath already replaced with a columnar JSONBuffer.Benchmark
Measured on a local Postgres (Docker) with a pre-loaded 10M-row
orderstable, streaming the ungrouped result through the SQL API topsql, so the data source is not the bottleneck (server-side seq-scan ≈ 0.3s). Server-side = CubeLoad Requestduration (the streaming pipeline itself). The dimension-width axis mirrors our k6 benchmark (status, user_id, product_id+dim1..dimN).10M rows
2M rows
~1.4–1.8× faster server-side across all widths and row counts, with the largest gain at 32 dimensions — per-cell NAPI cost scales with the number of cells, so wide rows benefit most. The 10M × 32-dim case drops from ~173s to ~99s (−43%).