feat(ast_build): implement ifnull function #620
Conversation
devgony
requested review from
panarch,
24seconds,
devgony,
ever0de and
MRGRAVITY817
| FunctionNode::IfNull(expr_node, then_node) => expr_node | ||
| .try_into() | ||
| .and_then(|expr| then_node | ||
| .try_into() | ||
| .map(|then| Function::IfNull { expr, then }) | ||
| .map(Box::new) | ||
| .map(Expr::Function)) |
There was a problem hiding this comment.
It doesn't seem like rustfmt is applied.
Would you like to run cargo fmt ?
There was a problem hiding this comment.
@ever0de Yes, I totally forgot to do so.
I ran the command and pushed the commit.
Thank you for the fast feedback.
Codecov Report
@@ Coverage Diff @@
## main #620 +/- ##
=======================================
Coverage 94.27% 94.27%
=======================================
Files 214 214
Lines 15193 15207 +14
=======================================
+ Hits 14323 14337 +14
Misses 870 870
Continue to review full report at Codecov.
|
bugoverdose
force-pushed
the
feat/ast_builder/ifnull
branch
from
6a242e2
to
9f82299
Compare
| #[derive(Clone)] | ||
| pub enum FunctionNode { | ||
| Abs(ExprNode), | ||
| IfNull(ExprNode, ExprNode), | ||
| Floor(ExprNode), | ||
| } |
There was a problem hiding this comment.
At first, I just ordered the Abs, IfNull, Floor alphabetically,
but I think that it makes more sense to match the order at Function enum (core/src/ast/function.rs).
All the code below also follows the same order.
| function::{abs, floor, FunctionNode}, | ||
| function::{abs, floor, ifnull, FunctionNode}, | ||
| }; |
There was a problem hiding this comment.
This part gets ordered alphabetically when I run cargo fmt though.
|
|
||
| #[test] | ||
| fn function_abs() { | ||
| fn abs_test() { |
There was a problem hiding this comment.
Since we used the #[test] macro, it turns out that this is a test function.
It seems good to omit the test prefix.
example)
https://github.com/gluesql/gluesql/blob/main/core/src/ast_builder/select/having.rs#L73-L74
https://github.com/gluesql/gluesql/blob/main/core/src/plan/evaluable.rs#L175-L176
There was a problem hiding this comment.
I agree. I really wanted to just write it like abs, ifnull, floor.
But I get an error[E0255]: the name abs is defined multiple times message when I do so (because of line 68).
I'll rollback the changes to function_abs format for now.
| let actual = col("updated_at").ifnull(col("created_at")); | ||
| let expected = "IFNULL(updated_at, created_at)"; | ||
| test_expr(actual, expected); | ||
|
|
||
| let actual = text("HELLO").ifnull(text("WORLD")); | ||
| let expected = "IFNULL('HELLO', 'WORLD')"; | ||
| test_expr(actual, expected); |
There was a problem hiding this comment.
Current two unit tests are identical.
Both tests use
pub fn ifnull(self, another: ExprNode) -> ExprNode {
..
}Let's replace one of those to use
pub fn ifnull<T: Into<ExprNode>, V: Into<ExprNode>>(expr: T, then: V) -> ExprNode {
..
}There was a problem hiding this comment.
You are right. I'll fix one of them.
- run cargo fmt
Hello, I implemented the
ifnullfunction atast_build.I was going to name it
if_nullbecause it's snake_case,but I found the already implemented
ifnullfunction atexecutor/evaluate/function.rs, so I erased the_part.Please leave an RC if there's any problem, suggestion or conflict at main branch.
Thank you.
Closes #618