Skip to content

fix(cubesql): Support EXTRACT(EPOCH) over timestamp diff on Snowflake#11105

Open
MazterQyou wants to merge 1 commit into
masterfrom
cubesql/snowflake-extract-epoch-diff
Open

fix(cubesql): Support EXTRACT(EPOCH) over timestamp diff on Snowflake#11105
MazterQyou wants to merge 1 commit into
masterfrom
cubesql/snowflake-extract-epoch-diff

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 detects EXTRACT(EPOCH FROM (a - b)) over a timestamp subtraction in the wrapper and, when the dialect provides an expressions/extract_epoch_diff template, renders the operands separately as a seconds diff. This is needed because Snowflake accepts EPOCH on a timestamp but not on an interval. Snowflake maps it to DATEDIFF(SECOND, b, a); all other dialects fall back to the existing EXTRACT(EPOCH FROM (a - b)) output unchanged. Related test is included.

Signed-off-by: Alex Qyoun-ae <4062971+MazterQyou@users.noreply.github.com>
@MazterQyou MazterQyou requested review from a team as code owners June 17, 2026 15:04
@github-actions github-actions Bot added rust Pull requests that update Rust code javascript Pull requests that update Javascript code labels Jun 17, 2026
@claude

claude Bot commented Jun 17, 2026

Copy link
Copy Markdown
Contributor

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


Code Review

Verdict: LGTM — 0 high, 0 medium, 2 low (nits)
  • Read PR diff
  • Inspect surrounding wrapper logic
  • Verify template-existence pattern matches existing usage
  • Check edge cases (Cast peeling, non-diff EPOCH, SQL injection)

Summary

Clean, well-scoped change. The detection path is correctly gated: dialect must opt in via expressions/extract_epoch_diff, otherwise behavior is unchanged. The SQL-injection guard on date_part runs before the new branch (wrapper.rs:3139), so the new code only executes for the literal "epoch". args[1] is safe — guarded by args.len() >= 2. The contains_template opt-in pattern is consistent with the existing expressions/interval handling in transport/service.rs:802.

The timestamp_diff_operands helper recursively peels Cast wrappers, which correctly handles EXTRACT(EPOCH FROM CAST(a - b AS …)) and falls through cleanly for non-subtraction inputs (regular EXTRACT(EPOCH FROM ts_column) still uses the standard expressions/extract template). Good fallback hygiene.

Snowflake template DATEDIFF(SECOND, {{ right }}, {{ left }}) correctly inverts operand order to match left - right semantics.

The test covers both arms — generic dialect keeps EXTRACT(epoch …), Snowflake-style dialect emits DATEDIFF(SECOND, …).

Nits (low)

  1. wrapper.rs:3066-3078timestamp_diff_operands only matches BinaryExpr with Operator::Minus. Semantically equivalent forms like Expr::Negative + addition, or DataFusion's date_sub/timestamp_sub scalar functions, won't be routed through the new path. Probably fine since the rewriter normalizes the typical incoming shape, but worth noting if Snowflake users report EXTRACT(EPOCH FROM <interval>) slipping through from other expression shapes.

  2. wrapper.rs:3144-3151 — Minor: sql_generator.get_sql_templates() is called twice (once for contains_template, once for extract_epoch_diff_expr). Not hot-path, just slightly noisy. A local binding would tidy it up.
    • Branch: cubesql/snowflake-extract-epoch-diff

@codecov

codecov Bot commented Jun 17, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 85.88235% with 12 lines in your changes missing coverage. Please review.
✅ Project coverage is 79.10%. Comparing base (28c6938) to head (80cba97).
⚠️ Report is 1 commits behind head on master.

Files with missing lines Patch % Lines
...t/cubesql/cubesql/src/compile/engine/df/wrapper.rs 74.35% 10 Missing ⚠️
...bejs-schema-compiler/src/adapter/SnowflakeQuery.ts 0.00% 1 Missing ⚠️
rust/cubesql/cubesql/src/compile/mod.rs 97.22% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master   #11105      +/-   ##
==========================================
- Coverage   83.61%   79.10%   -4.51%     
==========================================
  Files         256      472     +216     
  Lines       78904    96264   +17360     
  Branches        0     3524    +3524     
==========================================
+ Hits        65975    76152   +10177     
- Misses      12929    19597    +6668     
- Partials        0      515     +515     
Flag Coverage Δ
cube-backend 58.50% <0.00%> (?)
cubesql 83.61% <86.90%> (-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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

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