-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Fix decimal scalar dyn kernels #5179
Conversation
@@ -189,28 +189,20 @@ pub(crate) fn add_decimal( | |||
} | |||
|
|||
pub(crate) fn add_decimal_dyn_scalar(left: &dyn Array, right: i128) -> Result<ArrayRef> { | |||
let left_decimal = left.as_any().downcast_ref::<Decimal128Array>().unwrap(); | |||
let (precision, scale) = get_precision_scale(left)?; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In last PR, forgot to update this and there.
CI seems broken:
|
9c1520c
to
72367ed
Compare
Proposed CI fix at #5182. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The changes to the Decimal logic makes sense to me (other than the changes to github config -- I think you can remove those prior to merge)
Operator::Plus, | ||
ScalarValue::Dictionary( | ||
Box::new(DataType::Int8), | ||
Box::new(ScalarValue::Decimal128(Some(1), 10, 0)), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
without this PR's code, is the actual type of the output array whatever the default precision of Decimal128
is (rather than 10, 0
)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, original code still modifies precision/scale for output array, but it just worked for primitive array not dictionary array.
This reverts commit 72367ed.
Benchmark runs are scheduled for baseline = a213d62 and contender = 4233752. 4233752 is a master commit associated with this PR. Results will be available as each benchmark for each run completes. |
Which issue does this PR close?
Follow of #5151.
Rationale for this change
For
add_decimal_dyn_scalar
andsubtract_decimal_dyn_scalar
, it should callget_precision_scale
anddecimal_array_with_precision_scale
likemultiply_decimal_dyn_scalar
anddivide_decimal_dyn_scalar
as the input/output could be DictionaryArray.What changes are included in this PR?
Are these changes tested?
Are there any user-facing changes?