Repository navigation
GH-46557: [C++][R] Check overflow in float to int casts that allow truncation - #52469
Open
advitrocks9 wants to merge 4 commits into
Open
advitrocks9 wants to merge 4 commits into
advitrocks9 wants to merge 4 commits into
Conversation
|
|
github-actions
Bot
added
Component: R
Component: C++
awaiting review
|
|
|
|
Comment on lines
+234
to
+235
| } else if (!options.allow_int_overflow) { | ||
| RETURN_NOT_OK(CheckFloatToInt<WasOutOfRange>(batch[0], *out)); |
Comment on lines
+131
to
+132
| return Status::Invalid("Float value ", val, " out of range converting to ", | ||
| *output.type); |
Member
|
Thanks for picking up this PR @advitrocks9! I've triggered a Copilot review and asked one of our C++ maintainers to take a look at this. |
Contributor
Author
|
Fixed copilot's concerns |
This file contains hidden or 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
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.
Rationale for this change
A float to int cast with
allow_float_truncate=trueandallow_int_overflow=falsenever checks the range. Out of range values come back as garbage instead of an error. Casting float64 to int32 on x86-64:The same values cast from int64 fail with
Integer value 3000000000 not in range.What changes are included in this PR?
Once truncation is allowed,
CastFloatingToIntegeronly checks for truncation, so nothing readsallow_int_overflow. This adds aWasOutOfRangecheck next toWasTruncatedand runs it in that case, through the same block loop. NaN and infinities count as out of range.The default safe cast is unchanged.
R's integer
%/%casts every row, including rows it then masks, so it now also passesallow_int_overflow = TRUEto keepx %/% 0Lreturning NA.Are these changes tested?
Yes.
Cast.FloatingToIntOverflowcovers int8, uint8 and the 64-bit edges for float16, float32 and float64, with and without truncation. It fails without the fix.arrow-compute-scalar-cast-testpasses under ASAN and UBSAN. I couldn't run the R tests locally.Are there any user-facing changes?
Yes. These casts now raise on out of range, NaN and infinite values, including R's
as.integer()andas.integer64()in dplyr queries.This PR contains a "Critical Fix". These casts silently produced incorrect values.
allow_int_overflowoption ignored whenallow_float_truncateset to True #46557