fix: issue introduced in #6833 - less than equal check for scale in decimal conversion #7070

himadripal · 2025-02-03T20:56:00Z

Which issue does this PR close?

Follow on to Casting Decimal128 to Decimal128 with smaller precision produces incorrect results in some cases #6833
Closes [Regression in 54.0.0]. Decimal cast to smaller precision gives invalid (off-by-one) result in some cases #7069

Closes #.

Rationale for this change

What changes are included in this PR?

Are there any user-facing changes?

arrow-cast/src/cast/mod.rs

Blizzara · 2025-02-03T23:05:15Z

LGTM, thanks!

name change Co-authored-by: Arttu <[email protected]>

himadripal · 2025-02-04T04:35:58Z

@andygrove @tustvold @alamb @viirya please take a look when you get time.

kazuyukitanimura

LGTM

viirya · 2025-02-04T18:15:33Z

arrow-cast/src/cast/decimal.rs

@@ -167,7 +167,7 @@ where
    let array: PrimitiveArray<T> =
        if input_scale == output_scale && input_precision <= output_precision {
            array.clone()
-        } else if input_scale < output_scale {
+        } else if input_scale <= output_scale {
            // the scale doesn't change, but precision may change and cause overflow
            convert_to_bigger_or_equal_scale_decimal::<T, T>(


As the call is to convert_to_bigger_or_equal_scale_decimal, looks like the condition was not correct before. Btw, the above comment is "the scale doesn't change...", however, the scale is actually changed, no?

Yes. Previous PR introduced this bug and this is to fixing it. Removed comment.

andygrove

Thanks @himadripal

alamb · 2025-02-05T14:43:53Z

Thank you @himadripal @Blizzara @andygrove and @viirya

findepi · 2025-02-05T15:40:11Z

thanks for fixing this!

additional test case would be nice. casting from value 0 of decimal(3,0) type to decimal(2,0) type used to produce 1 before this fix. it sounds like a test case that should be in the code base (will add)

@himadripal would you want to backport this fix to the 53.0.0_maintenance branch? latest 53.4.0 is affected by this bug

(assuming we would want to release 53.5.0 release, cc @alamb. for a correctness bug like this i think we should, but not my call)

findepi · 2025-02-05T15:48:06Z

additional test case would be nice. casting from value 0 of decimal(3,0) type to decimal(2,0) type used to produce 1 before this fix. it sounds like a test case that should be in the code base (will add)

-> #7078

…e in decimal conversion (apache#7070) * fix <= check for scale in decimal conversion * Update arrow-cast/src/cast/mod.rs name change Co-authored-by: Arttu <[email protected]> * remove incorrect comment --------- Co-authored-by: Arttu <[email protected]>

Blizzara · 2025-02-10T10:18:57Z

@alamb et al, would it be possible to get a release with this fix in? Given it's a very clear correctness issue and regression, I'd feel that should warrant a fix release, but I'm not sure of the process or customs here..

alamb · 2025-02-10T12:09:55Z

@alamb et al, would it be possible to get a release with this fix in? Given it's a very clear correctness issue and regression, I'd feel that should warrant a fix release, but I'm not sure of the process or customs here..

@Blizzara yes I think we can make a release with the fix given that the issue was a regression -- let's move the conversation to #7083 (comment)

…e32 (#7074) * Support converting large dates (i.e. +10999-12-31) from string to Date32 * Fix lint * Update arrow-cast/src/parse.rs Co-authored-by: Andrew Lamb <[email protected]> * fix: issue introduced in #6833 - less than equal check for scale in decimal conversion (#7070) * fix <= check for scale in decimal conversion * Update arrow-cast/src/cast/mod.rs name change Co-authored-by: Arttu <[email protected]> * remove incorrect comment --------- Co-authored-by: Arttu <[email protected]> * minor: re-export `OffsetBufferBuilder` in `arrow` crate (#7077) * Add another decimal cast edge test case (#7078) * Add another decimal cast edge test case Before 1019f5b this test would fail, as the cast produced 1. 0 is an edge case worth explicitly testing for. * typo/fmt Co-authored-by: Felipe Oliveira Carvalho <[email protected]> --------- Co-authored-by: Felipe Oliveira Carvalho <[email protected]> * Support both 0x01 and 0x02 as type for list of booleans in thrift metadata (#7052) * Support both 0x01 and 0x02 as type for list of booleans * Also support 0 for false inside boolean collections * Use hex notation in tests * Fix LocalFileSystem with range request that ends beyond end of file (#6751) * Fix LocalFileSystem with range request that ends beyond end of file * fix windows * add comment * Seek error * fix seek check * remove windows flag * Get file length from file metadata * Introduce `UnsafeFlag` to manage disabling `ArrayData` validation (#7027) * Introduce UnsafeFlag to manage disabling validation * fix docs * Refactor arrow-ipc: Rename `ArrayReader` to `RecodeBatchDecoder` (#7028) * Rename `ArrayReader` to `RecordBatchDecoder` * Remove alias for `self` * Minor: Update release schedule (#7086) * Minor: Update release schedule * realism * Refactor some decimal-related code and tests (#7062) * Refactor some decimal-related code and tests in preparation for adding Decimal32 and Decimal64 support * Fixed symbol * Apply PR feedback * Fixed format problem * Fixed logical merge conflicts * PR feedback * Refactor arrow-ipc: Move `create_*_array` methods into `RecordBatchDecoder` (#7029) * Move `create_primitive_array` into RecordBatchReader * Move `create_list-array` into RecordBatchReader * Move `create_dictionay_array` into RecordBatchReader * Print Parquet BasicTypeInfo id when present (#7094) * Print Parquet BasicTypeInfo id when present * Improve print_schema documentation * tiny cleanup * Add a custom implementation `LocalFileSystem::list_with_offset` (#7019) * Initial change from Daniel. * Upgrade unit test to be more generic. * Add comments on why we have filter * Cleanup unit tests. * Update object_store/src/local.rs Co-authored-by: Adam Reeve <[email protected]> * Add changes suggested by Adam. * Cleanup match error. * Apply formatting changes suggested by cargo +stable fmt --all. * Apply cosmetic changes suggested by clippy. * Upgrade test_path_with_offset to create temporary directory + files for testing rather than pointing to existing dir. --------- Co-authored-by: Adam Reeve <[email protected]> * fix: first none/empty list in `ListArray` panics in `cast_with_options` (#7065) * fix: first none in `ListArray` panics in `cast_with_options` * simplify * fix * Update arrow-cast/src/cast/list.rs Co-authored-by: Jeffrey Vo <[email protected]> --------- Co-authored-by: Jeffrey Vo <[email protected]> * Benchmarks for Arrow IPC writer (#7090) * Add benchmarks for Arrow IPC writer * Add benchmarks for Arrow IPC writer * reuse target buffer * rename, etc * Add compression type * update --------- Co-authored-by: Andy Grove <[email protected]> * Minor: Clarify documentation on `NullBufferBuilder::allocated_size` (#7089) * Minor: Clarify documentaiton on NullBufferBuilder::allocated_size * add note about why allocations are 64 bytes * Add more tests for edge cases * Add negative test case for incorrectly formatted large dates --------- Co-authored-by: Andrew Lamb <[email protected]> Co-authored-by: Himadri Pal <[email protected]> Co-authored-by: Arttu <[email protected]> Co-authored-by: Piotr Findeisen <[email protected]> Co-authored-by: Felipe Oliveira Carvalho <[email protected]> Co-authored-by: Jörn Horstmann <[email protected]> Co-authored-by: Kyle Barron <[email protected]> Co-authored-by: Curt Hagenlocher <[email protected]> Co-authored-by: Devin Smith <[email protected]> Co-authored-by: Corwin Joy <[email protected]> Co-authored-by: Adam Reeve <[email protected]> Co-authored-by: irenjj <[email protected]> Co-authored-by: Jeffrey Vo <[email protected]> Co-authored-by: Andy Grove <[email protected]>

fix <= check for scale in decimal conversion

e2915cb

github-actions bot added the arrow Changes to the arrow crate label Feb 3, 2025

himadripal changed the title ~~fix issue introduced in #6833 less than equal check for scale in decimal conversion~~ fix: issue introduced in #6833 - less than equal check for scale in decimal conversion Feb 3, 2025

Blizzara reviewed Feb 3, 2025

View reviewed changes

arrow-cast/src/cast/mod.rs Outdated Show resolved Hide resolved

Update arrow-cast/src/cast/mod.rs

521f4d9

name change Co-authored-by: Arttu <[email protected]>

kazuyukitanimura approved these changes Feb 4, 2025

View reviewed changes

viirya reviewed Feb 4, 2025

View reviewed changes

remove incorrect comment

1bd3c00

andygrove approved these changes Feb 4, 2025

View reviewed changes

viirya approved these changes Feb 4, 2025

View reviewed changes

alamb merged commit 1019f5b into apache:main Feb 5, 2025
26 checks passed

himadripal mentioned this pull request Feb 5, 2025

[53.0.0_maintenance] Backport: fix: issue introduced in #6833 - less than equal check for scale in … #7079

Open

This was referenced Feb 10, 2025

Release arrow-rs / parquet minor version 54.2.0 (~Mar~ Feb 2025) #7083

Open

[Regression in 54.0.0]. Decimal cast to smaller precision gives invalid (off-by-one) result in some cases #7069

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: issue introduced in #6833 - less than equal check for scale in decimal conversion #7070

fix: issue introduced in #6833 - less than equal check for scale in decimal conversion #7070

himadripal commented Feb 3, 2025 •

edited by alamb

Loading

Blizzara commented Feb 3, 2025

himadripal commented Feb 4, 2025 •

edited

Loading

kazuyukitanimura left a comment

viirya Feb 4, 2025

himadripal Feb 4, 2025

andygrove left a comment

alamb commented Feb 5, 2025

findepi commented Feb 5, 2025 •

edited

Loading

findepi commented Feb 5, 2025

Blizzara commented Feb 10, 2025

alamb commented Feb 10, 2025

fix: issue introduced in #6833 - less than equal check for scale in decimal conversion #7070

fix: issue introduced in #6833 - less than equal check for scale in decimal conversion #7070

Conversation

himadripal commented Feb 3, 2025 • edited by alamb Loading

Which issue does this PR close?

Rationale for this change

What changes are included in this PR?

Are there any user-facing changes?

Blizzara commented Feb 3, 2025

himadripal commented Feb 4, 2025 • edited Loading

kazuyukitanimura left a comment

Choose a reason for hiding this comment

viirya Feb 4, 2025

Choose a reason for hiding this comment

himadripal Feb 4, 2025

Choose a reason for hiding this comment

andygrove left a comment

Choose a reason for hiding this comment

alamb commented Feb 5, 2025

findepi commented Feb 5, 2025 • edited Loading

findepi commented Feb 5, 2025

Blizzara commented Feb 10, 2025

alamb commented Feb 10, 2025

himadripal commented Feb 3, 2025 •

edited by alamb

Loading

himadripal commented Feb 4, 2025 •

edited

Loading

findepi commented Feb 5, 2025 •

edited

Loading