support cypher parameter #2798

czpmango · 2021-09-07T02:19:02Z

Related pr:

nebula-go [DNM] support cypher parameter nebula-go#130
nebula-console support cypher parameter nebula-console#127
nebula-python support parameter nebula-python#147

src/common/expression/ParameterExpression.h

src/graph/visitor/FoldConstantExprVisitor.cpp

src/common/expression/ParameterExpression.cpp

src/common/expression/ParameterExpression.h

src/interface/graph.thrift

src/graph/service/RequestContext.h

src/graph/context/QueryContext.cpp

src/graph/context/QueryContext.h

yixinglu

Does this PR not take into account the implementation of parameter binding after the execution plan has been optimized?

src/graph/service/GraphService.cpp

tests/Makefile

src/common/expression/ParameterExpression.cpp

tests/tck/features/yield/parameter.feature

codecov-commenter · 2021-09-28T10:50:57Z

Codecov Report

Merging #2798 (5497494) into master (3c7d00e) will decrease coverage by 0.08%.
The diff coverage is 87.37%.

@@            Coverage Diff             @@
##           master    #2798      +/-   ##
==========================================
- Coverage   84.42%   84.34%   -0.09%     
==========================================
  Files        1284     1286       +2     
  Lines      113589   113765     +176     
==========================================
+ Hits        95899    95951      +52     
- Misses      17690    17814     +124

Impacted Files	Coverage Δ
src/common/expression/ExprVisitor.h	`100.00% <ø> (ø)`
src/common/expression/Expression.h	`100.00% <ø> (ø)`
...c/common/expression/test/ExpressionContextMock.cpp	`100.00% <ø> (ø)`
src/common/expression/test/TestBase.h	`85.84% <ø> (ø)`
src/graph/service/GraphService.h	`100.00% <ø> (ø)`
src/graph/visitor/DeduceTypeVisitor.h	`100.00% <ø> (ø)`
src/graph/visitor/ExtractFilterExprVisitor.cpp	`69.38% <0.00%> (-2.96%)`	⬇️
src/graph/visitor/ExtractFilterExprVisitor.h	`100.00% <ø> (ø)`
src/graph/visitor/ExtractPropExprVisitor.h	`100.00% <ø> (ø)`
src/graph/visitor/FindVisitor.h	`100.00% <ø> (ø)`
... and 86 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3c7d00e...5497494. Read the comment docs.

czpmango · 2021-09-29T08:25:21Z

Does this PR not take into account the implementation of parameter binding after the execution plan has been optimized?

Parameter push-down is currently not supported.
refer #2593 for more detail.

src/graph/service/GraphService.cpp

CPWstatic · 2021-09-29T09:26:28Z

tests/tck/features/yield/parameter.feature

+    When executing query:
+      """
+      $p1=GO FROM "Tim Duncan" OVER like WHERE like.likeness>$p1 yield like._dst as dst;
+      GO FROM $p1.dst OVER like YIELD DISTINCT $$.player.name AS name


$p1=GO FROM "Tim Duncan" OVER like WHERE like.likeness>$p1 yield like._dst as dst; GO FROM $p1.dst OVER like Where like.age > $p1 YIELD DISTINCT $$.player.name AS name.

What is the behavior of this test case?
We need more test cases.

$p1 started as a parameter and was rewritten as a variable, just consistent with what we talked about earlier.

Such as what test cases？

Where did you do the rewriting?

Expression rewriting?

The implementation layer is the same as VariableExpression, it is based on valueMap_, so there is no need to do rewriting. valueMap_ is automatically written to at runtime.

$p1=GO FROM "Tim Duncan" OVER like WHERE like.likeness>$p1 yield like._dst as dst; GO FROM $p1.dst OVER like Where like.age > $p1 YIELD DISTINCT $$.player.name AS name.

What is the behavior of this test case? We need more test cases.

Variables and parameters with the same name are not supported for the time being, just consistent with our offline discussion.

CPWstatic · 2021-09-29T09:27:09Z

src/common/expression/ParameterExpression.h

+// The ParameterExpression use for parameterized statement
+namespace nebula {
+
+class ParameterExpression : public Expression {


I doubt if we need this one.

Is needed.
Such as lookupValidators or some visitor or scenes that require expression rewriting.

Some Example :

rewrite ParameterExpression to ConstantExpression

EvaluableExprVisitor determines whether expressions are evaluable by their specific types. Code likes that:

void visit(VariableExpression *) override { isEvaluable_ = false; }

If ParameterExpression and VariableExpression are unified, then we must use if-else whenever we need to distinguish them, I don't think this is a good code design pattern.

jievince

LGTM now

CPWstatic · 2021-10-07T02:39:06Z

tests/tck/features/go/GoYieldVertexEdge.feature

@@ -224,7 +224,7 @@ Feature: Go Yield Vertex And Edge Sentence
      """
      GO FROM $var OVER like YIELD edge as e
      """
-    Then a SyntaxError should be raised at runtime: syntax error near `OVER'
+    Then a SyntaxError should be raised at runtime: can not go from a nonexist parameter near `FROM'


What happens to this test case? Why do you need modify it?

Go from parameter.

src/graph/context/QueryContext.h

src/graph/validator/Validator.cpp

tests/tck/features/yield/parameter.feature

src/common/expression/ParameterExpression.cpp

src/parser/parser.yy

yixinglu · 2021-10-08T03:21:07Z

src/parser/parser.yy

@@ -2003,6 +2021,27 @@ fetch_vertices_sentence
    | KW_FETCH KW_PROP KW_ON STAR vid_ref_expression yield_clause {
        $$ = new FetchVerticesSentence($5, $6);
    }
+    | KW_FETCH KW_PROP KW_ON name_label_list VARIABLE yield_clause {


extend vid_ref_expression to check whether VARIABLE is parameter

vid_ref_ex}r_wrapper:
| vid_ref_expression {
if () {
throw exception("illegal variable")
}
return $$
|

fix aysn block revert nebula-python url disable homonymic parameter variable support sdk interface of map/list support go from parameter support list and map fix tck support fetch small fix for point initialization fix tck add tck support subgraph support find path add tck

czpmango added the type/enhancement Type: make the code neat or more efficient label Sep 7, 2021

czpmango self-assigned this Sep 7, 2021

czpmango added the ready-for-testing PR: ready for the CI test label Sep 7, 2021

czpmango force-pushed the new-param branch from a92c128 to ae85cbf Compare September 7, 2021 02:28

jievince reviewed Sep 7, 2021

View reviewed changes

src/common/expression/ParameterExpression.h Outdated Show resolved Hide resolved

src/graph/visitor/FoldConstantExprVisitor.cpp Show resolved Hide resolved

src/common/expression/ParameterExpression.cpp Show resolved Hide resolved

yixinglu reviewed Sep 8, 2021

View reviewed changes

czpmango force-pushed the new-param branch 2 times, most recently from 508a41a to d656073 Compare September 9, 2021 09:45

czpmango force-pushed the new-param branch 3 times, most recently from 78b4919 to fab02c1 Compare September 23, 2021 02:20

yixinglu reviewed Sep 23, 2021

View reviewed changes

czpmango added the wip Solution: work in progress label Sep 23, 2021

Sophie-Xie added this to the v2.6.0 milestone Sep 27, 2021

czpmango force-pushed the new-param branch 2 times, most recently from c8dbfc8 to d8cc949 Compare September 28, 2021 09:04

czpmango removed the wip Solution: work in progress label Sep 28, 2021

czpmango requested review from yixinglu, jievince and CPWstatic September 29, 2021 06:24

czpmango force-pushed the new-param branch 3 times, most recently from b94aac6 to 65c7e42 Compare September 29, 2021 07:26

jievince reviewed Sep 29, 2021

View reviewed changes

src/graph/service/GraphService.cpp Outdated Show resolved Hide resolved

src/graph/service/GraphService.cpp Outdated Show resolved Hide resolved

CPWstatic reviewed Sep 29, 2021

View reviewed changes

jievince previously approved these changes Sep 29, 2021

View reviewed changes

czpmango dismissed jievince’s stale review via 028f077 September 30, 2021 02:08

czpmango force-pushed the new-param branch from 4ca634a to 028f077 Compare September 30, 2021 02:08

Shylock-Hg requested review from yixinglu and jievince September 30, 2021 11:34

czpmango requested a review from Shylock-Hg September 30, 2021 11:44

czpmango force-pushed the new-param branch 7 times, most recently from 0fa92dd to 9d5c8c6 Compare October 5, 2021 11:32

CPWstatic reviewed Oct 7, 2021

View reviewed changes

jievince reviewed Oct 8, 2021

View reviewed changes

src/common/expression/ParameterExpression.cpp Outdated Show resolved Hide resolved

yixinglu reviewed Oct 8, 2021

View reviewed changes

czpmango force-pushed the new-param branch from 5497494 to a733719 Compare October 8, 2021 07:40

Sophie-Xie removed this from the v2.6.0 milestone Oct 8, 2021

czpmango force-pushed the new-param branch from a733719 to 1dcc863 Compare October 9, 2021 03:52

Sophie-Xie added this to the v2.7.0 milestone Oct 9, 2021

czpmango force-pushed the new-param branch 6 times, most recently from f3b717d to 86116aa Compare October 12, 2021 11:05

Sophie-Xie modified the milestones: v2.7.0, v3.0.0 Oct 15, 2021

czpmango force-pushed the new-param branch from 86116aa to 8cad6cd Compare October 15, 2021 06:31

Merge branch 'master' into new-param

47d747c

Sophie-Xie added the doc affected PR: improvements or additions to documentation label Nov 12, 2021

czpmango closed this Dec 1, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

support cypher parameter #2798

support cypher parameter #2798

czpmango commented Sep 7, 2021 •

edited

Loading

yixinglu left a comment

codecov-commenter commented Sep 28, 2021 •

edited

Loading

czpmango commented Sep 29, 2021

CPWstatic Sep 29, 2021

czpmango Sep 29, 2021 •

edited

Loading

CPWstatic Sep 29, 2021

czpmango Sep 29, 2021

czpmango Sep 29, 2021

czpmango Oct 2, 2021

CPWstatic Sep 29, 2021

czpmango Sep 29, 2021 •

edited

Loading

jievince left a comment

CPWstatic Oct 7, 2021

czpmango Oct 8, 2021

yixinglu Oct 8, 2021

yixinglu Oct 8, 2021

support cypher parameter #2798

support cypher parameter #2798

Conversation

czpmango commented Sep 7, 2021 • edited Loading

yixinglu left a comment

Choose a reason for hiding this comment

codecov-commenter commented Sep 28, 2021 • edited Loading

Codecov Report

czpmango commented Sep 29, 2021

Choose a reason for hiding this comment

czpmango Sep 29, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

czpmango Sep 29, 2021 • edited Loading

Choose a reason for hiding this comment

jievince left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

czpmango commented Sep 7, 2021 •

edited

Loading

codecov-commenter commented Sep 28, 2021 •

edited

Loading

czpmango Sep 29, 2021 •

edited

Loading

czpmango Sep 29, 2021 •

edited

Loading