Skip to content

[CALCITE-7491] Literals of type TIMESTAMP WITH TIME ZONE cause crashes#4910

Merged
mihaibudiu merged 1 commit intoapache:mainfrom
mihaibudiu:issue7491
Apr 29, 2026
Merged

[CALCITE-7491] Literals of type TIMESTAMP WITH TIME ZONE cause crashes#4910
mihaibudiu merged 1 commit intoapache:mainfrom
mihaibudiu:issue7491

Conversation

@mihaibudiu
Copy link
Copy Markdown
Contributor

Jira Link

https://issues.apache.org/jira/browse/CALCITE-7491

Changes Proposed

Fixes bugs that prevented the runtime from instantiating TIMESTAMP WITH TIME ZONE literal values.

Also, fixes defines two TIMESTAMP WITH TIME ZONE to be equal when they represent the same UTC timestamp.


# Test case for [CALCITE-7491] https://issues.apache.org/jira/browse/CALCITE-7491
# Literals of type TIMESTAMP WITH TIME ZONE cause crashes
# TODO: thie result should probably be displayed using time-zone
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thie -> this

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: We generally don't add TODOs, it's preferable to file a ticket for Avatica and cite it here IMO

Copy link
Copy Markdown
Member

@asolimando asolimando left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, temporal data types, especially with time zones, are always a headache, thanks for fixing this!

@mihaibudiu
Copy link
Copy Markdown
Contributor Author

I have addressed @asolimando's comments, but I will wait a bit to see whether there are other comments.

!ok

# Two timestamps with time zone are equal if they represent the same UTC time
SELECT TIMESTAMP WITH TIME ZONE '2020-01-01 08:10:10 America/New_York' = TIMESTAMP WITH TIME ZONE '2020-01-01 05:10:10 America/Los_Angeles';
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we add a field here that returns false for the same time but different time zones?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can, but I was planning to leave more complex tests for TIMESTAMP WITH TIME ZONE for a separate PR, including arithmetic, functions, etc.
I will add this one here, since it's similar.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you have this plan, then you don't need to add it; you can supplement it with more comprehensive testing in the new PR.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See last commit.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

Signed-off-by: Mihai Budiu <mbudiu@feldera.com>
@mihaibudiu mihaibudiu added the LGTM-will-merge-soon Overall PR looks OK. Only minor things left. label Apr 28, 2026
@sonarqubecloud
Copy link
Copy Markdown

@mihaibudiu mihaibudiu merged commit ad273f0 into apache:main Apr 29, 2026
22 of 37 checks passed
@mihaibudiu mihaibudiu deleted the issue7491 branch April 29, 2026 02:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

LGTM-will-merge-soon Overall PR looks OK. Only minor things left.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants