Skip to content

fix(cubesql): Push down UTCTIMESTAMP (CURRENT_TIMESTAMP)#11195

Merged
MazterQyou merged 1 commit into
masterfrom
cubesql/utctimestamp-push-down
Jul 6, 2026
Merged

fix(cubesql): Push down UTCTIMESTAMP (CURRENT_TIMESTAMP)#11195
MazterQyou merged 1 commit into
masterfrom
cubesql/utctimestamp-push-down

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 adds UTCTIMESTAMP SQL push down templates so CURRENT_TIMESTAMP can use SQL push down when needed. Related test is included.

@MazterQyou MazterQyou requested review from a team as code owners July 2, 2026 20:39
@github-actions github-actions Bot added driver:druid Issues relating to the Druid driver rust Pull requests that update Rust code javascript Pull requests that update Javascript code data source driver driver:databricks driver:duckdb driver:pinot cubejs-jdbc-driver labels Jul 2, 2026
@claude

claude Bot commented Jul 2, 2026

Copy link
Copy Markdown
Contributor

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


Code Review

LGTM — 0 blocking issues, 2 minor notes. Expand for details.
  • Read PR diff
  • Verify each driver's SQL is UTC-correct
  • Check coverage vs. drivers without overrides
  • Check Rust test wiring

Summary

Straightforward, well-scoped change: each dialect-specific sqlTemplates() gets a UTCTIMESTAMP entry, and a rewriter test in test_wrapper.rs verifies the push-down surfaces NOW() AT TIME ZONE 'UTC' inside a subquery filter. The Rust side already had BuiltinScalarFunction::UtcTimestamp wired via eval_utc_timestamp — this PR just supplies the missing template so the emitted SQL is dialect-appropriate.

Per-dialect correctness (spot check)

Driver Template Notes
Postgres / Redshift-parent (NOW() AT TIME ZONE 'UTC') ✅ naive UTC timestamp
Redshift (override) GETDATE() ✅ Redshift is UTC-internal; NOW() is leader-node only — override is well-reasoned
BigQuery CURRENT_TIMESTAMP() ✅ always UTC
ClickHouse now('UTC')
MSSQL GETUTCDATE()
MySQL UTC_TIMESTAMP()
Oracle SYS_EXTRACT_UTC(SYSTIMESTAMP)
Snowflake SYSDATE() ✅ TIMESTAMP_NTZ in UTC
Presto CAST(NOW() AT TIME ZONE 'UTC' AS TIMESTAMP)
Databricks TO_UTC_TIMESTAMP(CURRENT_TIMESTAMP(), CURRENT_TIMEZONE())
DuckDB (NOW() AT TIME ZONE 'UTC') ✅ (requires ICU — commented)
Druid CURRENT_TIMESTAMP ⚠️ correctness depends on sqlTimeZone=UTC — comment acknowledges
Pinot NOW() ✅ epoch millis, inherently UTC

Minor notes (non-blocking)

  1. Drivers with no override are unchanged. BaseQuery does not define functions.UTCTIMESTAMP, so dialects that only extend BaseQuery (Sqlite, Vertica, Hive, Crate, ElasticSearch, Firebolt, QuestDB, KSQL, Dremio, CubeStore) will not push down CURRENT_TIMESTAMP — they will fall back to non-pushed-down evaluation, which is safe. Worth mentioning in the PR body so it's clear this is scoped to the listed dialects. Trino/Athena inherit correctly through PrestodbQuery.

  2. Druid sqlTimeZone assumption. If a downstream user overrides sqlTimeZone via query context, the pushed-down value will silently be wrong for that connection. The inline comment covers it, but if you want defense-in-depth you could emit TIMESTAMP_TO_MILLIS(CURRENT_TIMESTAMP) → MILLIS_TO_TIMESTAMP via UTC — probably not worth the complexity given the driver's default.

  3. Test could be tightened. test_wrapper_filter_subquery_current_timestamp only asserts the substring "NOW() AT TIME ZONE 'UTC'" appears anywhere in the wrapped SQL. Consider also asserting the surrounding shape (e.g. that it lands inside the WHERE/subquery, not a leftover expression) so a regression that emits it in the wrong place would still fail. Not required.

Nothing else stands out — nice, focused patch.

• Branch: [`cubesql/utctimestamp-push-down`](https://github.com/cube-js/cube/tree/cubesql/utctimestamp-push-down)

@codecov

codecov Bot commented Jul 2, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 96.87500% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 79.32%. Comparing base (fc0d82b) to head (66baf07).

Files with missing lines Patch % Lines
...t/cubesql/cubesql/src/compile/test/test_wrapper.rs 95.23% 1 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##           master   #11195   +/-   ##
=======================================
  Coverage   79.31%   79.32%           
=======================================
  Files         472      472           
  Lines       96921    96953   +32     
  Branches     3539     3539           
=======================================
+ Hits        76877    76908   +31     
- Misses      19538    19539    +1     
  Partials      506      506           
Flag Coverage Δ
cube-backend 59.35% <100.00%> (+0.02%) ⬆️
cubesql 83.71% <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.

@MazterQyou MazterQyou force-pushed the cubesql/utctimestamp-push-down branch from 3f7da9f to 66baf07 Compare July 6, 2026 18:05
@MazterQyou MazterQyou requested a review from a team as a code owner July 6, 2026 18:05
@claude

claude Bot commented Jul 6, 2026

Copy link
Copy Markdown
Contributor

Claude finished @MazterQyou's task in 38s —— View job


Code Review

LGTM — 0 blocking issues, 3 minor notes. Expand for details.
  • Read PR diff
  • Verify each driver's SQL is UTC-correct
  • Check coverage vs. drivers without overrides
  • Check Rust test wiring

Summary

Focused, well-scoped change. Each dialect-specific sqlTemplates() gets a UTCTIMESTAMP entry so CURRENT_TIMESTAMP push-down emits dialect-appropriate SQL. BuiltinScalarFunction::UtcTimestamp was already wired in Rust via eval_utc_timestamp; this PR just supplies the missing templates. The new rewriter test in test_wrapper.rs verifies the push-down surfaces NOW() AT TIME ZONE 'UTC' inside a subquery filter, and a matching smoke test snapshot is added in smoke-cubesql.test.ts.

Per-dialect correctness

Driver Template Notes
Postgres (NOW() AT TIME ZONE 'UTC') ✅ naive UTC timestamp
Redshift GETDATE() ✅ Redshift is UTC-internal; NOW() is leader-node only — override well-reasoned
BigQuery CURRENT_TIMESTAMP() ✅ always UTC
ClickHouse now('UTC')
MSSQL GETUTCDATE()
MySQL UTC_TIMESTAMP()
Oracle SYS_EXTRACT_UTC(SYSTIMESTAMP)
Snowflake SYSDATE() ✅ TIMESTAMP_NTZ in UTC
Presto CAST(NOW() AT TIME ZONE 'UTC' AS TIMESTAMP) ✅ (Trino/Athena inherit)
Databricks TO_UTC_TIMESTAMP(CURRENT_TIMESTAMP(), CURRENT_TIMEZONE())
DuckDB (NOW() AT TIME ZONE 'UTC') ✅ (ICU bundled)
Druid CURRENT_TIMESTAMP ⚠️ correctness depends on sqlTimeZone=UTC — comment acknowledges
Pinot NOW() ✅ epoch millis, inherently UTC

Minor notes (non-blocking)

  1. Drivers without overrides fall back to non-pushed-down evaluation. BaseQuery does not define functions.UTCTIMESTAMP, so dialects that only extend BaseQuery (Sqlite, Vertica, Hive, Crate, ElasticSearch, Firebolt, QuestDB, KSQL, Dremio, CubeStore) will not push down CURRENT_TIMESTAMP. That's safe — worth calling out in the PR body for clarity.

  2. Druid sqlTimeZone assumption. If a connection overrides sqlTimeZone, the pushed-down value will silently be wrong. Inline comment covers it, but defense-in-depth (e.g. MILLIS_TO_TIMESTAMP(TIMESTAMP_TO_MILLIS(CURRENT_TIMESTAMP)) normalized to UTC) could remove the ambiguity — probably not worth the complexity.

  3. Test could be tightened. test_wrapper_filter_subquery_current_timestamp only asserts the substring "NOW() AT TIME ZONE 'UTC'" appears anywhere in the wrapped SQL. Consider asserting the surrounding shape (that it lands inside the WHERE/subquery) so a regression emitting it in the wrong place still fails. Not required.

Nothing else stands out — nice, focused patch.

• Branch: cubesql/utctimestamp-push-down

@MazterQyou MazterQyou merged commit cde0cca into master Jul 6, 2026
151 of 153 checks passed
@MazterQyou MazterQyou deleted the cubesql/utctimestamp-push-down branch July 6, 2026 22:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

cubejs-jdbc-driver data source driver driver:databricks driver:druid Issues relating to the Druid driver driver:duckdb driver:pinot 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