Conversation
…ws-transport The `--features integration-cubestore` harness talked to cubestored as a MySQL client via `mysql_async` — the only use of it in `rust/cube`, and not the transport Cube actually uses. Production reaches CubeStore over WebSocket + FlatBuffers; `cubestore-ws-transport` is the Rust port of that driver and is already a member of the same workspace. `mysql_async` 0.34.2 was the sole consumer of `lru` 0.12.5 (GHSA-rhfx-m35p-ff5j), so this closes the alert without the breaking 0.37 bump. Dropping it removed 38 packages from `rust/cube/Cargo.lock` (incl. `mysql_common` and the bindgen/clang-sys/zstd chain) and added none — ws-transport's graph was already present via `cubestore-cli`. Results now arrive as Arrow record batches rather than MySQL rows, so `mysql_value_to_string` is replaced by `cubestore_result_rows`, rendering through `ArrayFormatter`. Columns come from `QueryResult::get_columns()`, which also fixes an empty result rendering as `(empty result)` — they were previously derived from the first row. 35 `pre_aggregations/*_cubestore_result.snap` snapshots changed; the 15 under `tests/integration/cubestore/` did not. 61 cell changes, no row- or column-count changes, in two classes: | old | new | arrow type | why | | --- | --- | --- | --- | | `2025-01-01T00:00:00.000Z` | `2025-01-01 00:00:00` | `Timestamp(Microsecond, None)` | cubestored's MySQL layer stringified to ISO-8601; the new form is what the Postgres side of these tests already produces | | `1000` | `1000.00000` | `Decimal128(28, 5)` | MySQL trimmed trailing zeros; arrow renders the declared scale (also hit at scale 9 and 13) | `with_timestamp_format` is load-bearing rather than cosmetic: without it the switch_rolling CubeStore-vs-Postgres equality assertions fail. `cargo test -p cubesqlplanner --features integration-cubestore`: 1412 passed, 0 failed, 10 ignored. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Claude finished @ovr's task in 5m 27s —— View job Reviewed — no blockers. 2 medium, 2 low, posted inline.Review detail
Verified
Findings
Review threadsNo prior threads — |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| /// Stringifies a CubeStore result into `(columns, rows)` for | ||
| /// `format_rows_table`. The transport always asks for Arrow, but an older | ||
| /// server can still answer with the legacy row envelope, so both are handled. | ||
| #[cfg(feature = "integration-cubestore")] | ||
| fn mysql_value_to_string(value: Option<&mysql_async::Value>) -> String { | ||
| use mysql_async::Value; | ||
| match value { | ||
| None | Some(Value::NULL) => "NULL".to_string(), | ||
| Some(Value::Bytes(bytes)) => String::from_utf8_lossy(bytes).into_owned(), | ||
| Some(Value::Int(v)) => v.to_string(), | ||
| Some(Value::UInt(v)) => v.to_string(), | ||
| Some(Value::Float(v)) => v.to_string(), | ||
| Some(Value::Double(v)) => v.to_string(), | ||
| Some(Value::Date(y, m, d, h, min, s, micros)) => { | ||
| if *micros == 0 { | ||
| format!("{:04}-{:02}-{:02} {:02}:{:02}:{:02}", y, m, d, h, min, s) | ||
| } else { | ||
| format!( | ||
| "{:04}-{:02}-{:02} {:02}:{:02}:{:02}.{:06}", | ||
| y, m, d, h, min, s, micros | ||
| ) | ||
| } | ||
| } | ||
| Some(other) => format!("{:?}", other), | ||
| } | ||
| fn cubestore_result_rows( | ||
| result: &cubestore_ws_transport::QueryResult, | ||
| ) -> (Vec<String>, Vec<Vec<String>>) { | ||
| use cubestore_ws_transport::arrow::util::display::{ArrayFormatter, FormatOptions}; | ||
| use cubestore_ws_transport::ResultData; | ||
|
|
||
| // Arrow's defaults render NULL as an empty string and timestamps as | ||
| // RFC3339 (`2024-04-01T00:00:00`). Both are overridden to match the | ||
| // Postgres rendering, because the rolling-window tests assert the | ||
| // CubeStore and Postgres results are equal. chrono's `%.f` emits | ||
| // nothing when the fraction is zero. | ||
| let fmt_options = FormatOptions::default() | ||
| .with_null("NULL") | ||
| .with_display_error(true) | ||
| .with_timestamp_format(Some("%Y-%m-%d %H:%M:%S%.f")); | ||
|
|
||
| // Column names come from the payload, so they survive an empty result. | ||
| let columns = result.get_columns(); | ||
| let rows = match &result.data { | ||
| ResultData::Completed => Vec::new(), | ||
| ResultData::Legacy { rows, .. } => rows | ||
| .iter() | ||
| .map(|row| { | ||
| row.iter() | ||
| .map(|cell| cell.clone().unwrap_or_else(|| "NULL".to_string())) | ||
| .collect() | ||
| }) | ||
| .collect(), |
There was a problem hiding this comment.
The Legacy branch can't produce a passing snapshot, so as a fallback it only obscures the failure. The 35 snapshots in this PR were regenerated from the Arrow branch's rendering — 2025-01-01 00:00:00 and 1000.00000. cubestored's legacy envelope stringifies the same cells as 2025-01-01T00:00:00.000Z and 1000, i.e. exactly the old values. So a server that answers Legacy (older binary via @cubejs-backend/cubestore's downloaded/latest/bin/cubestored, which cubestored_bin() at cubestore_service.rs:61 will happily pick up) doesn't degrade gracefully here — it fails 35 tests with 61 unexplained cell diffs.
The PR body says you confirmed the server answers Arrow, but that check is manual and nothing in the harness pins it. Since get_format() is right there, make the assumption enforceable:
assert_eq!(
result.get_format(),
ResponseFormat::Arrow,
"cubestored answered {} — snapshots are recorded against the arrow rendering",
result.get_format()
);and drop the Legacy arm (or leave it unreachable!). That turns a wall of diffs into one line naming the cause, and removes an untested branch from the harness.
| let client = Client::connect(ClientConfig::new(url)) | ||
| .await | ||
| .expect("Failed to connect to cubestored"); |
There was a problem hiding this comment.
ClientConfig::new leaves max_connect_retries: 20 with the actor's linear backoff of (attempt + 1) * 1000 ms (actor.rs:227-229), so a query in flight when cubestored dies sits in attempt_reconnect for 1+2+…+20 s = 210 s before run() gives up and the query resolves to TransportError::Disconnected.
That's a reachable path, not a hypothetical: cubestored_bin() at :43-47 documents that a debug cubestored stack-overflows on deep multi-stage plans, and CUBESTORED_ALLOW_DEBUG=1 opts into exactly that. Under mysql_async the crash surfaced immediately as a connection error naming the failing SQL; now each in-flight test burns 3.5 minutes first and then panics with Disconnected, which points at nothing.
The fields are public — a one-off config for the harness keeps the fail-fast behaviour:
let mut cfg = ClientConfig::new(url);
// The server is a local child process: if it's gone it isn't coming back,
// and the default 20-attempt backoff would stall the test for ~3.5 minutes.
cfg.max_connect_retries = 1;
let client = Client::connect(cfg)
| // Arrow's defaults render NULL as an empty string and timestamps as | ||
| // RFC3339 (`2024-04-01T00:00:00`). Both are overridden to match the | ||
| // Postgres rendering, because the rolling-window tests assert the | ||
| // CubeStore and Postgres results are equal. chrono's `%.f` emits | ||
| // nothing when the fraction is zero. | ||
| let fmt_options = FormatOptions::default() | ||
| .with_null("NULL") | ||
| .with_display_error(true) | ||
| .with_timestamp_format(Some("%Y-%m-%d %H:%M:%S%.f")); |
There was a problem hiding this comment.
with_display_error(true) is the wrong direction for a snapshot harness. In that mode arrow writes the formatting error into the output instead of propagating it, so a column arrow can't render produces a cell containing the error text — which format_rows_table pads into the table and cargo insta accept then blesses as the expected result. false makes ValueFormatter::to_string() panic instead, which is what you want from a test. It also looks like arrow's default for safe, in which case the call is a no-op as written.
Separately, the comment above it is 5 lines for one decision. The load-bearing sentence is the with_null/with_timestamp_format rationale; the %.f aside and the RFC3339 example are recoverable from the format string:
// Overridden to match the Postgres rendering, because the rolling-window
// tests assert the CubeStore and Postgres results are equal.
| @@ -1,7 +1,8 @@ | |||
| --- | |||
| source: cubesqlplanner/cubesqlplanner/src/tests/integration/pre_aggregations/multi_fact.rs | |||
| assertion_line: 40 | |||
There was a problem hiding this comment.
All 35 regenerated snapshots gained an assertion_line: header — that's the entire content of the +2/-1 and part of every other diff here. Before this PR only 3 of the 57 snapshots in this directory carried it; after, 38 do, and the 5 *_cubestore_result.snap files the run didn't touch still don't.
It records the source line of the assert_snapshot! call, so every one of these files now produces a spurious diff the next time a test above it in multi_fact.rs / sql_generation.rs grows or shrinks by a line. insta ignores it when comparing, so it buys nothing.
cargo insta test --force-update-snapshots --accept rewrites them without it, or just strip the added lines.
…sults
Drop the `with_timestamp_format` override and render timestamps as arrow
produces them. `with_display_error(true)` goes too — `safe: true` is already
the `FormatOptions` default, so it was a no-op. Only the null override is
kept, since arrow renders a null as an empty string, which is
indistinguishable from an empty string value.
The rolling-window tests compare CubeStore against Postgres, and `normalize`
reconciled the two by matching on the `Z` suffix that cubestored's MySQL
layer used to emit. Arrow's RFC3339 has no `Z`, so the matcher is now
positional on the `YYYY-MM-DDTHH:MM:SS` shape — a plain `contains('T')` would
have mangled the `YTD` calc-group value.
18 `pre_aggregations/*_cubestore_result.snap` fixtures updated, every change a
space -> `T` on a timestamp, no row- or column-count changes. The 15 snapshots
under `tests/integration/cubestore/` are unaffected, which also confirms `YTD`
survives the new matcher.
`cargo test -p cubesqlplanner --features integration-cubestore`: 1412 passed,
0 failed, 10 ignored.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This branch has not been deployed
1 participant
Check List
Description of Changes Made
The
--features integration-cubestoreharness talked tocubestoredas a MySQL client viamysql_async— the only use of it anywhere inrust/cube, and not the transport Cube actually uses (production goes over WebSocket + FlatBuffers); it now usescubestore-ws-transport, the Rust port of that driver, already a member of the same workspace. This also closes thelru0.12.5 advisory (GHSA-rhfx-m35p-ff5j) without the breakingmysql_async0.37 bump, sincemysql_async0.34.2 was its sole consumer: dropping it removed 38 packages fromrust/cube/Cargo.lock(incl.mysql_commonand the bindgen/clang-sys/zstd chain) and added none, as ws-transport's graph was already present viacubestore-cli. Results now arrive as Arrow record batches rather than MySQL rows, somysql_value_to_stringis replaced bycubestore_result_rowsrendering throughArrayFormatter, with columns fromQueryResult::get_columns()— which also fixes an empty result rendering as(empty result), since columns were previously derived from the first row.cargo test -p cubesqlplanner --features integration-cubestore: 1412 passed, 0 failed, 10 ignored, against a releasecubestoredbuilt from this tree. Clippy clean;cubestore-ws-transport's own tests still green.Rendering
Arrow's own rendering is kept as-is, including RFC3339 timestamps. The single
FormatOptionsoverride iswith_null("NULL"), because arrow renders a null as an empty string, which is indistinguishable from an empty string value in the rendered table.The rolling-window tests compare the CubeStore result against Postgres, and
normalizereconciled the two by matching on theZsuffix that cubestored's MySQL layer emitted. RFC3339 has noZ, so that matcher is now positional on theYYYY-MM-DDTHH:MM:SSshape — a plaincontains('T')would have mangled theYTDcalc-group value.Snapshot changes
Only
pre_aggregations/*_cubestore_result.snapfiles changed; the 15 undertests/integration/cubestore/are untouched, which also confirmsYTDsurvives the new matcher. Compared at cell level (column padding shifts make the line diff unreadable): no row- or column-count changes, two classes, each verified against the reported Arrow schema:2025-01-01T00:00:00.000Z2025-01-01T00:00:00Timestamp(Microsecond, None).000Z; arrow renders plain RFC333910001000.00000Decimal128(28, 5)I also confirmed the server genuinely answers
Arrow, so a silentLegacyfallback isn't passing these tests without exercising the new wire format.The one remaining
mysqlreference in the harness is deliberate and commented:CUBESTORE_PORT/CUBESTORE_BIND_ADDRstay pinned to a free port because unset, cubestored binds0.0.0.0:3306and parallel test binaries collide.🤖 Generated with Claude Code