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

[CALCITE-6685] There is no support for checked arithmetic #4044

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

mihaibudiu
Copy link
Contributor

This implementation adds support for checked arithmetic in Calcite.
This solves some bugs which are at least 12 years old in Calcite, such as FNL_25.
This implementation only handles pure arithmetic. Aggregates are not handled currently, but using the existing framework it should become very easy.

The core idea is as follows:

  • add a set of checked arithmetic operators to StdSqlOperatorTable. There are five: plus, minus, divide, times, and unary minus.
  • add a new boolean property to the SqlConformance API: checkedArithmetic().
  • if this property is true, the Prepare class will use a RelShuttle/RexShuttle to replace all arithmetic operations with their checked counterparts.

The handling of Decimals is a bit different: there is not checked arithmetic for decimals, but casts to decimals perform overflow checking. As such, casts to decimals may not be elided from the program even if they seem noops (the input type matches the output type).

@mihaibudiu
Copy link
Contributor Author

This design is based on a discussion on the mailing list. Many thanks to all the people who helped drive this design.

Copy link

sonarcloud bot commented Nov 12, 2024

@caicancai
Copy link
Member

This design is based on a discussion on the mailing list. Many thanks to all the people who helped drive this design.

Looks really cool. Could you please post the link to the discussion on the mailing list so people can know the whole story?

@mihaibudiu
Copy link
Contributor Author

This is the email thread: https://lists.apache.org/[email protected]:2024-10:checked

@caicancai
Copy link
Member

This is the email thread: https://lists.apache.org/[email protected]:2024-10:checked

Thanks. I'm busy with work recently, I will review this PR on the weekend

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.

2 participants