fix: parse var len of decimal for parquet statistic #837
Conversation
| precision: 38, | ||
| scale: 0 | ||
| }, | ||
| PrimitiveLiteral::Int128(99999999999999999999999999999999999999_i128) |
There was a problem hiding this comment.
Hi, I assume the limitation of rust_decimal is that it only supports up to 28 digits of precision. Values of Decimal larger than Decimal::MAX or smaller than Decimal::MIN are not supported.
The best we can do here is raising an error if users trying to read or write Decimal larger than 28.
There was a problem hiding this comment.
But from the definition of iceberg spec, the precision of decimal can be 38. So I think for here we can't prevent user write decimal with precision larger than 28 but smaller than 38. https://iceberg.apache.org/spec/#primitive-types
There was a problem hiding this comment.
Yep, that is exactly the issue I raised here #669
There was a problem hiding this comment.
Actually, according to my understanding, our decimal is not restricted by rust_decimal.🤔 Internally iceberg have its own decimal representation.
For here, we just provide a way let the user pass a rust_decimal and convert it into our decimal representation. If the rust_decimal is that it only supports up to 28 digits of precision, when user want to more precision, they can pass other format to convert it into our decimal representation. Or as #669, we can provide a way to convert other decimal which support more precision to our decimal representation.
|
Hi @liurenjie1024 @Xuanwo , is there other question about this PR? |
Hi, I don't quite understand what issue this PR is trying to address. The test added in this PR is incorrect, as we can't support such a case unless we use other libraries to handle decimals. I'm also concerned about the root cause of this issue. Is it related to the limitations of our decimal library? |
Actually, this PR is used to fix case like: 1.1, 2.2, so it's not related the limitations of our decimal library.
Let me remove the incorrect the wrong case now. I think I should reuse a decimal library which can support 38 precision and after that we can add the test case back. |
|
Thank you, @ZENOTME, for the clarification. Tagging @liurenjie1024 and @Fokko to review this again. |
crates/iceberg/src/arrow/schema.rs
Outdated
| @@ -741,7 +766,7 @@ macro_rules! get_parquet_stat_as_datum { | |||
| }; | |||
| Some(Datum::new( | |||
| primitive_type.clone(), | |||
| PrimitiveLiteral::Int128(i128::from_be_bytes(bytes.try_into()?)), | |||
| PrimitiveLiteral::Int128(i128::from_be_bytes(extend_to_i128_big_endian(bytes.into())?)), | |||
There was a problem hiding this comment.
I revisited the code and found that the root cause might be that we didn't handle the decimal bytes data correctly. By design, Iceberg only keeps the smallest bits of the decimal. To decode it properly, we may need to follow the same logic outlined here:
iceberg-rust/crates/iceberg/src/spec/values.rs
Lines 436 to 445 in 09fa1fa
|
Thank you @ZENOTME for fixing this and thank you @liurenjie1024 for the review, let's merge! |
I find that the decimal encode for parquet statistic will be byte with var len. Not necessary with 16 len. So we need to extend it when the len smaller than 16.