GH-46557: [C++][R] Check overflow in float to int casts that allow truncation by advitrocks9 · Pull Request #52469 · apache/arrow

advitrocks9 · 2026-10-09T01:20:12Z

Rationale for this change

With allow_float_truncate=true and allow_int_overflow=false, a float to int cast never checks the range, so out of range values silently come back as garbage. Casting 1e308, 3e9 or NaN from float64 to int32 returns -2147483648.

What changes are included in this PR?

Adds a WasOutOfRange check for that case, which rejects out of range values before they are converted. R's %/% passes allow_int_overflow = TRUE, since it casts rows it then masks.

Are these changes tested?

Cast.FloatingToIntOverflow covers float16, float32 and float64 to int8, uint8, int64 and uint64, with and without truncation, including an out of range value in a null slot. It fails without the fix.

Are there any user-facing changes?

These casts raise on out of range, NaN and infinite values, including R's as.integer().

This PR contains a "Critical Fix". These casts silently returned wrong values.

GitHub Issue: [C++] During casting, allow_int_overflow option ignored when allow_float_truncate set to True #46557

…low truncation

Copilot

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

github-actions · 2026-10-09T01:20:37Z

⚠️ GitHub issue #46557 has been automatically assigned in GitHub to PR creator.

github-actions · 2026-10-09T01:29:49Z

⚠️ GitHub issue #46557 has been automatically assigned in GitHub to PR creator.

Copilot

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

github-actions · 2026-10-09T01:42:30Z

⚠️ GitHub issue #46557 has been automatically assigned in GitHub to PR creator.

Copilot

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

Copilot

🟡 Changes recommended

The safe path still performs undefined out-of-range conversions before validating them, and half-float diagnostics expose raw bit patterns.

2 open findings

🧠 Review effort: Balanced

