Skip to content

[CALCITE-7410] TIMESTAMP type for TUMBLE and HOP is hardwired to TIMESTAMP(3)#4799

Merged
mihaibudiu merged 1 commit intoapache:mainfrom
mihaibudiu:issue7410
Feb 25, 2026
Merged

[CALCITE-7410] TIMESTAMP type for TUMBLE and HOP is hardwired to TIMESTAMP(3)#4799
mihaibudiu merged 1 commit intoapache:mainfrom
mihaibudiu:issue7410

Conversation

@mihaibudiu
Copy link
Contributor

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

Given the way Calcite implements TIMESTAMPS in the runtime (representing them as an integer number of milliseconds), it's virtually impossible to write a test that proves that this fix allows computation on microseconds. However, our runtime does support microseconds, and I have used it to test that this change indeed works for higher precision timestamps.

@xiedeyantu
Copy link
Member

#2176 is a reverse operation, and I don't quite understand this part. I'd like to ask if a default value is needed to determine the precision? And should different systems configure this themselves?

@mihaibudiu
Copy link
Contributor Author

I will try to rework the PR to actually preserve explicitly the type of the original timestamp column. That's a better solution. I thought that this is what my PR does... but perhaps it's not.

@mihaibudiu
Copy link
Contributor Author

@xiedeyantu thank you for pointing out the previous PR. I think that the approach in this PR's second commit is better. (My previous version was indeed just reverting it). I retrieve the original timestamp column and I preserve it's precision. I think this is the approach proposed by @julianhyde in the original issue, which wasn't actually correctly implemented.

Copy link
Member

@xiedeyantu xiedeyantu left a comment

Choose a reason for hiding this comment

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

This version looks much better, and I don’t have any further questions. We can also see if others have any.

@mihaibudiu mihaibudiu added the LGTM-will-merge-soon Overall PR looks OK. Only minor things left. label Feb 19, 2026
@mihaibudiu
Copy link
Contributor Author

I will squash the commits but wait a bit to see if there are other comments.

@mihaibudiu
Copy link
Contributor Author

This is actually a breaking change, I wonder whether I should make it at all. I think that the existing implementation is wrong, though.

@mihaibudiu
Copy link
Contributor Author

@danny0405 do you have any comments about this PR? It modifies the changes you made in your PR.

@mihaibudiu
Copy link
Contributor Author

I documented this as a breaking change. We can take it, or we can skip it. Let's see if other users have comments.
One can work around it, but it is not easy at all, because TUMBLE, HOP and SESSION are in the standard operator table, so they are not optional. (I don't think they should be there.)

…STAMP(3)

Signed-off-by: Mihai Budiu <mbudiu@feldera.com>
@sonarqubecloud
Copy link

@xiedeyantu
Copy link
Member

Many days have passed without any additional comments, so it feels like we can merge them now.

@mihaibudiu
Copy link
Contributor Author

Ok, I will do it. I just squashed the commits today.

@mihaibudiu mihaibudiu merged commit e7e3925 into apache:main Feb 25, 2026
21 checks passed
@mihaibudiu mihaibudiu deleted the issue7410 branch February 25, 2026 00:46
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.

2 participants