Repository navigation
fix: support parentheses for negative decimal formatting - #23718
Conversation
There was a problem hiding this comment.
@wangzhigang1999
Thanks for the fix! I took a look and everything looks good to me. I have one small suggestion that could make the regression coverage a bit more complete.
| fn test_grouping_separator_parentheses_decimal() -> Result<()> { | ||
| // %(,15.2f on negative decimal — format_decimal ignores negative_in_parentheses, | ||
| // always uses '-'. Check TODO in fn format_decimal | ||
| test_scalar_function!( |
There was a problem hiding this comment.
Nice addition to the regression tests. Would it be worth adding a Decimal256 case for the same %(...) decimal formatting regression as well?
I believe the implementation path is shared between Decimal128 and Decimal256, so I do not think this is required for correctness. That said, having both public decimal scalar variants covered would help lock the behavior in and guard against future regressions.
There was a problem hiding this comment.
Good point, thanks! I added an equivalent Decimal256 regression case in 0d51a2c, so both public decimal scalar variants are now covered.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #23718 +/- ##
==========================================
- Coverage 80.69% 80.66% -0.04%
==========================================
Files 1088 1095 +7
Lines 367766 372318 +4552
Branches 367766 372318 +4552
==========================================
+ Hits 296781 300326 +3545
- Misses 53276 54070 +794
- Partials 17709 17922 +213 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
## Which issue does this PR close? - Closes apache#23717. ## Rationale for this change Spark and Java format negative numeric values with parentheses when the `(` flag is present. The decimal formatting path always emitted a minus sign because it ignored `negative_in_parentheses`, while the floating-point path already handled the flag. This made `format_string` inconsistent across numeric input types. ## What changes are included in this PR? - Add a closing suffix when a negative decimal uses parentheses formatting. - Include the suffix when calculating width, left alignment, and zero padding. - Add regression coverage for grouped negative decimals with and without an explicit width. ## Are these changes tested? Yes. The following checks pass locally: - `cargo fmt --all -- --check` - `cargo clippy --all-targets --all-features -- -D warnings` - `cargo test -p datafusion-spark --lib` - The full workspace test command from the contributor guide with `avro,json,backtrace,extended_tests,recursive_protection,parquet_encryption` enabled ## Are there any user-facing changes? Yes. Spark-compatible formatting of negative decimal values now honors the parentheses flag. There are no public API or breaking changes.
Which issue does this PR close?
Rationale for this change
Spark and Java format negative numeric values with parentheses when the
(flag is present. The decimal formatting path always emitted a minus sign because it ignorednegative_in_parentheses, while the floating-point path already handled the flag. This madeformat_stringinconsistent across numeric input types.What changes are included in this PR?
Are these changes tested?
Yes. The following checks pass locally:
cargo fmt --all -- --checkcargo clippy --all-targets --all-features -- -D warningscargo test -p datafusion-spark --libavro,json,backtrace,extended_tests,recursive_protection,parquet_encryptionenabledAre there any user-facing changes?
Yes. Spark-compatible formatting of negative decimal values now honors the parentheses flag. There are no public API or breaking changes.