Skip to content

fix: SMALLINT addition overflow should error instead of silently wrapping#21319

Open
xiedeyantu wants to merge 5 commits intoapache:mainfrom
xiedeyantu:overflow
Open

fix: SMALLINT addition overflow should error instead of silently wrapping#21319
xiedeyantu wants to merge 5 commits intoapache:mainfrom
xiedeyantu:overflow

Conversation

@xiedeyantu
Copy link
Copy Markdown
Member

Which issue does this PR close?

Rationale for this change

DataFusion was silently wrapping on integer arithmetic overflow instead of returning an error. For example:

SELECT CAST(32767 AS SMALLINT) + CAST(2 AS SMALLINT);
-- Before: -32767  (silent wrapping)
-- After:  Error: Arithmetic overflow: Overflow happened on: 32767 + 2

This behavior is incorrect. DuckDB and PostgreSQL both return an error on integer overflow, which is the expected behavior for SQL engines.

What changes are included in this PR?

  • BinaryExpr::new (binary.rs): Change the default value of fail_on_overflow from false to true. This causes +, -, and * on integer types to use Arrow's checked kernels instead of the wrapping variants, returning an error on overflow.
  • Update snapshot assertions in dynamic_filters.rs and physical_planner.rs to reflect the new default.
  • Update explain_analyze.slt to use slt:ignore for output_bytes on integer arithmetic projections, since Arrow's checked kernels may allocate a validity buffer, slightly changing the reported byte size.
  • Add sqllogictest cases in operator.slt covering overflow for TINYINT, SMALLINT, INT, and BIGINT with +, -, and *.

Are these changes tested?

Yes.

  • New query error tests in operator.sltverify that overflow on all integer widths (Int8, Int16, Int32, Int64) returns an error for addition, subtraction, and multiplication.
  • Existing tests test_add_with_overflow, test_subtract_with_overflow, and test_mul_with_overflowin binary.rscontinue to pass (they already used with_fail_on_overflow(true)explicitly).

Are there any user-facing changes?

Yes. Integer arithmetic (+, -, *) on types TINYINT, SMALLINT, INT, and BIGINT now returns an error on overflow instead of silently wrapping. This aligns DataFusion's behavior with DuckDB and PostgreSQL.

@github-actions github-actions bot added physical-expr Changes to the physical-expr crates core Core DataFusion crate sqllogictest SQL Logic Tests (.slt) labels Apr 2, 2026
@github-actions github-actions bot added sql SQL Planner common Related to common crate labels Apr 3, 2026
@github-actions github-actions bot added the documentation Improvements or additions to documentation label Apr 3, 2026
@xiedeyantu
Copy link
Copy Markdown
Member Author

The datafusion-testing repository should be updated to align with this change. I have forked the datafusion-testing project on my GitHub and submitted a apache/datafusion-testing#18. I'm not sure about the merging process. The current step is to update the submodule using the datafusion-testing PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

common Related to common crate core Core DataFusion crate documentation Improvements or additions to documentation physical-expr Changes to the physical-expr crates sql SQL Planner sqllogictest SQL Logic Tests (.slt)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

SMALLINT addition overflow should error instead of silently wrapping

2 participants