-
Notifications
You must be signed in to change notification settings - Fork 1.7k
feat(spark): implement Spark bitwise function shiftleft/shiftright/shiftrightunsighed #17013
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
Conversation
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.
Thanks for the PR; it's quite chunky, so would appreciate some explanations on some of the decisions made in the code
// Coerce smaller integer types to Int32 | ||
let coerced_first = match &arg_types[0] { | ||
DataType::Int8 | DataType::Int16 | DataType::Null => DataType::Int32, | ||
DataType::UInt8 | DataType::UInt16 => DataType::UInt32, | ||
_ => arg_types[0].clone(), | ||
}; | ||
|
||
Ok(vec![coerced_first, DataType::Int32]) |
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.
Is there an explanation for why we coerce to these specific types?
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.
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.
I see, so it's to align with Spark (well Java) only having access to Int/Long types; I do feel this coercion logic might already be expressible via the TypeSignature
API, have you given that a shot?
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.
Some suggested simplifications, otherwise I'm fine with merging as is
/// | ||
/// # Arguments | ||
/// * `value` - The array of values to shift. | ||
/// * `shift` - The array of shift amounts (must be Int32). | ||
/// | ||
/// # Returns | ||
/// A new array with the shifted values. | ||
/// |
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.
/// | |
/// # Arguments | |
/// * `value` - The array of values to shift. | |
/// * `shift` - The array of shift amounts (must be Int32). | |
/// | |
/// # Returns | |
/// A new array with the shifted values. | |
/// |
Would prefer to minimize verbosity of documentation where possible (applicable for the other similar instances below)
Which issue does this PR close?
Rationale for this change
What changes are included in this PR?
implement shiftleft/shiftright/shiftrightunsighed.
Are these changes tested?
UT
Are there any user-facing changes?
No