feat(cubestore): Introduce HttpQueryResultCompleted for zero-column r…

[ovr]

…esults

Write commands (CREATE TABLE/INSERT, queue/cache writes) return zero columns. When such a result was requested in Arrow format, building the Arrow IPC stream failed with "must either specify a row count or at least one column" because RecordBatch::try_new can't infer the row count from an empty column set.

Instead of forcing an empty Arrow stream onto the wire, extend the protocol with a dedicated result type. HttpQueryResult already carries the HttpQueryResultData union; add an HttpQueryResultCompleted marker to it. When an Arrow-format query produces no columns, the server now answers with QueryResultCompleted rather than an Arrow payload. QueryResultFormat (the request enum) is left untouched, so Completed is a server-chosen result shape that clients can never request.

The JS driver uses the legacy format, so the only Arrow consumer is the cubestore-ws-transport crate, which decodes the marker into the new ResultData::Completed / ResponseFormat::Completed.

[claude]

Claude finished @ovr's task in 53s —— View job

---

Code Review

Verdict: LGTM — clean protocol extension with solid test coverage. 0 high, 1 medium, 2 low.

Details

• Read PR diff and surrounding context
• Evaluate protocol/wire compatibility
• Check test coverage
• Check exhaustive match arms and error handling

Findings

Medium — wire-level back-compat between old/new peers

The PR body says "the only Arrow consumer is the cubestore-ws-transport crate, which decodes the marker into the new ResultData::Completed," which is true for the JS driver path, but the change is still a wire-format extension: an old cubestore-ws-transport client talking to a new cubestore server will now blow up on any zero-column Arrow query with HttpQueryResult.data variant is not HttpQueryResultArrow (since the old codec.rs unconditionally calls data_as_http_query_result_arrow()).

Before this PR, the same case errored on the server side ("must either specify a row count…"), so this is a change in where the error surfaces, not a regression. Still worth confirming that operators are expected to upgrade client + server together, or document the version requirement. If mixed-version operation matters, the new server could be kept opt-in (e.g. only emit Completed when a request-time capability flag is set).

Low — zero-column detection is structural

rust/cubestore/cubestore/src/http/mod.rs:609 uses query_result.schema().fields().is_empty() to decide between QueryResultCompleted and QueryResultArrow. This is fine for today's CREATE TABLE/INSERT/queue/cache writes, but it conflates "completion marker" with "any zero-column schema." If a future SQL feature ever produces a legitimately zero-column-but-non-empty result, it would be silently reclassified as Completed. Probably not worth changing now, but a comment on QueryResult/schema() noting that an empty schema is the canonical "no result set" signal would make the contract explicit.

Low — exhaustive log-format match arms duplicated

rust/cubestore/cubestore/src/http/mod.rs:332 and :413 repeat the same match cmd { HttpCommand::Query… HttpCommand::QueryResultCompleted … } block just to format a debug string. Adding a new variant requires touching both sites (this PR did). A Debug/helper method on HttpCommand would localize the change. Pre-existing pattern; flagging since the PR doubles down on it.

Praise

• The protocol extension is minimal: a marker table on the existing HttpQueryResultData union rather than a new top-level command type. Avoids churn in HttpMessage::command_type().
• Leaving QueryResultFormat (the request enum) alone and making Completed a server-chosen response shape is the right call — clients shouldn't be able to request "completion."
• decode_frame switches from ok_or_else to a match on data_type() with an explicit other => arm — forward-compatible for future union variants.
• tests/wire_roundtrip.rs and the new arrow_response_format_zero_columns_completed test in http/mod.rs cover both decode and end-to-end encode-via-process_command paths.
• ResponseFormat::Completed / ResultData::Completed are documented as decoded-only descriptors that clients can never request — matches the design.

• `feat/cubestore-completed-result`

[claude]

Claude finished @ovr's task in 37s —— View job

---

Code Review (updated for 835d5b5)

Verdict: LGTM — the new commit is a clean test-only refactor with no behavioral change. Prior findings unchanged.

Details

• Read PR diff and surrounding context
• Re-review latest commit (835d5b5 refactor(cubestore): share StubService across http arrow tests)
• Evaluate protocol/wire compatibility
• Check test coverage
• Check exhaustive match arms and error handling

Latest commit (835d5b5)

Lifts the inline StubService impl out of arrow_response_format_round_trip and arrow_response_format_zero_columns_completed into a single module-level stub in rust/cubestore/cubestore/src/http/mod.rs. Pure deduplication — same trait impls, same unimplemented!("Mock") arms, same QueryResult::Frame(self.0.clone()) return. Both tests now construct StubService(df) directly. Net −36 lines. No new issues.

Findings (carried over from prior review)

Medium — wire-level back-compat between old/new peers

The PR body says "the only Arrow consumer is the cubestore-ws-transport crate, which decodes the marker into the new ResultData::Completed," which is true for the JS driver path, but the change is still a wire-format extension: an old cubestore-ws-transport client talking to a new cubestore server will now blow up on any zero-column Arrow query with HttpQueryResult.data variant is not HttpQueryResultArrow (since the old codec.rs unconditionally calls data_as_http_query_result_arrow()).

Before this PR, the same case errored on the server side ("must either specify a row count…"), so this is a change in where the error surfaces, not a regression. Still worth confirming that operators are expected to upgrade client + server together, or document the version requirement. If mixed-version operation matters, the new server could be kept opt-in (e.g. only emit Completed when a request-time capability flag is set).

