perf(cubeorchestrator): Arrow-backed CubeStore results + columnar driver batches#11236
perf(cubeorchestrator): Arrow-backed CubeStore results + columnar driver batches#11236ovr wants to merge 2 commits into
Conversation
… wrapper Wrap the Arrow arrays a CubeStore result arrives in (`ColumnarArray::Arrow`) instead of materializing every cell into `DBResponsePrimitive` at parse time. Values are read straight from the Arrow buffers on demand via a borrowed `DBResponseValueRef` view, so: - parsing no longer allocates per string cell (`to_owned()` per value is gone); - the columnar transform shares identity columns as an `Arc` bump per chunk instead of a second per-cell `transform_value(cell.clone())` copy — only "time"-typed string columns still materialize; - JSON serialization and the SQL-API `FieldValue` path write string cells directly from the Arrow buffers (zero per-cell allocations), producing byte-identical output to the previous materialized path. Row-major consumers (compact/vanilla plans, `get_cubestore_result`) convert Arrow-backed columns once at plan build time via `to_cells()`, matching the old parse-time materialization cost. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Add `ColumnarResponse` to base-driver: an object-mode Transform that pivots
position-indexed array rows into `{ members, columns }` batches, plus a
`columnar` hint on `StreamOptions`. PostgresDriver honors the hint by reading
rows with `rowMode: 'array'` (no per-row object allocation) and piping them
through `ColumnarResponse`.
The SQL-API streaming path (`QueryCache.renewQuery` stream handler) requests
columnar batches; `QueryStream` remaps batch member aliases once per batch
instead of per row, and the native stream writer forwards batches straight to
Rust without row accumulation or the row->columnar transpose. Drivers without
columnar support keep emitting row objects through the old path.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Claude finished @ovr's task in 2m 41s —— View job Review: Arrow-backed CubeStore results + columnar driver batchesOverall: solid, well-structured change with strong internal docs and a matching test on the JS side. The Verdict: 0 high · 3 medium · 1 low — nothing blocking; all findings are inline. Details
Findings (posted inline):
Positive notes:
Not covered by tests (worth considering):
|
| // Columnar batch produced by a driver (base-driver `ColumnarResponse`, | ||
| // marker `COLUMNAR_RESPONSE_TYPE`): already `{ members, columns }`, so send it | ||
| // straight through without row accumulation or the row→columnar transpose. | ||
| if (row && row.$type === 'ColumnarResponse') { |
There was a problem hiding this comment.
Nit: this uses the string literal 'ColumnarResponse' while packages/cubejs-base-driver/src/ColumnarResponse.ts exports COLUMNAR_RESPONSE_TYPE as the canonical marker. Importing the constant here (from @cubejs-backend/base-driver) would keep the two ends in lockstep — if the sentinel ever changes, the native writer won't silently fall through to the row-accumulation path.
| DataType::Decimal128(_, scale) => TypedChunk::Decimal128 { | ||
| array: downcast_chunk!(array, Decimal128Array), | ||
| scale: (*scale).max(0) as u32, | ||
| }, | ||
| DataType::Decimal256(_, scale) => TypedChunk::Decimal256 { | ||
| array: downcast_chunk!(array, Decimal256Array), | ||
| scale: (*scale).max(0) as u32, |
There was a problem hiding this comment.
(*scale).max(0) as u32 silently drops Arrow's negative decimal scales. Arrow allows negative scale to mean "multiply mantissa by 10^|scale|" (e.g. Decimal128(p, -2) with mantissa 123 represents 12300). With this clamp, that value would render as "123" instead of "12300".
If CubeStore never emits negative-scale decimals in practice this is moot, but if the intent is "unsupported", it'd be safer to reject via ParseError::UnsupportedArrowType at TypedChunk::try_new time rather than silently truncate at read time.
| if (chunk && chunk.$type === COLUMNAR_RESPONSE_TYPE) { | ||
| const members = this.aliasNameToMember | ||
| ? chunk.members.map((alias) => this.aliasNameToMember[alias]) | ||
| : chunk.members; | ||
| const rowCount = chunk.columns[0] ? chunk.columns[0].length : 0; | ||
| if (this.counter + rowCount < this.readableHighWaterMark) { | ||
| this.counter += rowCount; | ||
| } else { | ||
| this.counter = 0; | ||
| this.debounce(); | ||
| } | ||
| callback(null, { $type: COLUMNAR_RESPONSE_TYPE, members, columns: chunk.columns }); |
There was a problem hiding this comment.
Two small concerns in the columnar branch:
-
Alias remap silently drops unknown aliases.
chunk.members.map((alias) => this.aliasNameToMember[alias])will produceundefinedentries when an alias is missing from the map. The row-path has the same latent issue (row[undefined] = …), but here theundefinedends up in the batch'smembersarray that Rust decodes asJsRawColumnarData. Worth double-checking that upstream guarantees a full map, or falling back to the raw alias when a lookup misses. -
The counter/backpressure arithmetic differs from the row path. Row path increments by 1 and compares
counter < HWM. Here it'scounter + rowCount < HWM— if a single batch'srowCount >= HWM, the counter is reset to 0 and the debounce timer fires on every subsequent batch. Not a correctness bug (only affects the idle-destroy timer), but the divergence is worth a comment or normalization.
| if (columnar) { | ||
| const members = fields.map((f) => f.name); | ||
| const columnarStream = new ColumnarResponse({ members, batchSize: highWaterMark }); | ||
| // `pipe()` does not forward errors — propagate manually so the consumer sees the | ||
| // failure and `release()` still runs during cleanup. | ||
| rowStream.once('error', (e) => columnarStream.destroy(e)); | ||
| rowStream.pipe(columnarStream); |
There was a problem hiding this comment.
Two things worth checking here:
batchSize: highWaterMark:highWaterMarkis the pg row-buffer size (default 8192 per the native writer). Using it as the columnar batch size means each object-modepushmoves ~8k rows worth of column data — fine, but if the intent was "one batch per pipe write" this is what you want; if it was "flush more often to smooth latency" you may want a distinctbatchSize.- Error propagation only wires
rowStream -> columnarStream.destroy(e). If the downstream consumer errors first,columnarStreamis destroyed butrowStreamkeeps producing untilpipe()'s internal cleanup fires. ConsidercolumnarStream.once('error', (e) => rowStream.destroy(e))too, so a downstream failure tears down the pg cursor promptly andrelease()isn't blocked waiting for the source to drain.
Stacked on #11235. Two follow-ups that extend the columnar pipeline on both ends:
1. Keep CubeStore Arrow results columnar under a wrapper (cubeorchestrator)
CubeStore results arrive as Arrow IPC, but were materialized into per-cell
DBResponsePrimitives at parse time (aStringallocation per string cell), and the columnar transform then copied every cell again viatransform_value(cell.clone())— even though the transform is identity for everything except"time"-typed string cells.ColumnarArrayis now an enum:Cells(materialized; JS raw input, legacy result sets, synthetic columns) orArrow(the Arrow arrays as they arrived, one chunk per record batch). Values are read straight from the Arrow buffers through a borrowedDBResponseValueRefview:Arcbump per chunk instead of a per-cell copy (only"time"-typed string columns still materialize);FieldValuepath write string cells directly from the Arrow buffers, with byte-identical output to the materialized path;getCubestoreResult) materialize once at plan build time viato_cells(), matching the old parse-time cost.2. Stream columnar batches from the driver (base-driver / postgres-driver)
The SQL-API streaming path still received row objects from drivers and transposed them to columnar in
cubejs-backend-native. NewColumnarResponseTransform in base-driver pivots position-indexed array rows into{ members, columns }batches, with acolumnarhint onStreamOptions:rowMode: 'array'(no per-row object allocation) and pipes them throughColumnarResponse;QueryStreamremaps member aliases once per batch instead of per row;Tests
cargo test -p cubeorchestrator— 85 passedyarn jest test/unit/ColumnarResponse.test.ts(base-driver) — 4 passed🤖 Generated with Claude Code