Conversation
Render Float32 and Float64 literals through dialect casts so integral-valued floats do not become integer arithmetic after serialization or constant folding. This follows existing decimal literal handling and preserves intentional integer division. Explicit float typing can change source-inferred decimal arithmetic and requires compatible dialect cast support. Add renderer and planner regressions and update affected SQL expectations. Validation: 821 compiler tests passed, 6 ignored; workspace formatting and all-targets Clippy passed. Cross-dialect end-to-end validation remains outstanding.
github-actions
Bot
added
rust
|
/bot-deep-review |
|
Claude finished @ovr's task in 7m 17s —— View job Deep review complete — the fix is correct in principle, but the cast target isn't valid on every shipped dialect. 1 high, 1 medium, 2 low (4 inline comments).Full reviewTodos
What the change does
The diagnosis is right and the test expectation changes confirm it. The clearest one is - CEIL((CAST(EXTRACT(doy FROM …) AS INTEGER) / 7))
+ CEIL((CAST(EXTRACT(doy FROM …) AS INTEGER) / CAST(7 AS DOUBLE)))
Findings
Finding 1 is the one that decides whether this is mergeable as-is. Details and two possible fix directions are in the inline comment; the short version is either gate float-literal pushdown on template availability in Smaller notes (no inline comment)
ValidationReviewed statically only — I did not build or run the Rust test suite in this job ( |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
|
Addressed the review comments. I've added a provider test which I ran on all platforms I could easily setup in my docker: MySQL 8.0.22 |
|
/bot-deep-review |
|
Claude finished @MazterQyou's task in 10m 55s —— View job Deep review complete — the template gate and the MySQL/Oracle work hold up; the Vertica fix is on the wrong class. 1 high, 3 low (4 inline comments), 1 prior thread resolved.Full reviewTodos
Prior roundThree of the four threads were addressed and are already resolved. The percentile one I've now resolved too — a The restructuring is a clear improvement over the first round:
Findings
Only finding 1 needs to happen before merge. It's a one-line move plus a test that points at the class the runtime actually uses. Dialect sweepI re-checked
Smaller notes (no inline comment)
ValidationStatic review plus source verification — no build or test run in this job. The runtime class-resolution claim in finding 1 was traced through CI has produced no signal: all 13 workflow runs on this PR are |
|
/bot-deep-review |
|
Claude finished @MazterQyou's task in 10m 6s —— View job Deep review complete — the Vertica fix lands on the right class this time; what's left is where these type names get replayed and what CI actually runs. 2 medium, 1 low (3 inline), 2 prior threads re-affirmed.Full reviewTodos
Prior roundThe high finding is genuinely fixed, and I verified the resolution path rather than taking it on trust: The three low threads from last round: one you declined with a reason I accept (an existence sweep wouldn't have caught an invalid type name, which was the actual bug), two are still open and still apply — I have not re-posted them:
Findings
Finding 1 is cheap and I'd do it before merge — a one-line What I re-verified this round
Smaller notes (no inline comment)
ValidationStatic review plus source verification; no build or test run in this job. The Cube Store claim in finding 2 was traced through CI still gives no signal: all 9 workflow runs on this PR are |
|
/bot-deep-review |
|
Claude finished @MazterQyou's task in 17m 36s —— View job Deep review complete — approach is sound; 3 findings (1 medium, 2 low) + 3 red CI checks to triageFull review
Findings
Each is an inline comment with the concrete failing input. CI
I could not read any of those job logs from this runner (the log paths sit outside the workspace and What holds up
One note, not a findingThe cast applies to every float literal, including ones that never lost their type — No prior review threads needed resolving — all 24 are already resolved, and none duplicate the findings above. |
Cover the runtime dialect through KsqlDriver.dialectClass(). Scope the float provider test out of QuestDB Tesseract after reproducing its parameter-finalization panic with an integer-only query; retain legacy coverage and all SQL assertions.
|
/bot-deep-review |
|
Claude finished @MazterQyou's task in 19m 37s —— View job Deep review: 2 findings (1 medium, 1 low) — plus a note on the red
|
| # | Severity | Location | Concern |
|---|---|---|---|
| 1 | medium | rust/cubesql/cubesql/src/compile/rewrite/rules/wrapper/literal.rs:95 |
DataSource::Unrestricted makes the new gate return true without reading a template, so a dialect lacking types/float (ksqlDB, after this PR) hard-errors instead of falling back to local evaluation |
| 2 | low | packages/cubejs-testing-drivers/fixtures/questdb.json:236 |
The new tesseractSkip documents an unchecked-index panic at wrapper.rs:268 with nothing tracking it, so the skip has no removal condition |
Both posted inline. No prior threads were re-affirmed or resolved: all 27 review threads on this PR are already resolved, and neither finding duplicates one.
One candidate finding was dropped rather than posted: float_literal_expr ignores data_type on the expressions/float_literal path, so MySQL/MongoBI get double-precision exponent literals for Float32. That is an explicitly documented decision (transport/service.rs:938-940) already settled in two earlier threads, and I have no dialect where the single-precision difference is observable — restating it would not be actionable.
CI
Unit (Rewrite Engine) (job 105509903736) is red on the head commit. Its log isn't reachable from this sandbox, so this is a hypothesis rather than a conclusion: the step runs cargo insta test --all-features --workspace --unreferenced reject, this is a fork PR (davidda/cube), so CUBESQL_TESTING_CUBE_TOKEN / CUBESQL_TESTING_CUBE_URL are empty, PostgresIntegrationTestSuite::before_all returns Skipped (e2e/tests/postgres.rs:59), and the 9 snapshots under e2e/tests/snapshots/ are then never referenced — which --unreferenced reject fails on. That would be PR-independent. Worth confirming against another community PR before spending time on it.
The other red check, unit (26.x, 3.13), failed in its Yarn install step — infrastructure, not the diff.
I could not run the Rust suite or the JS unit tests here (cargo and cargo fetch are blocked in this environment, and the CI log endpoints are unreadable), so everything above is static review plus CI metadata.
What holds up
The core of the change is right, and the parts most likely to be wrong are the parts that were checked hardest:
- The gate (
literal.rs:94-116) and the renderer (service.rs:932-963) now share one type-availability lookup viacontains_sql_type/sql_type_template, so "can push" and "can render" cannot drift apart — this was the right fix for the earlier hardcoded template names. - Deleting
templates.types.floatfor ksqlDB is safe beyond the literal path: the only other Float32 renderer isExpr::Cast, andtransform_cast_expralready gates oncontains_sql_type(rules/wrapper/cast.rs:76-80), so a user-writtenCAST(x AS REAL)falls back locally rather than erroring.generate_typed_nullis only reached for types ksqlDB keeps. - NULL floats now go through
nullable_type()on the cast path, matchinggenerate_typed_null, and thetypes/nullablecase is covered by the ClickHouse-shaped generator inwrapper.rs's new test. - Oracle (
BINARY_FLOAT/BINARY_DOUBLE) and Vertica (DOUBLE PRECISION) are strict fixes — neither engine has a bareDOUBLEtype, so those casts were already wrong for decimal literals and typed NULLs. Fixing bothVerticaQuerycopies, with the driver-owned one pinned throughVerticaDriver.dialectClass(), is the right call given only that copy is live at runtime. - Blocking NaN/infinity at the gate replaces SQL that no dialect could parse with a local fallback.
- Every dialect that keeps the
FLOAT/DOUBLEbase defaults accepts them as cast targets (BigQuery overrides toFLOAT64, Presto/Trino toREAL, Postgres/Redshift toREAL/DOUBLE PRECISION, MSSQL toFLOAT(24)/FLOAT(53); ClickHouse, Databricks, Druid, DuckDB, Hive, Pinot, QuestDB, Snowflake, SQLite, CubeStore and Crate all take the defaults). Oracle, Vertica and ksqlDB were the three gaps, and all three are closed here.
Rollout note
This is in cubesql's push-down layer, so it applies to both planners equally — Tesseract has no float-literal renderer of its own (types/float/types/double appear only in its test mocks).
Two consequences worth stating in the release notes, both already acknowledged in the PR body:
- Arithmetic against a source-decimal column changes.
SELECT 1.0 * SUM(price) / COUNT(*)previously pushed1 * SUM(price)and PostgreSQL evaluated it innumeric; it now pushesCAST(1 AS DOUBLE PRECISION) * SUM(price)and evaluates indouble precision. That is the intended fix (it is what DataFusion's plan says), but a deployment over money columns will see values move in the last digits. - Pushed member-expression SQL changes for any query containing a float literal, so the first run after upgrade misses the query cache.
Require float support across available generators for unrestricted contexts. Apply the same check to literal scan members, which bypass expression rewrite gates, so missing float types retain local evaluation. Cover both widths with the LIMIT 0 aggregate reproducer and extend the existing gate matrix to unrestricted contexts.
|
/bot-deep-review |
|
Claude finished @MazterQyou's task in 15m 43s —— View job Deep review — looks good to merge; 3 low-severity findings (0 high, 0 medium, 3 low), all test/comment qualityReview details
Findings posted inline
What I verified and found soundGate/renderer agreement. SQL generation happens during planning ( Second gate site. Dialect cast targets. Walked every Pre-aggregations. The Oracle thread's answer generalises: Behaviour changes are the intended ones. The two Gate hardening is a net improvement beyond the reported symptom. Notes, not findings
Review threadsThe listing returned 29 of my own threads, all already resolved — nothing unresolved to resolve or re-affirm this round. Four of my findings duplicate resolved threads carrying the author's deferral and are not re-posted: the cast path losing negative zero ( |
Exercise supported, unsupported, and unrestricted contexts in the existing float gate matrix, and identify the source in assertion failures. Move the non-finite literal explanation next to the helper checks it describes.
|
/bot-deep-review |
|
Claude finished @MazterQyou's task in 16m 58s —— View job Deep review done — 3 findings posted inline (1 medium, 2 low); CI is red on the head commitReview detailTodo
Findings
No duplicate threads to skip — all 32 prior review threads on this PR are already resolved, and none of mine were left stale. CI
Verified as correct
Residual risk (not re-raised inline)
|
|
/bot-deep-review |
|
Claude finished @MazterQyou's task in 22m 37s —— View job Deep review — PR #11781 — 1 new finding (medium), 5 prior threads re-affirmed; CI is red on
|
| Severity | Where | What |
|---|---|---|
| Medium | rust/cubesql/cubesql/src/compile/rewrite/rules/wrapper/literal.rs:107-113 |
The DataSource::Unrestricted arm quantifies over every data source in the tenant. This PR also deletes types.float from KsqlQuery (packages/cubejs-ksql-driver/src/KsqlQuery.ts:61), so one ksqlDB data source anywhere in the tenant blocks Float32 literal pushdown in unrestricted contexts for all the other data sources too — a pushdown regression created by the two halves of this PR landing together. Posted inline. |
CI
Build native (run 35698870320) is failing on the head commit 83c90ba, in two Rust test jobs:
- Unit (Rewrite Engine) — step Unit tests (Rewrite Engine) (
cargo insta test --all-features --workspace --unreferenced reject) - Build windows-2022 24.x with Python fallback — step Tests
Check fmt/clippy passed, so this is a runtime test failure, not a compile or lint error. I could not read the job log (log paths are outside this runner's permitted directories) and Rust builds are not available here, so I cannot name the failing test — please check it before merge. Note the job gate is --unreferenced reject, so an orphaned .snap would also fail it.
What I checked and found clean
- Renderer. The literal operand string is byte-identical to the old
format!("{f}")for both widths —(value as f32).to_string()round-trips exactly throughf64::from, andDisplayforf64is unchanged. Only the surrounding cast is new, so the pre-existing decimal-literal range limits (e.g. MSSQL'snumeric(38)ceiling forf32::MAX) are not a new regression. The NULL path reproducesgenerate_typed_nullexactly, includingnullable_type. - Gate coverage. A missing type template is not a soft fallback:
evaluate_wrapped_sqlatrust/cubesql/cubesql/src/compile/query_engine.rs:341propagates a generation error as aCompilationError, so an ungated float-literal path would be a hard query failure. I traced the three render paths —generate_sql_for_expr'sExpr::Literal(gated bytransform_literal), literal scan members ingenerate_sql_for_cube_scan(wrapper.rs:1058) and the push-to-cube column path (wrapper.rs:2902) — and all three are now covered, the latter two by the singlewrapper-cube-scan-wrapentry point thatcube_scan_wrapper.rs:186-201filters. No other wrapper rule synthesises a floatLiteralExprValue. - Borrowck/NLL. Hoisting
let Some(members) = …out of thedata_sourcesblock incube_scan_wrapper.rs:155keeps the immutable e-graph borrow alive only to line 201, before theegraph.addcalls — behaviour is unchanged for theNonecase (both returnvec![]). - Dialect audit. Walked every
types.float/types.doublevalue that a float literal can now reach: BigQueryFLOAT64, Presto/Trino/AthenaREAL/DOUBLE, Postgres/RedshiftREAL/DOUBLE PRECISION, MSSQLFLOAT(24)/FLOAT(53), OracleBINARY_FLOAT/BINARY_DOUBLE, VerticaFLOAT/DOUBLE PRECISION, and the baseFLOAT/DOUBLEinherited by ClickHouse, Snowflake, Hive, SQLite, Crate, DuckDB, Databricks, Pinot, QuestDB, Druid, Firebolt and CubeStore — all valid cast targets in their dialect. The one gap (ksqlDB has no 32-bit float) is the one this PR closes. - Template plumbing.
expressions/float_literalis additive: both consumers flattengroup/nameinto aHashMap<String, String>(transport/service.rs,cube_bridge/sql_templates_render.rs), so no dialect struct rejects the new key, and the MySQL template parses under minijinja. - Side effects worth calling out as improvements.
GROUP BY 0for a Float64 literal dimension (the Power BIpivot_groupingcase,mod.rs:7286) becomesGROUP BY CAST(0 AS DOUBLE), which stops PostgreSQL reading it as an ordinal; andCAST(… AS INTEGER) / 7(mod.rs:7830) becomes a real float division. - Comments. Every comment added is ≤3 lines and load-bearing; none restate the code under it.
I could not run the test suites: node_modules is not installed in this checkout and the Rust toolchain is not available, so nothing below the static review was executed here.
Review threads
The listing returned 35 threads whose first comment is mine. Resolved 24 as no longer live (22 fixed in the current diff, 2 findings withdrawn after davidda refuted them — the Snowflake/percentile constant-expression one and the Oracle Cube Store pre-aggregation one). Re-affirmed 5 that the new findings duplicate: dropped error context at wrapper.rs:3183, the override path's missing type/NULL contract at service.rs:945, the pinned template string at float-literal-template.test.ts:12, the NUMERIC-comparison tradeoff at mod.rs:12273, and the renderer-level guard at service.rs:949. Six others stay open on davidda's deferrals. Note: gh reply-to-thread is not configured in this environment, so the two withdrawn findings were resolved without the explanatory reply the process asks for.
Verdict
The core change is sound and the fix matches the symptom in #11780. Two things gate merge independently of my finding: the red Rust CI, and the semantic decision the PR body already asks for — casting float literals changes comparisons against NUMERIC/DECIMAL columns in PostgreSQL, both at precision boundaries and by making the column's numeric index unusable for the pushed filter. That one is a maintainer call, not something I can settle.
|
/bot-deep-review |
|
Claude finished @MazterQyou's task in 19m 54s —— View job Deep review — PR #11781 · 1 medium, 1 low · no blockersFull review
Findings posted inline
Review threadsThe listing returned 36 threads first-authored by Three findings I had were dropped as duplicates of threads you already answered and closed, so they are not re-posted:
What I checked and found cleanGate coverage. Every production path into Dialect spellings. Walked every Pre-aggregations. The Injection surface. The only values interpolated are Nullability. Comments. All new comment blocks are ≤3 lines and load-bearing (the MySQL exponent rationale, the decimal-literal range caveat, the literal-scan-member bypass note). Nothing to trim. Blast radius worth a maintainer's eyeBeyond the precision/index trade-off already in the PR description, the snapshot at Not verifiedRust tests and the driver suites were not run here — no |
| let data_sources_out = data_sources_out | ||
| .into_iter() | ||
| .filter(|source| { | ||
| let data_source = source | ||
| .as_deref() | ||
| .map_or(DataSource::Unrestricted, DataSource::Specific); |
There was a problem hiding this comment.
The gate and the renderer disagree on how a literal-only scan resolves its data source, and the disagreement always falls on the strict side.
Member::LiteralMember::name() returns None (analysis.rs:220), so a scan whose only members are literals feeds no names into data_sources_for_member_names and data_sources comes back empty. Line 177 then makes data_sources_out exactly vec![None], and line 193 maps that None to DataSource::Unrestricted — which sends can_push_down_float_literal down its ControlFlow::Break(true) arm, demanding that every generator in meta.data_source_to_sql_generator render the float type.
At render time that same scan resolves a specific source: WrappedSelectNode::data_source_for_cube_scan (wrapper.rs:986-992) falls back to meta.data_source_for_cube_names(node.used_cubes) precisely because "a scan can have no members to resolve from".
Concretely, on a tenant with postgres + ksql (which this PR makes the one dialect without types/float, KsqlQuery.ts:61):
SELECT SUM(v) FROM (SELECT CAST(100 AS REAL) AS v FROM KibanaSampleDataEcommerce LIMIT 0) qKibanaSampleDataEcommerce is on postgres, whose generator has types/float = REAL and expressions/cast, so the literal renders fine — but ksql's generator fails the all() and the whole CubeScan is refused a wrapper, dropping pushdown for every other member in the scan. test_float_literal_member_pushdown_fallback (test_wrapper.rs:101) only exercises the single-source mock, so it passes either way.
alias_to_cube is already bound at line 134 in this same closure, so the specific source is available without extra lookups — resolving through it (as the renderer does) would keep the gate and the renderer in agreement:
let data_source = match source.as_deref() {
Some(source) => DataSource::Specific(source),
// A literal-only scan has no member names; the renderer resolves it from
// the cubes it scans, so the gate has to as well.
None => meta
.data_source_for_cube_names(alias_to_cube.iter().map(|(_, cube)| cube.as_str()))
.unwrap_or(DataSource::Unrestricted),
};(The general "unrestricted quantifies over every source" question is the one you already declined on literal.rs:113; this is the narrower case where the source is in fact known.)
| ScalarValue::Float32(_) | ScalarValue::Float64(_) => return false, | ||
| _ => return true, | ||
| }; | ||
| let supports_templates = |templates: &crate::transport::SqlTemplates| { |
There was a problem hiding this comment.
crate::transport::SqlTemplates is spelled out inline while line 14 already pulls DataSource and MetaContext from that module.
| let supports_templates = |templates: &crate::transport::SqlTemplates| { | |
| let supports_templates = |templates: &SqlTemplates| { |
with SqlTemplates added to the use crate::transport::{DataSource, MetaContext}; list.
This branch was successfully deployed
3 participants
Render Float32 and Float64 literals through dialect casts so integral-valued floats do not become integer arithmetic after serialization or constant folding. This follows existing decimal literal handling and preserves intentional integer division.
Explicit float typing can change source-inferred decimal arithmetic and comparisons and requires compatible dialect cast support. In PostgreSQL, pushed comparisons against NUMERIC/DECIMAL columns can coerce the column to floating point, changing precision-boundary results and preventing use of its numeric index for filtering. This trade-off preserves the planned Float32/Float64 type and needs maintainer agreement.
Check List
Issue Reference this PR resolves
#11780