Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Casting Decimal128 to Decimal128 with smaller precision produces incorrect results in some cases #6833

Closed
andygrove opened this issue Dec 3, 2024 · 2 comments · Fixed by #6836
Closed

Casting Decimal128 to Decimal128 with smaller precision produces incorrect results in some cases #6833

andygrove opened this issue Dec 3, 2024 · 2 comments · Fixed by #6836
Labels
arrow Changes to the arrow crate bug

Comments

@andygrove
Copy link
Member

andygrove commented Dec 3, 2024 •
edited
Loading

Describe the bug

If I try and cast floating-point value 12345.67 to decimal(6,2) then I get an error, as expected:

"1234567 is too large to store in a Decimal128 of precision 6. Max is 999999"

However, if I try and cast a decimal(24,2) value of 12345.67 to decimal(6,2) then I do not get an error but instead have an invalid decimal value.

PrimitiveArray<Decimal128(6, 2)>
[
    1234567,
]

If I then convert this decimal to a string, it produces 1234.56 instead of 12345.67

StringArray
[
  "1234.56",
]

To Reproduce

Expected behavior

Additional context

This is the root cause of apache/datafusion#13492
@andygrove andygrove added the bug label Dec 3, 2024
@himadripal
Copy link
Contributor

Working on it

@himadripal himadripal mentioned this issue Dec 4, 2024
Merged
@andygrove andygrove mentioned this issue Dec 6, 2024
Merged
@alamb alamb added the arrow Changes to the arrow crate label Dec 17, 2024
@alamb
Copy link
Contributor

alamb commented Dec 17, 2024

label_issue.py automatically added labels {'arrow'} from #6850

This was referenced Feb 3, 2025
Open
Closed
@alamb alamb mentioned this issue Feb 4, 2025
Merged
alamb pushed a commit that referenced this issue Feb 5, 2025
@himadripal @Blizzara
fix: issue introduced in #6833 - less than equal check for scale in d…
1019f5b 
…ecimal 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]>
felipecrv pushed a commit to sdf-labs/arrow-rs that referenced this issue Feb 5, 2025
@himadripal @Blizzara @felipecrv
fix: issue introduced in apache#6833 - less than equal check for scal…
e070c4a 
…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]>
himadripal added a commit to himadripal/arrow-rs that referenced this issue Feb 5, 2025
@himadripal @Blizzara
fix: issue introduced in apache#6833 - less than equal check for scal…
807ac46 
…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]>
himadripal added a commit to himadripal/arrow-rs that referenced this issue Feb 5, 2025
@himadripal @Blizzara
fix: issue introduced in apache#6833 - less than equal check for scal…
efa7e8d 
…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]>
phillipleblanc pushed a commit to spiceai/arrow-rs that referenced this issue Feb 10, 2025
@himadripal @Blizzara @phillipleblanc
fix: issue introduced in apache#6833 - less than equal check for scal…
458773c 
…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]>
alamb added a commit that referenced this issue Feb 12, 2025
@phillipleblanc @alamb
@himadripal @Blizzara @findepi @felipecrv @jhorstmann @kylebarron @CurtHagenlocher @devinrsmith @corwinjoy @adamreeve @irenjj @Jefffrey @andygrove
Support converting large dates (i.e. +10999-12-31) from string to Dat…
2bce568 
…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]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
arrow Changes to the arrow crate bug
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

fix: decimal conversion looses value on lower precision himadripal/arrow-rs
3 participants
@alamb @andygrove @himadripal