-
Notifications
You must be signed in to change notification settings - Fork 1k
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
feat: Support Binary bitwise shift operators (<< and >>) #3037
Conversation
a5a06ff
to
6449570
Compare
Right now, DF uses Generic Dialect for Token::ShiftLeft if dialect_of!(self is PostgreSqlDialect) => {
Some(BinaryOperator::PGBitwiseShiftLeft)
}
Token::ShiftRight if dialect_of!(self is PostgreSqlDialect) => {
Some(BinaryOperator::PGBitwiseShiftRight)
} @alamb what should I do with it? Should I Introduce new methods for Dialect to allow these operators with Generic dialect? Thanks |
What about allowing I can make another sql-parser release soon if you need one as well |
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 looked at the PR and I think the basic direction is very nice 👍 thanks @ovr
@@ -94,6 +94,50 @@ pub(crate) fn bitwise_and(left: ArrayRef, right: ArrayRef) -> Result<ArrayRef> { | |||
} | |||
} | |||
|
|||
pub(crate) fn bitwise_shift_right(left: ArrayRef, right: ArrayRef) -> Result<ArrayRef> { |
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.
❤️
LGTM, if we can merge the sql-rs first |
Looks likes I forget about overflowing here. I think I can use
|
4e1dfde
to
36aff67
Compare
Panic on overflow was fixed in 474ed5a Without error to be similar with PostgreSQL |
https://crates.io/crates/sqlparser/0.20.0 is released, FYI |
Awaiting #3072 |
Is merged 🎉 |
474ed5a
to
636be02
Compare
@@ -49,7 +50,7 @@ macro_rules! binary_bitwise_array_op { | |||
/// like int64, int32. | |||
/// It is used to do bitwise operation on an array with a scalar. | |||
macro_rules! binary_bitwise_array_scalar { |
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've changed binary_bitwise_array_scalar
to pass function as expr
to do casting, because wrapping_shr
and wrapping_shl
requires u32. I tried to do it with let right: $TYPE = scalar.try_into().unwrap();
but I found that it will show a error because it's not possible to cast all numeric types to u32
.
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 think it makes sense -- thank you
636be02
to
17fb689
Compare
17fb689
to
92fa53c
Compare
Codecov Report
@@ Coverage Diff @@
## master #3037 +/- ##
==========================================
- Coverage 85.95% 85.91% -0.04%
==========================================
Files 291 291
Lines 52382 52475 +93
==========================================
+ Hits 45025 45084 +59
- Misses 7357 7391 +34
📣 We’re building smart automated test selection to slash your CI/CD build times. 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.
This PR is looking pretty good 👍
Please mark it as "ready for review" when it is ready.
Thanks !
Marked PR as ready for review ✅ |
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.
Looks great to me @ovr - thank you for the contribution!
I had one tiny testing suggestion, but otherwise I think this PR is ready to go
let modules = Arc::new(Int32Array::from(vec![Some(100)])) as ArrayRef; | ||
let result = bitwise_shift_left(input.clone(), modules.clone())?; | ||
|
||
let expected = Int32Array::from(vec![Some(32)]); |
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.
2<<100
=>> 2<<(100%bit_width(i32))
=>> 2 << 4
?
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 am surprised that this is rotational shifting
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 am surprised that this is rotational shifting
me too...
#[test] | ||
fn bitwise_shift_array_test() -> Result<()> { | ||
let input = Arc::new(Int32Array::from(vec![Some(2), None, Some(10)])) as ArrayRef; | ||
let modules = |
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 suggest a test for when the modules
is Null (you cover NULL for the input already)
#[test] | ||
fn bitwise_shift_scalar_test() -> Result<()> { | ||
let input = Arc::new(Int32Array::from(vec![Some(2), None, Some(4)])) as ArrayRef; | ||
let module = ScalarValue::from(10i32); |
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.
Likewise, here a test for null handling might be good
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.
Instead of doing unit test here, I did a simple SQL test which test the whole pipeline, and I found that DF doesn't handle nulls for bitwise operators.
Fixed in:
@@ -49,7 +50,7 @@ macro_rules! binary_bitwise_array_op { | |||
/// like int64, int32. | |||
/// It is used to do bitwise operation on an array with a scalar. | |||
macro_rules! binary_bitwise_array_scalar { |
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 think it makes sense -- thank you
92fa53c
to
624e8b3
Compare
@@ -139,12 +139,14 @@ pub fn coerce_types( | |||
fn bitwise_coercion(left_type: &DataType, right_type: &DataType) -> Option<DataType> { | |||
use arrow::datatypes::DataType::*; | |||
|
|||
if !is_numeric(left_type) || !is_numeric(right_type) { | |||
if !both_numeric_or_null_and_numeric(left_type, right_type) { |
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.
Co-authored-by: Jiayu Liu <Jimexist@users.noreply.github.com>
Can we update the user guide to show this new functionality? This can be a follow-on issue. |
I didn't find any place where I should update the documentation, as I can see there is a small comment about binary expressions, but it doesn't include all binary expressions.
I updated the description on this PR to reference #1619 |
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 again @ovr
Benchmark runs are scheduled for baseline = 5ddad47 and contender = ee55d89. ee55d89 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?
This PR introduces support for binary bitwise shift operators like
>>
and<<
.Refs #1619
Are there any user-facing changes?
It's new functionality, and there are no breaking changes.
Thanks!