Throw exception for cast of nan and infinity to int types #22917
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
ClarenceThreepwood
previously approved these changes
Jun 4, 2024
Another data point - this is also the approach that most other db engines take (e.g. Postgres) |
elharo
requested changes
Jun 5, 2024
| } | ||
| catch (ArithmeticException e) { | ||
| throw new PrestoException(NUMERIC_VALUE_OUT_OF_RANGE, "Out of range for integer: " + value, e); | ||
| if (e.getMessage().equals("not in range")) { |
There was a problem hiding this comment.
How can this be done without relying on undocumented exception messages that are subject to change between JDKs and JDK versions?
If it can't, I'd drop the conditional and simply always use an INVALID_CAST_ARGUMENT
There was a problem hiding this comment.
yeah, I didn't love it, but didn't see other options.. I'll switch to always using INVALID_CAST_ARGUMENT instead.
| return Shorts.checkedCast(DoubleMath.roundToInt(value, HALF_UP)); | ||
| } | ||
| catch (ArithmeticException e) { | ||
| if (e.getMessage().equals("not in range")) { |
| return SignedBytes.checkedCast(DoubleMath.roundToInt(value, HALF_UP)); | ||
| } | ||
| catch (ArithmeticException e) { | ||
| if (e.getMessage().equals("not in range")) { |
| return DoubleMath.roundToLong(intBitsToFloat((int) value), HALF_UP); | ||
| } | ||
| catch (ArithmeticException e) { | ||
| if (e.getMessage().equals("not in range")) { |
| } | ||
| catch (ArithmeticException e) { | ||
| throw new PrestoException(NUMERIC_VALUE_OUT_OF_RANGE, "Out of range for integer: " + value, e); | ||
| if (e.getMessage().equals("not in range")) { |
Fix cast of nan and infinity from DOUBLE/REAL to BIGINT/INT/SMALLINT/TINYTINT. Previously all except double -> bigint would return zero. Now they will all throw an INVALID_CAST_ARGUMENT exception. Fix silent overflow for casting from REAL to BIGINT type. We now throw an INVALID_CAST_ARGUMENT if the value is out of the BIGINT range. Change error code from NUMERIC_VALUE_OUT_OF_RANGE to INVALID_CAST_ARGUMENT for out of range values in casts from floating point to integer types. With the library we now use for the implementaiton for these casts, we can't always tell by the exception type that the value is out of range, so we return the more generic, but still relevant INVALID_CAST_ARGUMENT instead.
elharo
approved these changes
Jun 5, 2024
arhimondr
approved these changes
Jun 5, 2024
36 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
Fix cast of nan and infinity from DOUBLE/REAL to
BIGINT/INT/SMALLINT/TINYTINT. Previously all except double -> bigint would return zero. Now they will all throw an INVALID_CAST_ARGUMENT exception.
Fix silent overflow for casting from REAL to BIGINT type. We now throw
an INVALID_CAST_ARGUMENT if the value is out of the BIGINT range.
Change error code from NUMERIC_VALUE_OUT_OF_RANGE to
INVALID_CAST_ARGUMENT for out of range values in casts from floating
point to integer types. With the library we now use for the
implementaiton for these casts, we can't always tell by the exception
type that the value is out of range, so we return the more generic, but
still relevant INVALID_CAST_ARGUMENT instead.
Motivation and Context
Fixes #22910.
Nan and infinity can't be represented as an integer, so casting to an int type should throw an error.
This also brings Presto and velox behavior in alignment. We are choosing to modify Presto in this case because velox behavior is correct here (we would want to change Presto regardless of the native worker migration)
Impact
CAST of nan and infinity to BIGINT, INTEGER, SMALLINT, and TINYINT types will now return an exception with the INVALID_CAST_ARGUMENT error code
Cast of out of range values for DOUBLE or FLOAT to BIGINT, INTEGER, SMALLINT, or TINYINT will now return an INVALID_CAST_ARGUMENT error code rather than NUMERIC_VALUE_OUT_OF_RANGE error code
CAST of real values outside of BIGINT range will now return a NUMERIC_VALUE_OUT_OF_RANGE error. Previously they would silently overflow.
Test Plan
unit tests
Contributor checklist
Release Notes
Please follow release notes guidelines and fill in the release notes below.