Skip to content

feat: Add mathematical operators for IntervalYearMonth type #11612

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.

Sign up for GitHub

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

Jump to bottom
Closed
pramodsatya wants to merge 1 commit into from
Closed

feat: Add mathematical operators for IntervalYearMonth type #11612

pramodsatya wants to merge 1 commit into from

Conversation

@pramodsatya
Copy link
Collaborator

@pramodsatya pramodsatya commented Nov 21, 2024 •
edited
Loading

Add support for mathematical functions plus, minus, multiply, and divide
with IntervalYearMonth type. The function signatures added match that of Presto,
accounting for the Presto function signature changes in prestodb/presto#24089.

@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 Nov 21, 2024
@netlify
Copy link

netlify bot commented Nov 21, 2024 •
edited
Loading

✅ Deploy Preview for meta-velox canceled.

Name Link
🔨 Latest commit 6158737
🔍 Latest deploy log https://app.netlify.com/sites/meta-velox/deploys/67f870a0860c8200080b2296

aditi-pandit
aditi-pandit reviewed Nov 25, 2024
View reviewed changes
Copy link
Collaborator

@aditi-pandit aditi-pandit left a comment

Choose a reason for hiding this comment

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

Thanks @pramodsatya.

We can use more templates in this code I feel to reduce the volume of new code. PTAL.

@pramodsatya
Copy link
Collaborator Author

pramodsatya commented Dec 11, 2024

Thanks @aditi-pandit, updated the PR to use more templates and reduce the lines of new code. Could you please take another look?

aditi-pandit
aditi-pandit reviewed Jan 2, 2025
View reviewed changes
Copy link
Collaborator

@aditi-pandit aditi-pandit left a comment

Choose a reason for hiding this comment

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

@pramodsatya : The code changes look good.

Is this code tested exhaustively by the expression fuzzer ? Also with the PrestoQueryRunner ? Would be great if you could paste the results.

velox/functions/prestosql/Arithmetic.h Outdated
if constexpr (std::is_same_v<TResult, int64_t>) {
min = kLongMin;
max = kLongMax;
maxCheck = kMaxDoubleBelowInt64Max;
Copy link
Collaborator

@aditi-pandit aditi-pandit Dec 17, 2024

Choose a reason for hiding this comment

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

Rename to maxResult.

@pramodsatya
Copy link
Collaborator Author

pramodsatya commented Jan 22, 2025

@pramodsatya : The code changes look good.

Is this code tested exhaustively by the expression fuzzer ? Also with the PrestoQueryRunner ? Would be great if you could paste the results.

Thanks @aditi-pandit, the expression fuzzer CI failure is because the newly added function signature for plus is not supported by Presto yet. Once the linked Presto PR is merged, advancing the Presto version used by PrestoQueryRunner should fix the fuzzer CI error. Upon running the expression fuzzer locally on the main branch, I encountered other errors (checking further).

@aditi-pandit
Copy link
Collaborator

aditi-pandit commented Jan 23, 2025

@pramodsatya : Can you ping Tim about the Presto PR ?

@libianoss
Copy link
Collaborator

libianoss commented Feb 24, 2025

@aditi-pandit @pramodsatya can anyone else review and approve this PR since Tim is out? Thanks.

@pramodsatya pramodsatya mentioned this pull request Feb 25, 2025
Merged
@pramodsatya
Copy link
Collaborator Author

pramodsatya commented Mar 22, 2025

@aditi-pandit, the expression fuzzer CI is passing now, could you please take another look?

aditi-pandit
aditi-pandit approved these changes Mar 24, 2025
View reviewed changes
Copy link
Collaborator

@aditi-pandit aditi-pandit left a comment

Choose a reason for hiding this comment

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

Thanks @pramodsatya

@aditi-pandit aditi-pandit requested a review from Yuhta March 24, 2025 19:18
@aditi-pandit
Copy link
Collaborator

aditi-pandit commented Mar 24, 2025

@Yuhta : Please can you help review.

@libianoss
Copy link
Collaborator

libianoss commented Mar 31, 2025

@Yuhta : can you help to review?Thanks.

majetideepak
majetideepak reviewed Apr 7, 2025
View reviewed changes
@majetideepak majetideepak added the ready-to-merge PR that have been reviewed and are ready for merging. PRs with this tag notify the Velox Meta oncall label Apr 8, 2025
@facebook-github-bot
Copy link
Contributor

facebook-github-bot commented Apr 14, 2025

@kKPulla has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

@facebook-github-bot
Copy link
Contributor

facebook-github-bot commented Apr 17, 2025

@kKPulla merged this pull request in 53e2684.

@prestodb-ci prestodb-ci mentioned this pull request Apr 17, 2025
Closed
1 task
@pramodsatya pramodsatya deleted the ym_multiply branch April 17, 2025 16:57
zhanglistar pushed a commit to bigo-sg/velox that referenced this pull request Apr 22, 2025
@pramodsatya @zhanglistar
feat: Add mathematical operators for IntervalYearMonth type (facebook…
7059c7d 
…incubator#11612)

Summary:
Add support for mathematical functions `plus`, `minus`, `multiply`, and `divide`
with `IntervalYearMonth` type. The function signatures added match that of [Presto](https://github.com/prestodb/presto/blob/master/presto-main/src/main/java/com/facebook/presto/type/IntervalYearMonthOperators.java),
accounting for the Presto function signature changes in prestodb/presto#24089.

Pull Request resolved: facebookincubator#11612

Reviewed By: pedroerp

Differential Revision: D72997442

Pulled By: kKPulla

fbshipit-source-id: 5fcf40639ed0af63e6ec198994aeaef49ba367cb
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@majetideepak majetideepak majetideepak approved these changes

@aditi-pandit aditi-pandit aditi-pandit approved these changes

@Yuhta Yuhta Awaiting requested review from Yuhta

Assignees

No one assigned

Labels

CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. Merged ready-to-merge PR that have been reviewed and are ready for merging. PRs with this tag notify the Velox Meta oncall

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@pramodsatya @aditi-pandit @libianoss @facebook-github-bot @majetideepak