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(timestamp): toMicros() should work for all timestamps fitting in bigint #11774

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

boneanxs
Copy link
Contributor

@boneanxs boneanxs commented Dec 6, 2024

As #7506, toMicros() throws because the computation overflows when multiplying
negative number then add positive number. In this case the final result still
fits in int64_t. This PR fixes this issue by using int128_t to make sure the
computation does not overflows.

@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Dec 6, 2024
Copy link

netlify bot commented Dec 6, 2024

Deploy Preview for meta-velox canceled.

Name Link
🔨 Latest commit a8ca595
🔍 Latest deploy log https://app.netlify.com/sites/meta-velox/deploys/675a9128e1adc00008c4125b

velox/type/Timestamp.h Outdated Show resolved Hide resolved
velox/type/Timestamp.h Outdated Show resolved Hide resolved
@boneanxs boneanxs changed the title Fix toMicros() should work for all spark timestamps fix(timestamp): toMicros() should work for all spark timestamps Dec 12, 2024
@rui-mo rui-mo changed the title fix(timestamp): toMicros() should work for all spark timestamps fix(timestamp): toMicros() should work for all Spark timestamps Dec 12, 2024
@rui-mo rui-mo changed the title fix(timestamp): toMicros() should work for all Spark timestamps fix(timestamp): toMicros() should work for all timestamps Dec 12, 2024
@rui-mo rui-mo changed the title fix(timestamp): toMicros() should work for all timestamps fix(timestamp): toMicros() should work for all timestamps fitting in bigint Dec 12, 2024
Copy link
Collaborator

@rui-mo rui-mo left a comment

Choose a reason for hiding this comment

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

Thanks!

velox/type/Timestamp.h Outdated Show resolved Hide resolved
@rui-mo
Copy link
Collaborator

rui-mo commented Dec 12, 2024

@laithsakka @kagamiori Would you please help review this fix similar to toMillis()? Thanks!

@boneanxs
Copy link
Contributor Author

@laithsakka @kagamiori @pedroerp Hey, could you please help review this? Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants