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
Add support for SQL bitwise operators on integer types #8350
Add support for SQL bitwise operators on integer types #8350
Conversation
@@ -1166,6 +1166,54 @@ fn add_interval<'a>(a: Datum<'a>, b: Datum<'a>) -> Result<Datum<'a>, EvalError> | |||
.map(Datum::from) | |||
} | |||
|
|||
fn bit_not_int16<'a>(a: Datum<'a>) -> Datum<'a> { |
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.
Could the unary funcs be expressed as a macro? Not sure tbh, but maybe worth checking!
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.
Probably, but this issue is more general, as most functions come in families indexed by the possible combination of types (e.g. see neg_int{XX}
, abs_int{XX}
, ...). I agree that we should think how we can minimize code for this functions, but I prefer to not tackle this as part of my starter issue. We could use a macro, think about how to utilize generics in a better way, or choose another zero cost abstraction.
I'm happy to open a follow-up issue if more people agree that this is a problem.
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.
@petrosagg ^^^
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 was thinking about something like this:
sqlfunc!(
fn bit_not_int16(a: i16) -> i16 {
!a
}
);
Elsewhere, there seems to be a TryFrom
missing:
impl TryFrom<Datum<'_>> for i16 {
type Error = ();
fn try_from(from: Datum<'_>) -> Result<Self, Self::Error> {
match from {
Datum::Int16(f) => Ok(f),
_ => Err(()),
}
}
}
impl TryFrom<Datum<'_>> for Option<i16> {
type Error = ();
fn try_from(from: Datum<'_>) -> Result<Self, Self::Error> {
match from {
Datum::Null => Ok(None),
Datum::Int16(f) => Ok(Some(f)),
_ => Err(()),
}
}
}
But, totally up to you :D
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.
Using the sqlfunc!
macro for this unary function is totally doable and totally optional since this is your first issue. We're in the process of transitioning the traditional declarations (like the one you provided for bit_not_int16
) to use sqlfunc!
. The plan is to transition all functions (even binary and variadic), but for now only unarfy functions are supported by the macro.
So, if you want to merge as is, that's totally fine, I will include this function in a future PR that will migrate all the int16
functions whole sale.
If you want to dip your toes in the new way of defining unary functions you can use @antiguru's snippets above and a commit like this one for reference: 88a1443
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.
So, if you want to merge as is, that's totally fine, I will include this function in a future PR that will migrate all the int16 functions whole sale.
@petrosagg: I like the approach outlined in #7704! Unfortunately, missed that and started working on a this feature shortly before the PR was merged, so I prefer to merge the current PR without further. I am happy take off some of the refactoring work (including the ~
operator) in the next days.
Elsewhere, there seems to be a TryFrom missing:
@antiguru Is the above somehow affecting the correctness of the current implementation and actionable on my end?
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, TryFrom
is only needed if you go down the sqlfunc!
path. Don't worry, I'm slowly converting all the functions in batches, let's merge this as is :)
abe06c2
to
1cc213b
Compare
c8de9d5
to
b53125c
Compare
@@ -48,7 +48,9 @@ Wrap your release notes at the 80 character mark. | |||
|
|||
{{% version-header v0.9.5 %}} | |||
|
|||
- Timezone parsing is now case insensitive to be compatible with PostgreSQL | |||
- Timezone parsing is now case insensitive to be compatible with PostgreSQL. | |||
|
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.
extra new line here
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 followed the formatting from the last few versions (line items are separated by a new line). I now realize this is not correct markdown, though (it will be interpreted as a sequence of singleton lists, so maybe we should omit the newlines as you are suggesting.
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 once remaining comments are addressed! Thanks!
b53125c
to
b4687e6
Compare
Adds the following bitwise operators on `smallint`, `int`, and `bigint` types. - Bitwise AND (`&`) - Bitwise XOR (`#`) - Bitwise OR (`|`) - Bitwise NOT (`~`) - Shift left (`<<`) - Shift right (`>>`) I had to add a new precedence for the bitwise NOT in order to maintain compatibility with Postgres (see diff in `parser.rs`). This differs from the CockroachDB semantics which does not align with Postgres. Closes MaterializeInc#7192
b4687e6
to
d055125
Compare
Motivation
Adds missing bitwise operators on integers as suggested in #7192.
Description
Adds the following bitwise operators on
smallint
,int
, andbigint
types&
)#
)|
)~
)<<
)>>
)Tips for reviewer
parser.rs
). Note that this is different from the CockroachDB semantics which are not aligned with Postgres.Checklist