Skip to content

fix(cubesql): Handle array-typed columns in streaming SQL mode#11149

Open
MazterQyou wants to merge 1 commit into
masterfrom
cubesql/streaming-mode-array-fix
Open

fix(cubesql): Handle array-typed columns in streaming SQL mode#11149
MazterQyou wants to merge 1 commit into
masterfrom
cubesql/streaming-mode-array-fix

Conversation

@MazterQyou

Copy link
Copy Markdown
Member

Check List

  • Tests have been run in packages where changes have been made if available
  • Linter has been run for changed code
  • Tests for the changes have been added if not covered yet
  • Docs have been added / updated if required

Description of Changes Made

This PR fixes handling array-typed columns in streaming mode. Related test is included.

This behavior is not documented and unsupported, so it should not be relied upon. The fix is being landed to bring streaming and non-streaming modes in line.

Signed-off-by: Alex Qyoun-ae <4062971+MazterQyou@users.noreply.github.com>
@MazterQyou MazterQyou requested review from a team as code owners June 23, 2026 18:16
@github-actions github-actions Bot added driver:postgres Issues relating to the Postgres driver rust Pull requests that update Rust code javascript Pull requests that update Javascript code data source driver labels Jun 23, 2026
@claude

claude Bot commented Jun 23, 2026

Copy link
Copy Markdown
Contributor

Claude finished @MazterQyou's task in 1m 34s —— View job


Claude PR Review

Reviewed the four-file change. Overall this is a focused, well-scoped fix that brings streaming-mode behavior in line with the non-streaming path (both now JSON-stringify non-scalar values) and is backed by a unit test in scan.rs plus an integration test in the Postgres driver.

Verdict: LGTM — 0 high, 1 medium, 2 low.

Details

Findings

Medium — PostgresDriver.ts may have silently fixed a latent enum-mapping bug

In packages/cubejs-postgres-driver/src/PostgresDriver.ts:304-316, adding AS typname aliases the CASE expression so current.typname is now populated. Before this change the result column for the CASE branch had no explicit name, so depending on driver behavior current.typname was likely undefined for every row — meaning the existing enum (typcategory = 'E') mapping to varchar may not have been working at all, and userDefinedTypes was being populated with { [oid]: undefined }. Worth confirming and calling out in the PR description, since this silently fixes more than just arrays.

Low — stream.rs: JsObject branch is a broad catch-all

packages/cubejs-backend-native/src/stream.rs:322-326 will JSON.stringify any JsObject not already matched. The branch ordering (after JsDate) is correct so dates still error rather than serializing, but Buffers / typed arrays / Maps will silently produce {"type":"Buffer",...} / {} / {} rather than a clear error. For the documented streaming-mode use case this is fine, but a brief comment noting the intent ("treat any unknown object as JSON for parity with the non-streaming path") would help the next reader.

Low — stream.rs: js_value_to_json_string re-resolves JSON.stringify on every call

packages/cubejs-backend-native/src/stream.rs:264-283 looks up JSON and JSON.stringify on the global for every array/object cell. For chunks with many array columns this is a non-trivial overhead. Caching the JsFunction handle (e.g. on JsValueObject or a per-chunk scope) would avoid the repeated global lookups. Not a blocker; mention in case throughput matters here.

Notes / Praise

  • Replacing the .unwrap() at the old transform_response call site with a proper throw_error (stream.rs:349-353) is a nice correctness improvement on its own — previously a malformed chunk would crash the Node process.
  • The scan.rs unit test (convert_transport_response_serializes_non_scalar_values_to_json_strings) is well-targeted: covers both array and object shapes, plus the null case.
  • Postgres integration test exercises both text[] and int[] — good coverage of the array category.
  • Conventional commit message and CHANGELOG-friendly description.
• [branch](https://github.com/cube-js/cube/tree/cubesql/streaming-mode-array-fix)

@codecov

codecov Bot commented Jun 23, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 95.45455% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 79.20%. Comparing base (e317395) to head (a04208e).
⚠️ Report is 3 commits behind head on master.

Files with missing lines Patch % Lines
rust/cubesql/cubesql/src/compile/engine/df/scan.rs 95.45% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master   #11149      +/-   ##
==========================================
- Coverage   83.68%   79.20%   -4.49%     
==========================================
  Files         257      474     +217     
  Lines       79384    96934   +17550     
  Branches        0     3563    +3563     
==========================================
+ Hits        66434    76774   +10340     
- Misses      12950    19642    +6692     
- Partials        0      518     +518     
Flag Coverage Δ
cube-backend 58.80% <ø> (?)
cubesql 83.69% <95.45%> (+0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

data source driver driver:postgres Issues relating to the Postgres driver javascript Pull requests that update Javascript code rust Pull requests that update Rust code

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants