-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
Re-enable TPCH query 6 (#4024) #4229
Conversation
Sadly it would appear not 😢 |
Getting close ... I think there are still some decimal rounding issues to be resolved
|
Maybe related to decimal casting rounding issue apache/arrow-rs#1043 (comment) |
Looking at the plan, it looks like the decimal filter values is coming through nicely (though I think that we should see if we can default DataFusion to So my guess is that either the multiplication of
|
Looks like both multiplication as addition is wrong. pub(crate) fn multiply_decimal(
left: &Decimal128Array,
right: &Decimal128Array,
) -> Result<Decimal128Array> {
let divide = 10_i128.pow(left.scale() as u32);
let array = multiply(left, right)?;
let array = divide_scalar(&array, divide)?
.with_precision_and_scale(left.precision(), left.scale())?;
Ok(array)
} Correct logic without losing information should be to multiply and add the scales instead of using division. I think there is some error in addition as well: pub(crate) fn add_decimal(
left: &Decimal128Array,
right: &Decimal128Array,
) -> Result<Decimal128Array> {
let array =
add(left, right)?.with_precision_and_scale(left.precision(), left.scale())?;
Ok(array)
}
pub(crate) fn add_decimal_scalar(
left: &Decimal128Array,
right: i128,
) -> Result<Decimal128Array> {
let array = add_scalar(left, right)?
.with_precision_and_scale(left.precision(), left.scale())?;
Ok(array)
} This only looks at the left scale / precision, but it should increase the precision to capture. |
Completed in main :) |
Which issue does this PR close?
Closes #4024
Rationale for this change
#4199 might have resolved this 🤞 (my laptop is too piddly to confirm locally)
What changes are included in this PR?
Are these changes tested?
Are there any user-facing changes?