Skip to content

fix(tesseract): parenthesize compound exprs before ::timestamptz cast#10859

Merged
waralexrom merged 1 commit into
masterfrom
repro-granularity-time-cast-precedence
May 12, 2026
Merged

fix(tesseract): parenthesize compound exprs before ::timestamptz cast#10859
waralexrom merged 1 commit into
masterfrom
repro-granularity-time-cast-precedence

Conversation

@waralexrom
Copy link
Copy Markdown
Member

Summary

Fix a :: precedence trap in PlanSqlTemplates::convert_tz: when field is a composed expression (e.g. a granularity-expanded date_trunc(...) + interval '1 month', or a user dimension like {CUBE}.created_at - interval '1 day'), the driver template ${field}::timestamptz AT TIME ZONE 'UTC' binds :: to the trailing operand instead of the full expression. Postgres then errors out with cannot cast type interval to timestamp with time zone.

Changes

  • PlanSqlTemplates::convert_tz now wraps field in (...) when is_top_level_compound(&field) reports a top-level operator/operator-keyword. Atomic and already-parenthesized forms pass through unchanged.
  • Filter pipeline picks the fix up automatically via FilterSqlContext::convert_tz delegation.
  • Reuses the existing sql_expression_scanner::is_top_level_compound classifier — no new tokenizer work.

Testing

  • New integration test test_type_time_alias_wraps_compound_exprs_before_tz_cast (covers both + and - compound shapes via two dimensions: a fiscal_year_alias time-typed alias over a custom granularity, and a created_at - interval '1 day' expression).
  • Assertion checks two precedence-trap fingerprints (interval 'X'::timestamptz) are absent from the rendered SQL.
  • Runs the query against real Postgres via try_execute_pg and snapshots the result.

…convert_tz

`PlanSqlTemplates::convert_tz` passed `field` straight into the driver
template (`{}::timestamptz AT TIME ZONE 'UTC'`). For composed inputs
like a granularity-expanded `date_trunc(...) + interval '1 month'`,
`::` bound to the trailing interval literal instead of the full
expression — Postgres reported `cannot cast type interval to timestamp
with time zone`.

Wrap `field` in parentheses when `is_top_level_compound` reports a
top-level operator; atomic and already-parenthesized forms pass through
unchanged. Filter pipeline picks the fix up via `convert_tz` delegation.

Repro covered by `test_type_time_alias_wraps_compound_exprs_before_tz_cast`
(both `+` and `-` compound shapes, with PG execution snapshot).
@github-actions github-actions Bot added the rust Pull requests that update Rust code label May 11, 2026
@claude
Copy link
Copy Markdown
Contributor

claude Bot commented May 11, 2026

Claude finished @waralexrom's task in 2m 3s —— View job


PR #10859 — Code Review

  • Gather context and read changed files
  • Review core fix in plan.rs
  • Review test fixtures and integration tests
  • Review snapshot file
  • Post comprehensive review

Overall Assessment

This is a clean, well-scoped fix. The bug is real — Postgres's :: cast operator binds tighter than arithmetic, so date_trunc(...) + interval '1 month'::timestamptz parses as date_trunc(...) + (interval '1 month'::timestamptz) rather than (date_trunc(...) + interval '1 month')::timestamptz. The fix intercepts at the right layer (PlanSqlTemplates::convert_tz) and reuses existing infrastructure (is_top_level_compound) without introducing new tokenizer work.


Code Quality

The fix itself (plan.rs:56-63) is minimal and correct:

let safe = if is_top_level_compound(&field) {
    format!("({field})")
} else {
    field
};
self.driver_tools.convert_tz(safe)

Wrapping only when needed avoids unnecessary parentheses in the common case (plain column references). The is_top_level_compound scanner already has thorough unit tests (47 cases covering arithmetic, logical operators, CASE expressions, string/comment protection, etc.), so this doesn't need re-validation.

Minor observation: The parenthesization is correct even for the edge case where field is already parenthesized (e.g., (a + b)) — is_top_level_compound returns false for fully parenthesized expressions since the + lives at depth != 0, so no double-wrapping occurs. Good.


Fix Placement

The fix in PlanSqlTemplates::convert_tz is the right chokepoint — both call paths flow through it:

  1. Time dimension with granularitytime_dimension.rs:68 calls templates.convert_tz(input_sql)?
  2. Raw time dimensiontime_dimension.rs:95 calls templates.convert_tz(input_sql)?
  3. Filter pipelinefilter_sql_context.rs:102-103 delegates to self.plan_templates.convert_tz(field.to_string())

All three paths are covered by the single fix point.


Security

No concerns. The fix adds parentheses around an already-constructed SQL expression — it doesn't introduce any new string interpolation or user-input handling. The field value is built internally by the SQL planner, not from external input.


Performance

Negligible. is_top_level_compound does a single-pass tokenization of the SQL fragment (typically short — a column reference or arithmetic expression). This runs once per convert_tz call, not in a hot loop. The format!("({field})") allocation is only paid when wrapping is actually needed.


Testing

The test is well-designed:

  1. Two independent test dimensionsfiscal_year_alias (custom granularity offset producing date_trunc(...) + interval '1 month') and created_at_minus_one_day (user expression {CUBE}.created_at - interval '1 day') — covering both + and - compound shapes.

  2. Negative assertion checking for the specific precedence-trap fingerprints (interval '1 month'::timestamptz and interval '1 day'::timestamptz) is a good defensive regression check.

  3. Snapshot test via try_execute_pg verifies end-to-end correctness against real Postgres when available.

One suggestion (non-blocking): The scanner's own unit tests don't include an interval-arithmetic expression like date_trunc('year', x) + interval '1 month'. While a + b is already covered and is structurally identical, adding a domain-specific case to sql_expression_scanner.rs tests could serve as documentation of why this scanner matters for the convert_tz use case.


Summary

Clean, minimal fix at the right abstraction layer. Reuses existing infrastructure without introducing new complexity. Test coverage is solid with both assertion-based regression checks and real Postgres execution. Looks good to merge.

@codecov
Copy link
Copy Markdown

codecov Bot commented May 11, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 78.86%. Comparing base (e60ad99) to head (afd73b2).
⚠️ Report is 5 commits behind head on master.

Additional details and impacted files
@@             Coverage Diff             @@
##           master   #10859       +/-   ##
===========================================
+ Coverage   58.36%   78.86%   +20.49%     
===========================================
  Files         216      470      +254     
  Lines       16939    92289    +75350     
  Branches     3435     3435               
===========================================
+ Hits         9887    72783    +62896     
- Misses       6550    19004    +12454     
  Partials      502      502               
Flag Coverage Δ
cube-backend 58.36% <ø> (ø)
cubesql 83.47% <ø> (?)

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

☔ View full report in Codecov by Sentry.
📢 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.

@waralexrom waralexrom merged commit cab0a18 into master May 12, 2026
247 of 257 checks passed
@waralexrom waralexrom deleted the repro-granularity-time-cast-precedence branch May 12, 2026 19:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

rust Pull requests that update Rust code

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants