scovich commented on code in PR #8540:
URL: https://github.com/apache/arrow-rs/pull/8540#discussion_r2399754742
##########
parquet/src/arrow/schema/primitive.rs:
##########
@@ -132,35 +133,24 @@ fn from_parquet(parquet_type: &Type) -> Result<DataType> {
}
fn decimal_type(scale: i32, precision: i32) -> Result<DataType> {
- if precision <= DECIMAL128_MAX_PRECISION as i32 {
- decimal_128_type(scale, precision)
- } else {
- decimal_256_type(scale, precision)
- }
-}
-
-fn decimal_128_type(scale: i32, precision: i32) -> Result<DataType> {
+ // Convert and validate inputs once
let scale = scale
.try_into()
.map_err(|_| arrow_err!("scale cannot be negative: {}", scale))?;
-
let precision = precision
.try_into()
.map_err(|_| arrow_err!("precision cannot be negative: {}",
precision))?;
- Ok(DataType::Decimal128(precision, scale))
-}
-
-fn decimal_256_type(scale: i32, precision: i32) -> Result<DataType> {
- let scale = scale
- .try_into()
- .map_err(|_| arrow_err!("scale cannot be negative: {}", scale))?;
-
- let precision = precision
- .try_into()
- .map_err(|_| arrow_err!("precision cannot be negative: {}",
precision))?;
-
- Ok(DataType::Decimal256(precision, scale))
+ // Dispatch based on precision thresholds using DecimalType trait constants
+ if precision <= Decimal32Type::MAX_PRECISION {
Review Comment:
> Is this change needed for this PR?
Without it, almost all variant decimal integration tests fail because they
expect `Variant::Decimal4` or `Variant::Decimal8` and they get
`Variant::Decimal16` instead. I don't know any good way to "fix" the test
expectations and anyway the expectations are arguably correct and should _not_
change.
That said...
I suspect the current version is too aggressive -- it uses precision as the
ultimate lower bound regardless of the physical encoding; I'm working on a
revised version that uses precision as an _upper_ bound, with the physical
encoding as lower bound.
Still TBD whether that reduces the test carnage at all, let alone whether
it's actually the right approach.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
