Skip to content

fix(expressions): reject fractional decimal literals on integer columns - #4029

Open
breken-ai wants to merge 1 commit into
apache:mainfrom
breken-ai:fix/reject-fractional-integer-literals
Open

breken-ai wants to merge 1 commit into
apache:mainfrom
breken-ai:fix/reject-fractional-integer-literals

Conversation

@breken-ai

Copy link
Copy Markdown
Contributor

Rationale for this change

The expression parser reads an unquoted number with a decimal point as a DecimalLiteral. When that literal is bound to an int or long column, DecimalLiteral.to(IntegerType/LongType) rounds it with to_integral_value() (half-even), which changes the predicate and makes a scan return wrong rows:

tbl.append(pa.table({"x": pa.array([1, 2, 3, 4], pa.int32())}))

tbl.scan(row_filter="x > 2.6").to_arrow()["x"]  # [4]     expected [3, 4]  (bound as x > 3)
tbl.scan(row_filter="x < 2.5").to_arrow()["x"]  # [1]     expected [1, 2]  (bound as x < 2)
tbl.scan(row_filter="x = 2.5").to_arrow()["x"]  # [2]     expected []      (bound as x = 2)
tbl.scan(row_filter=GreaterThan("x", Decimal("2.6")))  # same as x > 3

The same rewritten predicate is used for partition and metrics pruning, so this also affects delete()/overwrite() with such a filter.

This PR makes the conversion raise a ValueError (Could not convert 2.6 into a int, value has a fractional part) when the decimal has a fractional part, the same way DecimalLiteral.to(DecimalType) already rejects a mismatched scale, and partition_to_py rejects fractional digits for integer partitions. Integral decimals such as 2.00 still convert, and out-of-range values still become IntAboveMax/IntBelowMin. Java has no decimal-to-integer literal conversion at all, so binding fails there too.

StringLiteral.to(IntegerType) truncates quoted values the same way (x < '2.5' binds as x < 2), but test_string_literal asserts literal("3.141").to(IntegerType()) == literal(3), so I left that path alone. Happy to follow up if you'd like it changed too.

Are these changes tested?

Yes.

  • tests/expressions/test_literals.py: test_fractional_decimal_to_integral_type_raises (2.5, 2.6, -2.5, 0.1 for int and long) and test_integral_decimal_to_integral_type.
  • tests/catalog/test_catalog_behaviors.py: test_scan_integer_column_with_decimal_literal appends to a real table (memory and SQL catalogs) and checks that x > 2.0 returns [3, 4] and x > 2.6 raises instead of returning [4].

On main the 11 new fractional cases fail with DID NOT RAISE ValueError. With the fix, all 37 selected tests pass. tests/expressions, tests/test_conversions.py, tests/catalog/test_catalog_behaviors.py, tests/catalog/test_sql.py, tests/io/test_pyarrow.py and tests/io/test_pyarrow_visitor.py pass. prek run --files (ruff, ruff-format, mypy, pydocstyle, codespell) passes.

Are there any user-facing changes?

Yes. A filter that compares an integer column with a fractional number now raises a ValueError instead of silently returning wrong rows. Filters with integral numbers are unchanged.

AI disclosure: this bug was found, fixed and tested by an AI coding agent (Claude) running under the breken-ai account; the red/green runs above are its local results.

DecimalLiteral.to(IntegerType/LongType) rounded the value with
to_integral_value(), so an unquoted filter such as "x > 2.6" on an int
column was bound as x > 3 and silently dropped x = 3; "x < 2.5" became
x < 2 and "x = 2.5" matched x = 2. Raise a ValueError for a value with a
fractional part instead, the same way a Decimal with a mismatched scale is
rejected. Integral values such as 2.00 still convert.

Generated-by: Claude Opus 5.5

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

@Fokko Fokko left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great catch @breken-ai 👍

@Fokko
Fokko added this pull request to the merge queue Oct 1, 2026
@github-merge-queue
github-merge-queue Bot removed this pull request from the merge queue due to a conflict with the base branch Oct 1, 2026
@Fokko

Fokko commented Oct 1, 2026

Copy link
Copy Markdown
Contributor

@breken-ai Can you fix the merge conflict?

@rambleraptor rambleraptor left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This might be too late, but I had a quick idea for a test. Otherwise, looks great!

tbl.append("not an arrow object")


def test_scan_integer_column_with_decimal_literal(catalog: Catalog) -> None:

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you also add a test for 2.0? That should also pass

This branch has not been deployed

No deployments
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.

3 participants