Skip to content

fix(schema-compiler): remove AthenaQuery.convertTz override that produces wrong timestamps on engine v3#10779

Merged
paveltiunov merged 5 commits intomasterfrom
cursor/fix-athena-convert-tz-2088
Apr 30, 2026
Merged

fix(schema-compiler): remove AthenaQuery.convertTz override that produces wrong timestamps on engine v3#10779
paveltiunov merged 5 commits intomasterfrom
cursor/fix-athena-convert-tz-2088

Conversation

@paveltiunov
Copy link
Copy Markdown
Member

@paveltiunov paveltiunov commented Apr 29, 2026

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

On Athena engine v3 (Trino-based), CAST(timestamp WITH TIME ZONE AS TIMESTAMP) returns the UTC instant's wall-clock fields, not the local wall-clock at the specified zone. The AthenaQuery.convertTz override used this exact pattern:

CAST((field AT TIME ZONE 'Asia/Kolkata') AS TIMESTAMP)
-- Returns: 2026-03-31 18:31:47 (UTC wall-clock) ❌
-- Expected: 2026-04-01 00:01:47 (Asia/Kolkata wall-clock) ✅

This caused every convertTz call on engine v3 to silently return UTC wall-clock instead of the target timezone's wall-clock, leading to incorrect time dimension bucketing (e.g., monthly buckets shifted by the timezone offset).

Fix: Remove the convertTz override from AthenaQuery so it inherits PrestodbQuery.convertTz, which correctly handles this by manually shifting using timezone_hour + timezone_minute offsets before casting:

CAST(date_add('minute', timezone_minute(field AT TIME ZONE 'tz'),
  date_add('hour', timezone_hour(field AT TIME ZONE 'tz'), field)) AS TIMESTAMP)

Changes:

  • packages/cubejs-schema-compiler/src/adapter/AthenaQuery.ts — removed the convertTz override and its stale comment
  • packages/cubejs-schema-compiler/test/unit/athena-query.test.ts — added unit test verifying AthenaQuery.convertTz uses the manual offset calculation
  • packages/cubejs-testing-drivers/src/tests/testQueries.ts — added E2E test case querying with timezone: 'Asia/Kolkata' to verify non-UTC monthly bucketing works correctly across all drivers including Athena
Open in Web Open in Cursor 

…uces wrong timestamps on engine v3

On Athena engine v3 (Trino-based), CAST(timestamp WITH TIME ZONE AS TIMESTAMP)
returns the UTC instant's wall-clock fields, not the local wall-clock at the
specified zone. The AthenaQuery.convertTz override used this pattern, silently
returning UTC wall-clock instead of the target timezone's wall-clock.

Remove the override so AthenaQuery inherits PrestodbQuery.convertTz, which
manually shifts by timezone_hour + timezone_minute offsets before casting.

Add a unit test to verify AthenaQuery.convertTz uses the manual offset
calculation rather than the broken simplified cast.

Co-authored-by: Pavel Tiunov <pavel.tiunov@gmail.com>
@claude
Copy link
Copy Markdown
Contributor

claude Bot commented Apr 29, 2026

Claude finished @paveltiunov's task in 2m 5s —— View job


Code Review: PR #10779

  • Gather context and review the diff
  • Review the removed convertTz override in AthenaQuery.ts
  • Review the inherited PrestodbQuery.convertTz implementation
  • Review the new unit test
  • Check for edge cases and related code
  • Post final review

Overall: This is a correct and well-scoped fix. The change is minimal, the problem is clearly explained, and the inherited PrestodbQuery.convertTz handles timezone conversion properly by extracting hour/minute offsets before casting.

What looks good

  1. Root cause is sound. The Trino-based Athena v3 engine's CAST(timestamp WITH TIME ZONE AS TIMESTAMP) returns the UTC wall-clock fields, not the local wall-clock. Falling back to the parent's manual offset calculation (timezone_hour + timezone_minute + date_add) avoids this behavior entirely.

  2. Minimal change. Removing a 5-line override and letting inheritance do the work is the right approach — no new code to maintain.

  3. Test covers the key contract. The test verifies both positive assertions (timezone_hour, timezone_minute present) and a negative assertion (the broken CAST((... AT TIME ZONE ...) AS TIMESTAMP) pattern is absent).

Issues and suggestions

TrinoQuery has the identical broken override (medium concern)

TrinoQuery.ts:6-8 has the exact same convertTz override that was just removed from AthenaQuery, with an identical comment:

// Trino doesn't require odd prestodb manual datetime offset calculations
// as it uses mature timestamps models
public override convertTz(field) {
    return this.timezone ? `CAST((${field} AT TIME ZONE '${this.timezone}') AS TIMESTAMP)` : field;
}

Since Athena v3 is Trino-based, and the CAST(... AT TIME ZONE ... AS TIMESTAMP) behavior is a Trino engine characteristic, this same bug likely exists in the standalone TrinoQuery adapter too. This might be intentionally out of scope for this PR, but it's worth investigating. If the Trino adapter is used with recent Trino versions that have the same CAST semantics, users hitting this via the Trino driver would see the same incorrect bucketing.

Investigate TrinoQuery →

Test regex could be slightly more precise (nit)

The negative assertion at line 49-51:

expect(sql).not.toMatch(
  /CAST\(\(.*AT TIME ZONE.*\) AS TIMESTAMP\)/
);

This works correctly today because PrestodbQuery.convertTz produces CAST(date_add(...) not CAST((...), so the CAST\(\( anchor won't match. However, it's a somewhat fragile check — a snapshot test of the full SQL output would be more resilient to future refactors. That said, the current approach is readable and explicit about what it's guarding against, which has its own value. Not blocking.

Empty class body (nit)

AthenaQuery.ts is now an empty class:

export class AthenaQuery extends PrestodbQuery {
}

This is fine — it serves as a named type for the query builder registry in QueryBuilder.ts:31 (athena: AthenaQuery), and having a dedicated file means future Athena-specific overrides have a natural home.

Summary

Clean fix for a real bug. The removed code was originally added with the assumption that Athena's CAST(... AT TIME ZONE ... AS TIMESTAMP) correctly localizes timestamps, which is not the case on engine v3. The inherited PrestodbQuery.convertTz with manual timezone_hour/timezone_minute offsets is the correct approach.

One action item: Please consider whether TrinoQuery needs the same fix — it has identical code.

@github-actions github-actions Bot added the javascript Pull requests that update Javascript code label Apr 29, 2026
Add a testQueries test case that queries ECommerce.count by month with
timezone 'Asia/Kolkata'. This exercises the convertTz code path in a
full Cube API stack to verify that non-UTC timezones produce correct
monthly time dimension bucketing on Athena (and all other drivers).

Co-authored-by: Pavel Tiunov <pavel.tiunov@gmail.com>
@codecov
Copy link
Copy Markdown

codecov Bot commented Apr 29, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 57.99%. Comparing base (d6ca598) to head (89b56dc).
⚠️ Report is 2 commits behind head on master.

❗ There is a different number of reports uploaded between BASE (d6ca598) and HEAD (89b56dc). Click for more details.

HEAD has 1 upload less than BASE
Flag BASE (d6ca598) HEAD (89b56dc)
cubesql 1 0
Additional details and impacted files
@@             Coverage Diff             @@
##           master   #10779       +/-   ##
===========================================
- Coverage   78.77%   57.99%   -20.79%     
===========================================
  Files         466      215      -251     
  Lines       92087    16775    -75312     
  Branches     3389     3388        -1     
===========================================
- Hits        72546     9728    -62818     
+ Misses      19049     6554    -12495     
- Partials      492      493        +1     
Flag Coverage Δ
cube-backend 57.99% <ø> (+0.02%) ⬆️
cubesql ?

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.

@paveltiunov paveltiunov marked this pull request as ready for review April 29, 2026 21:32
@paveltiunov paveltiunov requested review from a team as code owners April 29, 2026 21:32
@claude
Copy link
Copy Markdown
Contributor

claude Bot commented Apr 29, 2026

Claude finished @paveltiunov's task in 2m 4s —— View job


Code Review: PR #10779 — fix(schema-compiler): remove AthenaQuery.convertTz override

  • Gather context and review the diff
  • Review AthenaQuery.ts changes
  • Review PrestodbQuery.convertTz (inherited implementation)
  • Review TrinoQuery.convertTz for the same issue
  • Review the new unit test
  • Review the E2E test addition
  • Post final review

Overall: This is a correct, well-scoped fix. The root cause analysis is sound, the change is minimal, and the inherited PrestodbQuery.convertTz properly handles timezone conversion via manual offset extraction.

What looks good

  1. Root cause is accurate. On Trino-based Athena v3, CAST(timestamp WITH TIME ZONE AS TIMESTAMP) strips the timezone and returns UTC wall-clock values, not local wall-clock. The removed override was producing exactly this broken pattern. Falling back to PrestodbQuery.convertTz (packages/cubejs-schema-compiler/src/adapter/PrestodbQuery.ts:52-57) which uses timezone_hour/timezone_minute extraction before casting avoids the issue entirely.

  2. Minimal change, maximal clarity. Removing a 5-line override and inheriting from the parent is the right call. No new logic to maintain, and the empty AthenaQuery class still serves its purpose as a named type in the query builder registry.

  3. Test design is solid. The unit test at packages/cubejs-schema-compiler/test/unit/athena-query.test.ts verifies both positive (offset functions present) and negative (broken CAST pattern absent) conditions. The E2E test at packages/cubejs-testing-drivers/src/tests/testQueries.ts:1161-1177 exercises a real non-UTC timezone query path with snapshot verification.

Issues

TrinoQuery has the identical broken override (medium)

packages/cubejs-schema-compiler/src/adapter/TrinoQuery.ts:3-8 has the exact same convertTz override with the same stale comment:

export class TrinoQuery extends PrestodbQuery {
  // Trino doesn't require odd prestodb manual datetime offset calculations
  // as it uses mature timestamps models
  public override convertTz(field) {
    return this.timezone ? `CAST((${field} AT TIME ZONE '${this.timezone}') AS TIMESTAMP)` : field;
  }
}

Since the Athena v3 engine is Trino, this same bug will affect users of the standalone Trino driver. The CAST(... AT TIME ZONE ... AS TIMESTAMP) behavior is a Trino engine characteristic, not an Athena-specific one. This might be intentionally out of scope, but it should be investigated — standalone Trino users hitting this through TrinoQuery will see the same incorrect monthly bucketing.

Fix TrinoQuery →

Nits

Empty class body style (cosmetic, non-blocking)

AthenaQuery.ts is now:

export class AthenaQuery extends PrestodbQuery {
}

This is fine — it acts as a named entry point for the query builder registry and a natural home for any future Athena-specific overrides. No action needed.

Test regex precision (non-blocking)

The negative assertion at line 49-51:

expect(sql).not.toMatch(/CAST\(\(.*AT TIME ZONE.*\) AS TIMESTAMP\)/);

This works today because PrestodbQuery.convertTz produces CAST(date_add(...) not CAST((...), so the double-paren anchor CAST\(\( won't false-match. It's a perfectly acceptable guard for this specific regression. A SQL snapshot test would be more resilient to future refactors, but the current approach clearly communicates the intent: "this broken pattern must not appear."

Summary

Clean, minimal fix for a real bug that caused all Athena v3 convertTz calls to silently return UTC wall-clock instead of the target timezone's wall-clock. The inherited PrestodbQuery.convertTz with manual timezone_hour/timezone_minute offsets is the correct approach.

One action item: Please consider whether TrinoQuery needs the same fix — it has identical code and the same underlying Trino engine behavior.

cursoragent and others added 3 commits April 29, 2026 21:37
…ation mismatch

The non-UTC timezone test was failing because ECommerce.count with
orderDate matched the MultiTimeDimForCount pre-aggregation, which was
only built for UTC. Switch to customOrderDateNoPreAgg time dimension
which has no pre-aggregation, so the query goes directly to the source
database and exercises the convertTz code path.

Co-authored-by: Pavel Tiunov <pavel.tiunov@gmail.com>
…non-UTC timezone (Asia/Kolkata)' test

Co-authored-by: Pavel Tiunov <pavel.tiunov@gmail.com>
MySQL returns count values as numbers, not strings. Update the snapshot
to match MySQL's actual output format.

Co-authored-by: Pavel Tiunov <pavel.tiunov@gmail.com>
@paveltiunov paveltiunov merged commit f395432 into master Apr 30, 2026
117 of 118 checks passed
@paveltiunov paveltiunov deleted the cursor/fix-athena-convert-tz-2088 branch April 30, 2026 00:54
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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants