Repository navigation
Conversation
|
@breken-ai Can you fix the merge conflict? |
rambleraptor
left a comment
There was a problem hiding this comment.
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: |
There was a problem hiding this comment.
Could you also add a test for 2.0? That should also pass
There was a problem hiding this comment.
Added in c4489d7. 2.0 (and 2, 2.00, -2.0) has no fractional part, so it still converts and filters exactly:
tests/expressions/test_literals.py::test_integral_decimal_to_integral_typeis now parametrized over"2","2.0","2.00","-2.0"for bothIntegerTypeandLongType, and checks each one becomesLongLiteral(int(value)).tests/catalog/test_catalog_behaviors.py::test_scan_integer_column_with_decimal_literalchecksx > 2.0returns[3, 4]and now alsox = 2.0returns[2], on the memory, sql and sql_without_rowcount catalogs.
The 2.0 cases pass on both main and this branch, as you expected. The fractional cases (2.5, 2.6, -2.5, 0.1, and the x > 2.6 scan) fail on main and pass here: 11 failed / 8 passed with main's literals.py, and 19 passed with the fix. pytest tests/expressions tests/catalog/test_catalog_behaviors.py: 1307 passed. ruff check and ruff format --check are clean.
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>
f848e98 to
c4489d7
Compare
|
@Fokko Fixed. I rebased onto |
Rationale for this change
The expression parser reads an unquoted number with a decimal point as a
DecimalLiteral. When that literal is bound to anintorlongcolumn,DecimalLiteral.to(IntegerType/LongType)rounds it withto_integral_value()(half-even), which changes the predicate and makes a scan return wrong rows: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 wayDecimalLiteral.to(DecimalType)already rejects a mismatched scale, andpartition_to_pyrejects fractional digits for integer partitions. Integral decimals such as2.00still convert, and out-of-range values still becomeIntAboveMax/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 asx < 2), buttest_string_literalassertsliteral("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) andtest_integral_decimal_to_integral_type.tests/catalog/test_catalog_behaviors.py:test_scan_integer_column_with_decimal_literalappends to a real table (memory and SQL catalogs) and checks thatx > 2.0returns[3, 4]andx > 2.6raises instead of returning[4].On
mainthe 11 new fractional cases fail withDID 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.pyandtests/io/test_pyarrow_visitor.pypass.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
ValueErrorinstead 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.