-
Notifications
You must be signed in to change notification settings - Fork 867
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.
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
support cast for decimal data type #1043
Comments
please assign this issue to me. |
@liukun4515 I personally think it would be valuable to support I suspect once you implement |
Ok, I will implement signed numeric first, and leave other as the follow-up PR. |
On commit ab48e69 building with nightly-2021-12-05-aarch64-apple-darwin I am getting: ---- record::api::tests::test_convert_double_to_string stdout ----
thread 'record::api::tests::test_convert_double_to_string' panicked at 'assertion failed: `(left == right)`
left: `"1e-15"`,
right: `"0.000000000000001"`', parquet/src/record/api.rs:1097:9
---- record::api::tests::test_convert_float_to_string stdout ----
thread 'record::api::tests::test_convert_float_to_string' panicked at 'assertion failed: `(left == right)`
left: `"1e-15"`,
right: `"0.000000000000001"`', parquet/src/record/api.rs:1084:9
The string formatter is: arrow-rs/parquet/src/record/api.rs Lines 716 to 728 in ab48e69
Where are the requirements for this? |
I do not know @chadbrewbaker Some code archeology in https://github.com/apache/arrow-rs/blame/ab48e69099f2d7c4178a75f7e1cf8b223ecf8d76/parquet/src/record/api.rs#L717 suggests that there has been a special case for checking that value for quite a while Given that |
Same errors on x86. This seems to be the last commit that changed the rounding. |
@chadbrewbaker On a mac, the tests pass for me using
I also get the diff using nightly:
If you want to propose a patch to allow the tests to pass on both nightly and stable that would be 👍 |
Bah - my stupidity not running stable. I'll diff what they changed in nightly. Seems to be cross-platform. |
I'll try to upstream our test for the "1e-15" boundary. Doesn't seem to be one now. |
TIL cargo bisect-rustc --start 2021-10-01 -- cargo test test_convert_double_to_string |
I bisected the 1e-15 conversion regression down to this changeset between nightlies. |
From your range of commit you can see that there is a commit named Automatic exponential formatting in Debug that added an internal logic that switch to the exponential representation in some cases when using the I would suggest changing your formatting to the |
@liukun4515 I will do so today |
There are bugs for If we cast decimal(1256,12,2) to the type of decimal(12,1), the result should be decimal(126,12,1) not the decimal(125,12,1) |
👍 thanks @liukun4515 -- maybe it is worth its own ticket to talk about the rounding vs truncation behavior of decimals while casting |
Nice! I will look at the commit. For ML workloads, also going to need a version that stochastically flips the last bit. https://twitter.com/id_aa_carmack/status/1296295284994183168 |
Possibly related #3281 |
Is your feature request related to a problem or challenge? Please describe what you are trying to do.
In the apache/datafusion#122 (comment), we need to support decimal for
cast
andtry_cast
In the first step, we just support signed numeric data type to decimal, and other casting rules related to decimal will be supported in the feature.
future
enhancement
optimize iter the decimal array and use the from iter to construct Int、Float Array Add into iter for decimal array #1083
add benchmark result for cast decimal data type
Describe the solution you'd like
A clear and concise description of what you want to happen.
Describe alternatives you've considered
A clear and concise description of any alternative solutions or features you've considered.
Additional context
Add any other context or screenshots about the feature request here.
The text was updated successfully, but these errors were encountered: