Skip to content
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 cypher parameter(variable) #3379

Merged
merged 3 commits into from
Dec 21, 2021
Merged

Conversation

czpmango
Copy link
Contributor

@czpmango czpmango commented Nov 30, 2021

What type of PR is this?

  • bug
  • feature
  • enhancement

What does this PR do?

Support cypher parameter with ngql variable.

Which issue(s)/PR(s) this PR relates to?

#2593

Checklist:

  • Documentation affected (Please add the label if documentation needs to be modified.)
  • Incompatible (If it is incompatible, please describe it and add corresponding label.)
  • Need to cherry-pick (If need to cherry-pick to some branches, please label the destination version(s).)
  • Performance impacted: Consumes more CPU/Memory

Release notes:

Support for parameterized statement must be added to the release notes.

@czpmango czpmango added do not review PR: not ready for the code review yet ready-for-testing PR: ready for the CI test wip Solution: work in progress labels Nov 30, 2021
@czpmango czpmango force-pushed the varParam branch 3 times, most recently from b580610 to 7108b62 Compare December 2, 2021 08:21
@czpmango czpmango force-pushed the varParam branch 5 times, most recently from 37fcf81 to 5a2f6ad Compare December 7, 2021 02:46
@czpmango czpmango removed do not review PR: not ready for the code review yet wip Solution: work in progress labels Dec 7, 2021
@czpmango czpmango requested a review from CPWstatic December 7, 2021 02:47
@Sophie-Xie Sophie-Xie added the doc affected PR: improvements or additions to documentation label Dec 7, 2021
@czpmango czpmango force-pushed the varParam branch 2 times, most recently from 36b147f to 5a7bb86 Compare December 9, 2021 03:08
@czpmango czpmango force-pushed the varParam branch 2 times, most recently from 4c6ddb3 to 0090632 Compare December 13, 2021 02:35
Copy link
Contributor

@jievince jievince left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we really decide to unify variables and parameters, this PR is ok to me.

Comment on lines +117 to +120
folly::Future<ExecutionResponse> GraphService::future_executeWithParameter(
int64_t sessionId,
const std::string& query,
const std::unordered_map<std::string, Value>& parameterMap) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we unify future_executeWithParameter and future_execute? They share almost the same code.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After nebula-python's pr been merged.

src/graph/visitor/EvaluableExprVisitor.cpp Show resolved Hide resolved
src/parser/parser.yy Show resolved Hide resolved
tests/Makefile Outdated Show resolved Hide resolved
@czpmango czpmango force-pushed the varParam branch 3 times, most recently from 8412d5c to 8edc63d Compare December 15, 2021 05:58
@codecov-commenter
Copy link

codecov-commenter commented Dec 16, 2021

Codecov Report

Merging #3379 (9c5d706) into master (53b85dc) will decrease coverage by 0.00%.
The diff coverage is 75.12%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3379      +/-   ##
==========================================
- Coverage   85.19%   85.19%   -0.01%     
==========================================
  Files        1306     1307       +1     
  Lines      122158   122238      +80     
==========================================
+ Hits       104078   104139      +61     
- Misses      18080    18099      +19     
Impacted Files Coverage Δ
src/common/expression/VariableExpression.cpp 84.84% <ø> (-0.45%) ⬇️
...rc/graph/executor/query/AppendVerticesExecutor.cpp 98.30% <ø> (ø)
src/graph/executor/query/GetEdgesExecutor.cpp 94.00% <ø> (ø)
src/graph/executor/query/GetVerticesExecutor.cpp 96.77% <ø> (ø)
src/graph/executor/query/IndexScanExecutor.cpp 88.88% <ø> (ø)
src/graph/optimizer/rule/IndexScanRule.h 100.00% <ø> (ø)
...mizer/rule/PushLimitDownEdgeIndexRangeScanRule.cpp 20.00% <0.00%> (-0.69%) ⬇️
...raph/optimizer/rule/PushLimitDownIndexScanRule.cpp 14.81% <0.00%> (-0.57%) ⬇️
...imizer/rule/PushLimitDownTagIndexRangeScanRule.cpp 20.00% <0.00%> (-0.69%) ⬇️
src/graph/planner/plan/Query.h 97.22% <ø> (ø)
... and 64 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 53b85dc...9c5d706. Read the comment docs.

src/parser/parser.yy Outdated Show resolved Hide resolved
tests/tck/features/yield/parameter.feature Show resolved Hide resolved
@CPWstatic CPWstatic merged commit 9ea4889 into vesoft-inc:master Dec 21, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
doc affected PR: improvements or additions to documentation ready-for-testing PR: ready for the CI test
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants