-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Param MATCH vid seek fix #4024
Param MATCH vid seek fix #4024
Conversation
325aced
to
2eb5da4
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.
Good job! Does it work now? I'm looking forward to the test cases.
Both |
Please add tck test cases. |
What is the behavior in this case?
|
Thanks for the reminding.
|
I am preparing for the tck cases, while it's strange that this case passes. nebula/tests/tck/features/yield/parameter.feature Lines 19 to 28 in 1e68162
The behavior from console is shown as this:
Update: Now I came to know why, this case passed due to it's underlying selected to start from index:
|
done root@1827b82e88bf:/home/nebula/tests# python3 -m pytest -m "wey"
===================================================================================== test session starts =====================================================================================
platform linux -- Python 3.8.10, pytest-5.3.2, py-1.11.0, pluggy-0.13.1 -- /usr/bin/python3
cachedir: .pytest_cache
benchmark: 3.2.3 (defaults: timer=time.perf_counter disable_gc=False min_rounds=5 min_time=0.000005 max_time=1.0 calibration_precision=10 warmup=False warmup_iterations=100000)
metadata: {'Python': '3.8.10', 'Platform': 'Linux-5.4.0-94-generic-x86_64-with-glibc2.29', 'Packages': {'pytest': '5.3.2', 'py': '1.11.0', 'pluggy': '0.13.1'}, 'Plugins': {'reportlog': '0.1.0', 'benchmark': '3.2.3', 'forked': '1.4.0', 'metadata': '1.8.0', 'yapf3': '0.5.1', 'html': '2.0.1', 'xdist': '1.31.0', 'bdd': '4.0.2', 'drop-dup-tests': '0.3.0'}}
rootdir: /home/nebula/tests, inifile: pytest.ini
plugins: reportlog-0.1.0, benchmark-3.2.3, forked-1.4.0, metadata-1.8.0, yapf3-0.5.1, html-2.0.1, xdist-1.31.0, bdd-4.0.2, drop-dup-tests-0.3.0
collecting ... Generating LALR tables
collected 1354 items / 1350 deselected / 4 selected
tck/steps/test_tck.py::test_return_parameters PASSED [ 25%]
tck/steps/test_tck.py::test_cypher_with_parameters PASSED [ 50%]
tck/steps/test_tck.py::test_ngql_with_parameters PASSED [ 75%]
tck/steps/test_tck.py::test_error_check_2 PASSED [100%]
======================================================================= 4 passed, 1350 deselected, 4 warnings in 1.27s ======================================================================== |
I see mem leak in ci, it's not relevant to this PR, right? |
Yep. |
Fixing MATCH (n) WHERE id(n) IN [$t] RETURN n
Addressing Kyle's review comment
It's now rebased with all comments resolved. I will trigger review again when full CI passed. |
match (n) where n in $list_var return n
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. Nice fix!
MATCH (v) WHERE id(v) == $p7.a.b.d[4] RETURN v
@CPWstatic , two more commits since last review
@czpmango , one more commit since last review
Thanks! |
CI failed due to:
|
Codecov Report
@@ Coverage Diff @@
## master #4024 +/- ##
==========================================
- Coverage 85.10% 85.06% -0.04%
==========================================
Files 1339 1324 -15
Lines 132245 131657 -588
==========================================
- Hits 112544 111998 -546
+ Misses 19701 19659 -42
Continue to review full report at Codecov.
|
* Add qctx to isEvaluableExpr for kRelIn Fixing MATCH (n) WHERE id(n) IN [$t] RETURN n * Fix eq kVal parameter vidseek * Fix id(n) IN [$var] case * lint add explicit and remove tailing semicolon * clang-format-10 * double check type of rightListValue Addressing Kyle's review comment * ut added in features/yield/parameter.feature * support kAttribute match (n) where n in $list_var return n * Add kSubscript for eq case MATCH (v) WHERE id(v) == $p7.a.b.d[4] RETURN v Co-authored-by: Sophie <84560950+Sophie-Xie@users.noreply.github.com> Co-authored-by: kyle.cao <kyle.cao@vesoft.com> Co-authored-by: cpw <13495049+CPWstatic@users.noreply.github.com>
* fix memory leak (#4103) * Param MATCH vid seek fix (#4024) * Add qctx to isEvaluableExpr for kRelIn Fixing MATCH (n) WHERE id(n) IN [$t] RETURN n * Fix eq kVal parameter vidseek * Fix id(n) IN [$var] case * lint add explicit and remove tailing semicolon * clang-format-10 * double check type of rightListValue Addressing Kyle's review comment * ut added in features/yield/parameter.feature * support kAttribute match (n) where n in $list_var return n * Add kSubscript for eq case MATCH (v) WHERE id(v) == $p7.a.b.d[4] RETURN v Co-authored-by: Sophie <84560950+Sophie-Xie@users.noreply.github.com> Co-authored-by: kyle.cao <kyle.cao@vesoft.com> Co-authored-by: cpw <13495049+CPWstatic@users.noreply.github.com> * disable console pkg (#4110) * disable console pkg * fix * disable console pkg (#4112) * Fix service crash caused by using function call as a part of the filter in `LOOKUP` (#4111) * Fix function call purity check * Add UT for purity check * Add TCK cases * Fix cmake command error (#4114) Co-authored-by: yaphet <4414314+darionyaphet@users.noreply.github.com> Co-authored-by: Wey Gu <weyl.gu@gmail.com> Co-authored-by: kyle.cao <kyle.cao@vesoft.com> Co-authored-by: cpw <13495049+CPWstatic@users.noreply.github.com> Co-authored-by: Yichen Wang <18348405+Aiee@users.noreply.github.com> Co-authored-by: Yee <2520865+yixinglu@users.noreply.github.com>
What type of PR is this?
What problem(s) does this PR solve?
Issue(s) number: #3982
Description:
Fixed this case:
How do you solve it?
To make right part of the expression evaluated in
src/graph/visitor/VidExtractVisitor.cpp
Special notes for your reviewer, ex. impact of this fix, design document, etc:
Checklist:
Tests:
Affects:
Release notes:
Please confirm whether to be reflected in release notes and how to describe:
Fixed the bug on parameters for
id(n) == $var
,id(n) IN [$var]
,id(n) == $var.foo.bar
,id(n) IN $var.foo.bar