enhance match attribute filter #3272

nevermore3 · 2021-11-04T08:40:53Z

What type of PR is this?

enhancement

What does this PR do?

close #3270

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

Special notes for your reviewer, ex. impact of this fix, etc:

Additional context:

Checklist：

Release notes： `

Shylock-Hg · 2021-11-04T08:48:11Z

Add test cases.

Shylock-Hg · 2021-11-04T09:31:06Z

tests/tck/features/match/Base.feature

+      | e |
+    When executing query:
+      """
+      match (v:player{age:"24"-1})  return v


This will report error in cypher.

czpmango · 2021-11-05T09:59:13Z

src/parser/MatchSentence.h

+  MapExpression* props() { return props_; }
+


Why delete const?

czpmango · 2021-11-05T10:06:18Z

src/graph/validator/MatchValidator.cpp

 StatusOr<Expression *> MatchValidator::makeSubFilter(const std::string &alias,
-                                                     const MapExpression *map,
-                                                     const std::string &label) const {
+                                                     MapExpression *map,
+                                                     const std::string &label) {


Expressions related implementation changes should not change the Interface of the Validator.

need to rewrite the expression after fold into the mapexpression

Shylock-Hg · 2021-11-08T07:20:00Z

tests/tck/features/expression/function/TypeConversion.feature


  Scenario: toInteger
    When executing query:
      """
-      YIELD [toInteger(true), toInteger(false), toInteger(1), toInteger(3.14),


Why change this case?

toInteger(false), toInteger(true) will report error in opencypher, other modified cases too

I think you should keep the origin case, and add new one to test the origin function.

i know, but the result of origin case is wrong, and the modified logic test has been covered

codecov-commenter · 2021-11-08T07:29:47Z

Codecov Report

Merging #3272 (70099cd) into master (58f0b44) will decrease coverage by 0.00%.
The diff coverage is 71.42%.

@@            Coverage Diff             @@
##           master    #3272      +/-   ##
==========================================
- Coverage   85.24%   85.23%   -0.01%     
==========================================
  Files        1295     1295              
  Lines      118190   118210      +20     
==========================================
+ Hits       100748   100762      +14     
- Misses      17442    17448       +6

Impacted Files	Coverage Δ
src/graph/validator/MatchValidator.h	`100.00% <ø> (ø)`
src/graph/visitor/test/FilterTransformTest.cpp	`100.00% <ø> (ø)`
src/parser/MatchSentence.h	`100.00% <ø> (ø)`
src/graph/validator/MatchValidator.cpp	`86.74% <55.55%> (-0.53%)`	⬇️
src/graph/visitor/FoldConstantExprVisitor.cpp	`85.12% <100.00%> (+0.19%)`	⬆️
src/meta/processors/parts/GetSpaceProcessor.cpp	`60.60% <0.00%> (-18.19%)`	⬇️
src/graph/planner/Planner.cpp	`75.00% <0.00%> (-5.00%)`	⬇️
...eta/processors/session/SessionManagerProcessor.cpp	`74.19% <0.00%> (-4.52%)`	⬇️
src/common/expression/LogicalExpression.cpp	`91.66% <0.00%> (-1.73%)`	⬇️
src/graph/session/GraphSessionManager.cpp	`79.88% <0.00%> (-1.73%)`	⬇️
... and 25 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 58f0b44...70099cd. Read the comment docs.

CPWstatic

LGTM. Please fix the conflicts.

yixinglu

LGTM

cangfengzhs

LGTM. But, I have a small question. Is it reasonable to expose BAT_TYPE to users? I haven't learned about GQL and opencypher. If they are like this, that's OK. But if they are not like this, is it necessary for us to standardize the null type we return to user? After all, our Value has many kinds of null type.

jievince · 2021-11-22T09:56:03Z

LGTM. But, I have a small question. Is it reasonable to expose BAT_TYPE to users? I haven't learned about GQL and opencypher. If they are like this, that's OK. But if they are not like this, is it necessary for us to standardize the null type we return to user? After all, our Value has many kinds of null type.

Neither of them return BAD_TYPE, so the NULL, BAD_TYPE should be reconsidered

czpmango · 2022-01-05T02:53:01Z

LGTM. But, I have a small question. Is it reasonable to expose BAT_TYPE to users? I haven't learned about GQL and opencypher. If they are like this, that's OK. But if they are not like this, is it necessary for us to standardize the null type we return to user? After all, our Value has many kinds of null type.

All bad nulls is actually just an error with an error message. It may be more reasonable to provide a Value overload with customizable error messages, bcz current expressions’ ability to report and transmit errors is very limited.

nevermore3 force-pushed the enhance_match_filter branch from b194389 to 3e85b59 Compare November 4, 2021 09:23

nevermore3 added type/enhancement Type: make the code neat or more efficient ready-for-testing PR: ready for the CI test labels Nov 4, 2021

nevermore3 force-pushed the enhance_match_filter branch 2 times, most recently from 5c9ce6c to 9314898 Compare November 4, 2021 09:29

Shylock-Hg reviewed Nov 4, 2021

View reviewed changes

nevermore3 force-pushed the enhance_match_filter branch from dc7e184 to 842bbb1 Compare November 5, 2021 07:03

nevermore3 requested review from yixinglu, czpmango, CPWstatic and Shylock-Hg November 5, 2021 07:16

czpmango reviewed Nov 5, 2021

View reviewed changes

src/parser/MatchSentence.h

Comment on lines +166 to +169

MapExpression* props() { return props_; }

Copy link

Contributor

czpmango Nov 5, 2021

Choose a reason for hiding this comment

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

Why delete const?

czpmango reviewed Nov 5, 2021

View reviewed changes

nevermore3 force-pushed the enhance_match_filter branch 2 times, most recently from 002cfc3 to 70099cd Compare November 8, 2021 06:07

Shylock-Hg reviewed Nov 8, 2021

View reviewed changes

CPWstatic previously approved these changes Nov 18, 2021

View reviewed changes

Sophie-Xie removed the request for review from yixinglu November 22, 2021 02:31

nevermore3 added 6 commits November 22, 2021 15:57

enhance match attribute filter

744fd5b

add test case

b9af8ce

fix test error

a54bf40

delete some null test

f6a2e54

fix test error

c02c2ca

fix storage crash

f2d46e4

nevermore3 dismissed CPWstatic’s stale review via f2d46e4 November 22, 2021 07:58

nevermore3 force-pushed the enhance_match_filter branch from 660f3c6 to f2d46e4 Compare November 22, 2021 07:58

yixinglu approved these changes Nov 22, 2021

View reviewed changes

cangfengzhs approved these changes Nov 22, 2021

View reviewed changes

CPWstatic merged commit b575189 into vesoft-inc:master Nov 22, 2021

nevermore3 deleted the enhance_match_filter branch November 22, 2021 16:21

jamieliu1023 mentioned this pull request Nov 27, 2021

Weekly Report 2021-11-26 vesoft-inc/nebula-community#49

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

enhance match attribute filter #3272

enhance match attribute filter #3272

nevermore3 commented Nov 4, 2021

Shylock-Hg commented Nov 4, 2021

Shylock-Hg Nov 4, 2021

nevermore3 Nov 5, 2021

czpmango Nov 5, 2021

czpmango Nov 5, 2021

nevermore3 Nov 8, 2021

Shylock-Hg Nov 8, 2021

nevermore3 Nov 8, 2021

Shylock-Hg Nov 8, 2021

nevermore3 Nov 8, 2021

codecov-commenter commented Nov 8, 2021

CPWstatic left a comment

yixinglu left a comment

cangfengzhs left a comment

jievince commented Nov 22, 2021 •

edited by czpmango

czpmango commented Jan 5, 2022

enhance match attribute filter #3272

enhance match attribute filter #3272

Conversation

nevermore3 commented Nov 4, 2021

What type of PR is this?

What does this PR do?

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

Special notes for your reviewer, ex. impact of this fix, etc:

Additional context:

Checklist：

Release notes： `

Shylock-Hg commented Nov 4, 2021

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-commenter commented Nov 8, 2021

Codecov Report

CPWstatic left a comment

Choose a reason for hiding this comment

yixinglu left a comment

Choose a reason for hiding this comment

cangfengzhs left a comment

Choose a reason for hiding this comment

jievince commented Nov 22, 2021 • edited by czpmango

czpmango commented Jan 5, 2022

jievince commented Nov 22, 2021 •

edited by czpmango