+  } else if (!options.allow_int_overflow) {
+    RETURN_NOT_OK(CheckFloatToInt<WasOutOfRange>(batch[0], *out));


+      return Status::Invalid("Float value ", val, " out of range converting to ",
+                             *output.type);


thisisnic · 2026-10-11T17:19:42Z

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.

Copilot

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

advitrocks9 · 2026-10-11T18:20:13Z

Fixed copilot's concerns

dmitry-chirkov-dremio

The WasOutOfRange/Checker approach looks useful. Could we also use the range check before conversion on the checked float-to-integer paths, including when allow_float_truncate is false?

The current order still performs an undefined conversion before reporting the error. Adding ARROW_DISABLE_UBSAN to the half-float path suppresses that check rather than avoiding the invalid conversion. Please also cover null slots containing out-of-range values.

There may be an opportunity to share the validation loop with CheckIntegersInRange, but I would leave that cleanup and the similar Gandiva casts to separate work.

…on is allowed

Copilot

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

advitrocks9 · 2026-10-22T02:08:16Z

Thanks! I've committed the fixes.

For allow_float_truncate=false I kept the round trip, since an input-only check was 1.6-2x slower. I also kept the half float exemption for null slots and unchecked casts, same as float and double.

github-actions · 2026-10-22T02:10:43Z

⚠️ GitHub issue #46557 has been automatically assigned in GitHub to PR creator.

dmitry-chirkov-dremio · 2026-10-22T07:40:44Z

1.6-2x slower

Against what baseline? "The before" state or the 4th commit?
Also please re-check if PR description drifted from the actual code changes.

pitrou · 2026-10-22T13:21:46Z

+    for (bool allow_float_truncate : {false, true}) {
+      auto opts = CastOptions::Safe(int8());
+      opts.allow_float_truncate = allow_float_truncate;
+      for (auto json : {"[128.0]", "[-129.0]", "[NaN]", "[Inf]", "[-Inf]"}) {


Can we also test 127.5 and -128.5 when truncation is disabled?

pitrou · 2026-10-22T13:22:07Z

+      CheckCastFails(ArrayFromJSON(from, "[256.0]"), opts);
+      CheckCastFails(ArrayFromJSON(from, "[-1.0]"), opts);


What about 255.0 and -0.5?

pitrou · 2026-10-22T13:23:13Z

+    auto opts = CastOptions::Safe(int64());
+    opts.allow_float_truncate = true;
+    CheckCastFails(ArrayFromJSON(from, "[9223372036854775808.0]"), opts);
+    CheckCast(ArrayFromJSON(from, "[-9223372036854775808.0]"),


What about 9223372036854775807, 9223372036854775807.5, -9223372036854775808.5?

pitrou · 2026-10-22T13:23:40Z

+  EXPECT_RAISES_WITH_MESSAGE_THAT(Invalid, ::testing::HasSubstr("Float value 1.5"),
+                                  Cast(ArrayFromJSON(float16(), "[1.5]"), opts));
+  opts.allow_float_truncate = true;
+  EXPECT_RAISES_WITH_MESSAGE_THAT(Invalid, ::testing::HasSubstr("Float value inf out"),


What does "inf out" mean?

pitrou · 2026-10-22T13:25:02Z

 template <typename InType, typename OutType, typename InT = typename InType::c_type,
          typename OutT = typename OutType::c_type>
+struct WasOutOfRange {
+  static constexpr auto kMin = static_cast<InT>(std::numeric_limits<OutT>::min());


These seems to assume that OutT is integral, can we add a static_assert about that?

pitrou · 2026-10-22T13:27:09Z

+    if constexpr (std::is_same_v<Checker<InType, OutType>,
+                                 WasOutOfRange<InType, OutType>>) {


Can you please add a relevant method on the various checkers? It doesn't make sense to put type specialization here.

pitrou · 2026-10-22T13:29:00Z

+  auto out_at = [&](int64_t i) {
+    if constexpr (std::is_same_v<Checker<InType, OutType>,
+                                 WasOutOfRange<InType, OutType>>) {
+      return OutT{};
+    } else {
+      return out_data[i];
+    }
+  };


pitrou · 2026-10-22T13:30:37Z

 Status CastFloatingToInteger(KernelContext* ctx, const ExecSpan& batch, ExecResult* out) {
  const auto& options = checked_cast<const CastState*>(ctx->state())->options;
+  if (options.allow_float_truncate && !options.allow_int_overflow) {
+    RETURN_NOT_OK(CheckFloatToInt<WasOutOfRange>(batch[0], *out));


Why check before while allow_float_truncate checks after the conversion?

thisisnic · 2026-10-23T11:46:17Z

I've dismissed myself as a review so I don't get notifications on this, but once we're happy with the C++, mind either pinging me to take a look at the R side of things or splitting it out into a follow-up @advitrocks9?

apacheGH-46557: [C++][R] Check overflow in float to int casts that al…

d490bd6

…low truncation

Copilot AI balanced review requested due to automatic review settings October 9, 2026 01:20

advitrocks9 requested review from jonkeane and thisisnic as code owners October 9, 2026 01:20

Copilot AI reviewed Oct 9, 2026

github-actions Bot added Component: R Component: C++ awaiting review Awaiting review labels Oct 9, 2026

Keep safe cast error messages unchanged

d255ed4

Copilot AI balanced review requested due to automatic review settings October 9, 2026 01:40

Copilot AI reviewed Oct 9, 2026

Disable float-cast-overflow sanitizer in half float to int cast

a7a29ee

Copilot AI balanced review requested due to automatic review settings October 9, 2026 02:35

Copilot AI reviewed Oct 9, 2026

thisisnic requested review from pitrou and a balanced review from Copilot October 11, 2026 17:08

Copilot started reviewing on behalf of thisisnic October 11, 2026 17:09

View session

Copilot AI reviewed Oct 11, 2026

View reviewed changes

Decode half float values in cast error messages

95a2688

Copilot AI balanced review requested due to automatic review settings October 11, 2026 18:19

Copilot AI reviewed Oct 11, 2026

dmitry-chirkov-dremio reviewed Oct 21, 2026

View reviewed changes

github-actions Bot added awaiting committer review Awaiting committer review and removed awaiting review Awaiting review labels Oct 21, 2026

apacheGH-46557: [C++] Check the range before converting when truncati…

f2a1baa

…on is allowed

Copilot AI balanced review requested due to automatic review settings October 22, 2026 01:58

Copilot AI reviewed Oct 22, 2026

advitrocks9 requested a review from dmitry-chirkov-dremio October 22, 2026 02:08

pitrou requested changes Oct 22, 2026

View reviewed changes

thisisnic removed request for dmitry-chirkov-dremio and thisisnic October 23, 2026 11:44

		} else if (!options.allow_int_overflow) {
		RETURN_NOT_OK(CheckFloatToInt<WasOutOfRange>(batch[0], *out));

		return Status::Invalid("Float value ", val, " out of range converting to ",
		*output.type);

		CheckCastFails(ArrayFromJSON(from, "[256.0]"), opts);
		CheckCastFails(ArrayFromJSON(from, "[-1.0]"), opts);

		if constexpr (std::is_same_v<Checker<InType, OutType>,
		WasOutOfRange<InType, OutType>>) {

Conversation

advitrocks9 commented Oct 9, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Rationale for this change

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Uh oh!

github-actions Bot commented Oct 9, 2026

Uh oh!

github-actions Bot commented Oct 9, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Uh oh!

github-actions Bot commented Oct 9, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🟡 Changes recommended

Uh oh!

thisisnic commented Oct 11, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Uh oh!

advitrocks9 commented Oct 11, 2026

Uh oh!

dmitry-chirkov-dremio left a comment

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Uh oh!

advitrocks9 commented Oct 22, 2026

Uh oh!

github-actions Bot commented Oct 22, 2026

Uh oh!

dmitry-chirkov-dremio commented Oct 22, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

thisisnic commented Oct 23, 2026

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

advitrocks9 commented Oct 9, 2026 •

edited

Loading

dmitry-chirkov-dremio commented Oct 22, 2026 •

edited

Loading