You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@datafusion.apache.org by "1fanwang (via GitHub)" <gi...@apache.org> on 2026/10/02 08:57:23 UTC
[PR] fix: keep decimal fractions in modulo by one [datafusion]
1fanwang opened a new pull request, #25961:
URL: https://github.com/apache/datafusion/pull/25961
## Which issue does this PR close?
- Closes https://github.com/apache/datafusion/issues/25946.
## Rationale for this change
On a NOT NULL decimal column with a positive scale, col % 1 returns zero for every row: 1.50 % 1 returns 0.00 instead of 0.50, and WHERE col % 1 = 0 keeps rows it should drop. The same expression on a nullable column is correct. The simplifier rewrites a non-null A % 1 to 0 for every type except floats, which holds for integers but not for decimals that carry fractional digits.
## What changes are included in this PR?
The rewrite now applies only when A cannot have a fractional part: integer types and decimals with a scale of 0 or less. Other decimals keep the modulo, and Arrow's remainder kernel computes it at run time. The rule's comment now says so.
## What is the testing strategy for this PR?
math.slt now inserts 1.50 into the existing non-null decimal table and checks c1 % 1 with an integer and a decimal literal 1, plus the WHERE filter. The existing simplifier tests that fold an Int64 column % 1, including with a decimal literal 1, are unchanged.
### Testing Done
The issue's queries in datafusion-cli, built and run the same way on main (4d167a167) and on this branch:
```shell
cat > issue-25946.sql <<'EOF'
CREATE TABLE t (d DECIMAL(10,2) NOT NULL, n DECIMAL(10,2)) AS VALUES (1.50, 1.50);
SELECT d % 1 AS not_null_col, n % 1 AS nullable_col FROM t;
SELECT count(*) AS cnt FROM t WHERE d % 1 = 0;
SELECT d % CAST(1 AS DECIMAL(10,2)) AS dec_literal FROM t;
EOF
cargo build --profile ci -p datafusion-cli
target/ci/datafusion-cli -f issue-25946.sql
```
On main the non-null column returns 0.00 and the filter keeps the row:
```text
DataFusion CLI v55.1.0
0 row(s) fetched.
Elapsed 0.016 seconds.
+--------------+--------------+
| not_null_col | nullable_col |
+--------------+--------------+
| 0.00 | 0.50 |
+--------------+--------------+
1 row(s) fetched.
Elapsed 0.003 seconds.
+-----+
| cnt |
+-----+
| 1 |
+-----+
1 row(s) fetched.
Elapsed 0.004 seconds.
+-------------+
| dec_literal |
+-------------+
| 0.00 |
+-------------+
1 row(s) fetched.
Elapsed 0.001 seconds.
```
On this branch both columns return 0.50 and the filter drops the row:
```text
DataFusion CLI v55.1.0
0 row(s) fetched.
Elapsed 0.017 seconds.
+--------------+--------------+
| not_null_col | nullable_col |
+--------------+--------------+
| 0.50 | 0.50 |
+--------------+--------------+
1 row(s) fetched.
Elapsed 0.004 seconds.
+-----+
| cnt |
+-----+
| 0 |
+-----+
1 row(s) fetched.
Elapsed 0.009 seconds.
+-------------+
| dec_literal |
+-------------+
| 0.50 |
+-------------+
1 row(s) fetched.
Elapsed 0.002 seconds.
```
## Are there any user-facing changes?
Yes. A non-null decimal column with a positive scale now returns its fractional remainder for % 1 instead of zero. No API changes.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: keep decimal fractions in modulo by one [datafusion]
Posted by "1fanwang (via GitHub)" <gi...@apache.org>.
1fanwang commented on code in PR #25961:
URL: https://github.com/apache/datafusion/pull/25961#discussion_r4168152393
##########
datafusion/optimizer/src/simplify_expressions/expr_simplifier.rs:
##########
@@ -828,6 +828,16 @@ impl<'a> Simplifier<'a> {
}
}
+fn modulo_one_is_zero_type(data_type: &DataType) -> bool {
Review Comment:
Done in https://github.com/apache/datafusion/commit/79a19bf016dcaaed2aed2eb8305f088581d9f829.
##########
datafusion/optimizer/src/simplify_expressions/expr_simplifier.rs:
##########
@@ -828,6 +828,16 @@ impl<'a> Simplifier<'a> {
}
}
+fn modulo_one_is_zero_type(data_type: &DataType) -> bool {
Review Comment:
@2010YOUY01 Done in https://github.com/apache/datafusion/commit/a11b3a0bfb2ee37a95a843a5e644c98754c48f41.
##########
datafusion/optimizer/src/simplify_expressions/expr_simplifier.rs:
##########
@@ -828,6 +828,16 @@ impl<'a> Simplifier<'a> {
}
}
+fn modulo_one_is_zero_type(data_type: &DataType) -> bool {
+ match data_type {
+ DataType::Decimal32(_, scale)
+ | DataType::Decimal64(_, scale)
+ | DataType::Decimal128(_, scale)
+ | DataType::Decimal256(_, scale) => *scale <= 0,
+ data_type => !data_type.is_floating(),
Review Comment:
Done in https://github.com/apache/datafusion/commit/79a19bf016dcaaed2aed2eb8305f088581d9f829. It now folds only integer types, as you preferred in the review.
##########
datafusion/optimizer/src/simplify_expressions/expr_simplifier.rs:
##########
@@ -828,6 +828,16 @@ impl<'a> Simplifier<'a> {
}
}
+fn modulo_one_is_zero_type(data_type: &DataType) -> bool {
+ match data_type {
+ DataType::Decimal32(_, scale)
+ | DataType::Decimal64(_, scale)
+ | DataType::Decimal128(_, scale)
+ | DataType::Decimal256(_, scale) => *scale <= 0,
+ data_type => !data_type.is_floating(),
Review Comment:
@2010YOUY01 Done in https://github.com/apache/datafusion/commit/a11b3a0bfb2ee37a95a843a5e644c98754c48f41.
##########
datafusion/optimizer/src/simplify_expressions/expr_simplifier.rs:
##########
@@ -828,6 +828,16 @@ impl<'a> Simplifier<'a> {
}
}
+fn modulo_one_is_zero_type(data_type: &DataType) -> bool {
+ match data_type {
+ DataType::Decimal32(_, scale)
Review Comment:
@2010YOUY01 Done in https://github.com/apache/datafusion/commit/a11b3a0bfb2ee37a95a843a5e644c98754c48f41.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: keep decimal fractions in modulo by one [datafusion]
Posted by "1fanwang (via GitHub)" <gi...@apache.org>.
1fanwang commented on code in PR #25961:
URL: https://github.com/apache/datafusion/pull/25961#discussion_r4168152393
##########
datafusion/optimizer/src/simplify_expressions/expr_simplifier.rs:
##########
@@ -828,6 +828,16 @@ impl<'a> Simplifier<'a> {
}
}
+fn modulo_one_is_zero_type(data_type: &DataType) -> bool {
Review Comment:
Done in https://github.com/apache/datafusion/commit/79a19bf016dcaaed2aed2eb8305f088581d9f829.
##########
datafusion/optimizer/src/simplify_expressions/expr_simplifier.rs:
##########
@@ -828,6 +828,16 @@ impl<'a> Simplifier<'a> {
}
}
+fn modulo_one_is_zero_type(data_type: &DataType) -> bool {
+ match data_type {
+ DataType::Decimal32(_, scale)
+ | DataType::Decimal64(_, scale)
+ | DataType::Decimal128(_, scale)
+ | DataType::Decimal256(_, scale) => *scale <= 0,
+ data_type => !data_type.is_floating(),
Review Comment:
Done in https://github.com/apache/datafusion/commit/79a19bf016dcaaed2aed2eb8305f088581d9f829. It now folds only integer types, as you preferred in the review.
##########
datafusion/optimizer/src/simplify_expressions/expr_simplifier.rs:
##########
@@ -828,6 +828,16 @@ impl<'a> Simplifier<'a> {
}
}
+fn modulo_one_is_zero_type(data_type: &DataType) -> bool {
+ match data_type {
+ DataType::Decimal32(_, scale)
Review Comment:
The branch is gone in https://github.com/apache/datafusion/commit/79a19bf016dcaaed2aed2eb8305f088581d9f829, since decimals no longer fold.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: keep decimal fractions in modulo by one [datafusion]
Posted by "2010YOUY01 (via GitHub)" <gi...@apache.org>.
2010YOUY01 commented on code in PR #25961:
URL: https://github.com/apache/datafusion/pull/25961#discussion_r4165354653
##########
datafusion/optimizer/src/simplify_expressions/expr_simplifier.rs:
##########
@@ -828,6 +828,16 @@ impl<'a> Simplifier<'a> {
}
}
+fn modulo_one_is_zero_type(data_type: &DataType) -> bool {
Review Comment:
```suggestion
/// Returns whether values of the given type are guaranteed to satisfy `value % 1 == 0`.
fn modulo_one_is_zero_type(data_type: &DataType) -> bool {
```
##########
datafusion/optimizer/src/simplify_expressions/expr_simplifier.rs:
##########
@@ -828,6 +828,16 @@ impl<'a> Simplifier<'a> {
}
}
+fn modulo_one_is_zero_type(data_type: &DataType) -> bool {
+ match data_type {
+ DataType::Decimal32(_, scale)
+ | DataType::Decimal64(_, scale)
+ | DataType::Decimal128(_, scale)
+ | DataType::Decimal256(_, scale) => *scale <= 0,
+ data_type => !data_type.is_floating(),
Review Comment:
Can we expand this `!floating` line to a white list instead? I think this bug was originally introduced because it's using a blacklist instead of white list.
##########
datafusion/optimizer/src/simplify_expressions/expr_simplifier.rs:
##########
@@ -828,6 +828,16 @@ impl<'a> Simplifier<'a> {
}
}
+fn modulo_one_is_zero_type(data_type: &DataType) -> bool {
+ match data_type {
+ DataType::Decimal32(_, scale)
Review Comment:
would be great to add a short comment to explain why it's allowed for decimal when scale <= 0, I think this condition is correct.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: keep decimal fractions in modulo by one [datafusion]
Posted by "2010YOUY01 (via GitHub)" <gi...@apache.org>.
2010YOUY01 merged PR #25961:
URL: https://github.com/apache/datafusion/pull/25961
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: keep decimal fractions in modulo by one [datafusion]
Posted by "2010YOUY01 (via GitHub)" <gi...@apache.org>.
2010YOUY01 commented on PR #25961:
URL: https://github.com/apache/datafusion/pull/25961#issuecomment-5967772792
Thanks again.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: keep decimal fractions in modulo by one [datafusion]
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #25961:
URL: https://github.com/apache/datafusion/pull/25961#issuecomment-5949131213
## [Codecov](https://app.codecov.io/gh/apache/datafusion/pull/25961?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
:x: Patch coverage is `55.55556%` with `4 lines` in your changes missing coverage. Please review.
:white_check_mark: Project coverage is 82.60%. Comparing base ([`4d167a1`](https://app.codecov.io/gh/apache/datafusion/commit/4d167a167228a424ef8cc29fd6bea5f17fc23c2a?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`2d62a46`](https://app.codecov.io/gh/apache/datafusion/commit/2d62a46726352a9cedede3ba5606eabb322d8ad3?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
:warning: Report is 1 commits behind head on main.
| [Files with missing lines](https://app.codecov.io/gh/apache/datafusion/pull/25961?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Patch % | Lines |
|---|---|---|
| [...imizer/src/simplify\_expressions/expr\_simplifier.rs](https://app.codecov.io/gh/apache/datafusion/pull/25961?src=pr&el=tree&filepath=datafusion%2Foptimizer%2Fsrc%2Fsimplify_expressions%2Fexpr_simplifier.rs&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-ZGF0YWZ1c2lvbi9vcHRpbWl6ZXIvc3JjL3NpbXBsaWZ5X2V4cHJlc3Npb25zL2V4cHJfc2ltcGxpZmllci5ycw==) | 55.55% | [2 Missing and 2 partials :warning: ](https://app.codecov.io/gh/apache/datafusion/pull/25961?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## main #25961 +/- ##
==========================================
- Coverage 82.60% 82.60% -0.01%
==========================================
Files 1145 1145
Lines 444057 444065 +8
Branches 444057 444065 +8
==========================================
+ Hits 366796 366800 +4
- Misses 54996 54997 +1
- Partials 22265 22268 +3
```
</details>
[:umbrella: View full report in Codecov by Harness](https://app.codecov.io/gh/apache/datafusion/pull/25961?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
<details><summary> :rocket: New features to boost your workflow: </summary>
- :snowflake: [Test Analytics](https://docs.codecov.com/docs/test-analytics): Detect flaky tests, report on failures, and find test suite problems.
- :package: [JS Bundle Analysis](https://docs.codecov.com/docs/javascript-bundle-analysis): Save yourself from yourself by tracking and limiting bundle sizes in JS merges.
</details>
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: keep decimal fractions in modulo by one [datafusion]
Posted by "1fanwang (via GitHub)" <gi...@apache.org>.
1fanwang commented on code in PR #25961:
URL: https://github.com/apache/datafusion/pull/25961#discussion_r4168158755
##########
datafusion/optimizer/src/simplify_expressions/expr_simplifier.rs:
##########
@@ -828,6 +828,16 @@ impl<'a> Simplifier<'a> {
}
}
+fn modulo_one_is_zero_type(data_type: &DataType) -> bool {
+ match data_type {
+ DataType::Decimal32(_, scale)
Review Comment:
The branch is gone in https://github.com/apache/datafusion/commit/79a19bf016dcaaed2aed2eb8305f088581d9f829, since decimals no longer fold.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org