-
Notifications
You must be signed in to change notification settings - Fork 467
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
Convert more unary functions #11805
Convert more unary functions #11805
Conversation
990d4da
to
a2f5478
Compare
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 good.
sqlfunc!( | ||
#[sqlname = "octet_length"] | ||
fn byte_length_bytes<'a>(a: &'a [u8]) -> Result<i32, EvalError> { | ||
i32::try_from(a.len()).or(Err(EvalError::Int32OutOfRange)) |
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 often confused when I should use or
vs or_else
. Will the cost of constructing Err(EvalError::Int32OutOfRange)
be amortized somehow during compilation time because the expression is constant?
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.
Yes, that's exactly the intuition
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.
Also see this relevant clippy discussion on whether any const function should be allowed in an .or(..)
(the answer is no) rust-lang/rust-clippy#5658
#[derive(Ord, PartialOrd, Clone, Debug, Eq, PartialEq, Serialize, Deserialize, Hash, MzReflect)] | ||
pub struct JsonbStripNulls; | ||
|
||
impl LazyUnaryFunc for JsonbStripNulls { |
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.
What is the aspect of JsonbStripNulls
that prohibits from using sqlfunc!
in favor of impl LazyUnaryFunc
?
Is it just the fact that sqlfunc!
hard-codes an EagerUnaryFunc
implementation?
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.
It just allows it to re-use the temporary_storage
which is more efficient but I switched it now to sqlfunc!
which is a lot simpler
…ions Signed-off-by: Petros Angelatos <petrosagg@gmail.com>
Signed-off-by: Petros Angelatos <petrosagg@gmail.com>
a2f5478
to
0ec1c15
Compare
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
Signed-off-by: Petros Angelatos <petrosagg@gmail.com>
Signed-off-by: Petros Angelatos <petrosagg@gmail.com>
Signed-off-by: Petros Angelatos <petrosagg@gmail.com>
Signed-off-by: Petros Angelatos <petrosagg@gmail.com>
Signed-off-by: Petros Angelatos <petrosagg@gmail.com>
0ec1c15
to
e94e0a3
Compare
Motivation
Testing
Release notes
This PR includes the following user-facing behavior changes: