-
Notifications
You must be signed in to change notification settings - Fork 732
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
Support user defined function #3655
Conversation
Thanks for the contribution! Please review the labels and make any necessary changes. |
1 similar comment
Thanks for the contribution! Please review the labels and make any necessary changes. |
This pull request is being automatically deployed with Vercel (learn more). 🔍 Inspect: https://vercel.com/databend/databend/GQjLtYjJZivc7tVCNCN6AYzjiFym [Deployment for 25e29b8 canceled] |
@BohuTANG The stateless test of Output diff:
|
|
Codecov Report
@@ Coverage Diff @@
## main #3655 +/- ##
======================================
Coverage 60% 60%
======================================
Files 678 694 +16
Lines 36484 37116 +632
======================================
+ Hits 21951 22393 +442
- Misses 14533 14723 +190
Continue to review full report at Codecov.
|
Awesome, really surprised me that this pr can be landed in such a swift speed. |
str_expr, | ||
} => self.visit_position(substr_expr, str_expr), | ||
Expr::Value(value) => { | ||
self.rpn.push(ExprRPNItem::Value(value.clone())); |
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.
That's much better now, no extra function call.
Wait for another reviewer approval |
[[package]] | ||
name = "common-sql" | ||
version = "0.1.0" | ||
dependencies = [ | ||
"common-exception", | ||
"sqlparser", | ||
] | ||
|
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 seems this crate only involves expr_visitor
, what about rename it to common-ast
? Since the name common-sql
looks too common.
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 don't have a strong opinion about the naming, I will rename it in other MR.
Great job, let's merge! |
I hereby agree to the terms of the CLA available at: https://databend.rs/policies/cla/
Summary
Support user defined function
@\d+
and starts with@0
; c) Params much be continuous, such as@0, @1, @2
; d) The order of params doesn't matter; e) Params are repeatable;analyze_expr
phase;CREATE/DROP/SHOW
function is supported;Examples are available in stateless tests.
Changelog
Related Issues
Fixes #3440
Test Plan
Unit Tests
Stateless Tests