Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(cubesql): Match CubeScan timestamp literal types to member types #9275

Merged
merged 1 commit into from
Feb 26, 2025

Conversation

MazterQyou
Copy link
Member

@MazterQyou MazterQyou commented Feb 26, 2025

Check List

  • Tests have been run in packages where changes 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 changes type of timestamp literal values in CubeScan to TimestampNanosecond. They previously converted to TimestampMillisecond which does not match member type (which is actually TimestampNanosecond despite Cube returning millisecond precision values). This led to issues with two CubeScans, one containing a member and another a literal, being unable to UNION.

Example plan that fails to execute:

Union
  Projection: CAST(NULL AS Timestamp(Millisecond, None)) AS date, #SUM(amount) AS amount
    Aggregate: groupBy=[[]], aggr=[[SUM(#cube.amount)]]
      TableScan: cube projection=None
  Projection: #cube.date AS date, #SUM(amount) AS amount
    Aggregate: groupBy=[[#cube.date]], aggr=[[SUM(#cube.amount)]]
      TableScan: cube projection=None

As Cube date time columns are considered TimestampMillisecond, initial planning provides Millisecond type; but CubeScan member types are TimestampNanosecond, and two CubeScans cannot be unioned in this case. As such, rewriting produces an invalid plan.

@MazterQyou MazterQyou requested a review from a team as a code owner February 26, 2025 10:27
Copy link

codecov bot commented Feb 26, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 83.65%. Comparing base (1dfa10d) to head (d229fcf).
Report is 6 commits behind head on master.

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #9275   +/-   ##
=======================================
  Coverage   83.65%   83.65%           
=======================================
  Files         227      227           
  Lines       81773    81773           
=======================================
  Hits        68409    68409           
  Misses      13364    13364           
Flag Coverage Δ
cubesql 83.65% <100.00%> (ø)

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.

@MazterQyou MazterQyou force-pushed the cubesql/fix-literal-types branch from 37529f7 to d229fcf Compare February 26, 2025 13:09
@MazterQyou MazterQyou merged commit 4a4e82b into master Feb 26, 2025
74 checks passed
@MazterQyou MazterQyou deleted the cubesql/fix-literal-types branch February 26, 2025 13:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants