-
Notifications
You must be signed in to change notification settings - Fork 5.9k
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
expression: refactor functions to build expressions #50997
Conversation
Hi @lcwangchao. Thanks for your PR. PRs from untrusted users cannot be marked as trusted with I understand the commands that are listed here. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
a4238ba
to
9a826ad
Compare
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## master #50997 +/- ##
================================================
+ Coverage 70.5150% 72.6341% +2.1190%
================================================
Files 1467 1467
Lines 434890 434896 +6
================================================
+ Hits 306663 315883 +9220
+ Misses 108910 99077 -9833
- Partials 19317 19936 +619
Flags with carried forward coverage won't be shown. Click here to find out more.
|
9a826ad
to
23bc742
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
6341099
to
9c5457b
Compare
9c5457b
to
7f6aafe
Compare
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: lance6716, mjonss, XuHuaiyu, YangKeao, zimulala The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
003c23b
to
b624542
Compare
What problem does this PR solve?
Issue Number: close #50996
We have many functions to build expressions, but it is unclear which we should use for a certain case and some codes in these functions are duplicated. It's better to refactor them to make it clear.
What changed and how does it work?
In this PR, we provide two groups of functions to build expressions.
The first function group is exposed in the package
expression
and used to build simple expressions. The "simple" means they do not include some special expressions such as subquery. Here we simplified to 3 functions:expression.BuildSimpleExpr
: build a simple expression from an AST node.expression.ParseSimpleExpr
: parse a string and then build a simple expressionexpression.EvalSimpleAst
: build a simple expression from an AST node and evaluate it directly.The first group requires limited context and can be used in some places where you do not want to introduce too many dependencies. However, if you need to build some complex expressions like subquery, we should use the second group. The second group is exposed in the package
planner/util
and can build any expression. However, it requires the full context of a planner. These functions includes:util.RewriteAstExprWithPlanCtx
: build an expression with the plan contextutil.EvalAstExprWithPlanCtx
: evaluates an expression directly with the plan contextutil.ParseExprWithPlanCtx
: parse an expression from the string with the plan contextCheck List
Tests
Side effects
Documentation
Release note
Please refer to Release Notes Language Style Guide to write a quality release note.