Low — zero-column detection is structural

rust/cubestore/cubestore/src/http/mod.rs:609 uses query_result.schema().fields().is_empty() to decide between QueryResultCompleted and QueryResultArrow. This is fine for today's CREATE TABLE/INSERT/queue/cache writes, but it conflates "completion marker" with "any zero-column schema." If a future SQL feature ever produces a legitimately zero-column-but-non-empty result, it would be silently reclassified as Completed. Probably not worth changing now, but a comment on QueryResult/schema() noting that an empty schema is the canonical "no result set" signal would make the contract explicit.

Low — exhaustive log-format match arms duplicated

rust/cubestore/cubestore/src/http/mod.rs:332 and :413 repeat the same match cmd { HttpCommand::Query… HttpCommand::QueryResultCompleted … } block just to format a debug string. Adding a new variant requires touching both sites (this PR did). A Debug/helper method on HttpCommand would localize the change. Pre-existing pattern; flagging since the PR doubles down on it.

Praise

• The new shared StubService is the right call — the inline duplicates were already a copy-paste-and-edit hazard.
• Protocol extension is minimal: a marker table on the existing HttpQueryResultData union rather than a new top-level command type. Avoids churn in HttpMessage::command_type().
• Leaving QueryResultFormat (the request enum) alone and making Completed a server-chosen response shape is the right call — clients shouldn't be able to request "completion."
• decode_frame switches from ok_or_else to a match on data_type() with an explicit other => arm — forward-compatible for future union variants.
• tests/wire_roundtrip.rs and arrow_response_format_zero_columns_completed cover both decode and end-to-end encode-via-process_command paths.

• `feat/cubestore-completed-result`

[claude]

Claude finished @ovr's task in 42s —— View job

---

Code Review (updated for 8a27cf8)

Verdict: LGTM — small, correct CLI follow-up. Prior findings unchanged. 0 high, 1 medium, 2 low.

Details

• Read PR diff and surrounding context
• Re-review latest commit (8a27cf8 fix(cubestore-cli): handle Completed result variant in table renderer)
• Evaluate protocol/wire compatibility
• Check test coverage
• Check exhaustive match arms and error handling

Latest commit (8a27cf8)

Adds the missing ResultData::Completed arm to the match in render_table (rust/cube/cubestore-cli/src/format.rs:17-22). Since result.get_columns() returns Vec::new() for Completed (cubestore-ws-transport/src/result.rs:85), the early-return guard at L13-15 already handles it in practice — the new arm is structurally required by the now-three-variant enum and acts as a defensive return Ok(None). Correct and minimal. No test added, which is fine: the guard already covers this, and adding one would only exercise dead code.

Minor nit (optional, not blocking): the // Completed carries no columns, so the guard above already returned. comment is the kind of "explain the obvious" line CLAUDE.md discourages, but it does flag the structural-vs-reachable distinction, so reasonable to keep.

Findings (carried over from prior reviews)

Medium — wire-level back-compat between old/new peers

The PR body says "the only Arrow consumer is the cubestore-ws-transport crate, which decodes the marker into the new ResultData::Completed," which is true for the JS driver path, but the change is still a wire-format extension: an old cubestore-ws-transport client talking to a new cubestore server will now blow up on any zero-column Arrow query with HttpQueryResult.data variant is not HttpQueryResultArrow (since the old codec.rs unconditionally calls data_as_http_query_result_arrow()).

Before this PR, the same case errored on the server side ("must either specify a row count…"), so this is a change in where the error surfaces, not a regression. Still worth confirming operators upgrade client + server together, or document the version requirement.

Low — zero-column detection is structural

rust/cubestore/cubestore/src/http/mod.rs:609 uses query_result.schema().fields().is_empty() to decide between QueryResultCompleted and QueryResultArrow. Fine today, but conflates "completion marker" with "any zero-column schema." A future SQL feature producing a legitimately zero-column-but-non-empty result would be silently reclassified. A doc comment on the contract would make this explicit.

Low — exhaustive log-format match arms duplicated

rust/cubestore/cubestore/src/http/mod.rs:332 and :413 repeat the same match cmd { … } block just to format a debug string. Adding a variant requires touching both. A Debug/helper on HttpCommand would localize it. Pre-existing.

Praise

• The CLI fix is the right shape — relies on the existing get_columns() empty-guard rather than re-deriving "is completion" from the variant.
• Protocol extension stays minimal: a marker on the existing HttpQueryResultData union, no churn in HttpMessage::command_type().
• Leaving QueryResultFormat (request) untouched and making Completed server-chosen is the right call.
• decode_frame uses a match on data_type() with an other => arm — forward-compatible.
• tests/wire_roundtrip.rs and arrow_response_format_zero_columns_completed cover decode and end-to-end paths.

feat/cubestore-completed-result •

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 78.82%. Comparing base (adc8d26) to head (8a27cf8).
⚠️ Report is 6 commits behind head on master.

Additional details and impacted files

@@             Coverage Diff             @@
##           master   #11019       +/-   ##
===========================================
+ Coverage   58.57%   78.82%   +20.24%     
===========================================
  Files         216      471      +255     
  Lines       17181    94442    +77261     
  Branches     3502     3502               
===========================================
+ Hits        10064    74441    +64377     
- Misses       6609    19493    +12884     
  Partials      508      508               

Flag	Coverage Δ
cube-backend	58.57% <ø> (ø)
cubesql	83.32% <ø> (?)

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.