fix bug：Using the same statement to return same vertex different properties, the results show BAD TYPE. #4151

zhaohaifei · 2022-04-13T11:46:35Z

What type of PR is this?

bug
feature
enhancement

What problem(s) does this PR solve?

Issue(s) number:

#4076

Description:

Using the same statement to return same vertex different properties, the results show BAD TYPE.

How do you solve it?

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

Checklist:

Tests:

Unit test(positive and negative cases)
Function test
Performance test
N/A

Affects:

Documentation affected (Please add the label if documentation needs to be modified.)
Incompatibility (If it breaks the compatibility, please describe it and add the label.）
If it's needed to cherry-pick (If cherry-pick to some branches is required, please label the destination version(s).)
Performance impacted: Consumes more CPU/Memory

Release notes:

Please confirm whether to be reflected in release notes and how to describe:

ex. Fixed the bug .....
Fixed the bug：Using the same statement to return same vertex different properties, the results show BAD TYPE.

czpmango · 2022-04-14T12:03:54Z

src/graph/validator/MatchValidator.cpp

@@ -818,9 +823,16 @@ Status MatchValidator::validateGroup(YieldClauseContext &yieldCtx,

        yieldCtx.needGenProject_ = true;
        yieldCtx.aggOutputColumnNames_.emplace_back(agg->toString());
+
+        for (auto *labelExpr : labelExprs) {
+          yieldCtx.groupKeys_.emplace_back(labelExpr->clone());


match (v) return {abs(v.age),count(v.name)+1}
You should groupby abs(v.age) rather than v.age.

czpmango · 2022-04-19T10:29:15Z

Please add test cases related to the issue you want to fix.

czpmango · 2022-04-19T10:30:04Z

src/graph/visitor/ExtractGroupSuiteVisitor.h

+namespace nebula {
+namespace graph {
+
+class ExtractGroupSuiteVisitor : public ExprVisitorImpl {


Please add unit test for ExtractGroupSuiteVisitor.

codecov-commenter · 2022-04-19T14:21:46Z

Codecov Report

Merging #4151 (206eb69) into master (26f8fc8) will increase coverage by 0.06%.
The diff coverage is 86.73%.

@@            Coverage Diff             @@
##           master    #4151      +/-   ##
==========================================
+ Coverage   84.94%   85.01%   +0.06%     
==========================================
  Files        1319     1326       +7     
  Lines      131077   131682     +605     
==========================================
+ Hits       111343   111949     +606     
+ Misses      19734    19733       -1

Impacted Files	Coverage Δ
src/clients/meta/MetaClient.h	`92.30% <ø> (ø)`
src/graph/context/ast/CypherAstContext.h	`100.00% <ø> (ø)`
...rc/graph/executor/algo/MultiShortestPathExecutor.h	`100.00% <ø> (ø)`
src/graph/service/GraphFlags.cpp	`100.00% <ø> (ø)`
src/graph/util/ExpressionUtils.h	`100.00% <ø> (ø)`
src/meta/MetaServiceHandler.h	`100.00% <ø> (ø)`
.../processors/admin/VerifyClientVersionProcessor.cpp	`100.00% <ø> (ø)`
src/graph/visitor/ExtractGroupSuiteVisitor.cpp	`59.39% <59.39%> (ø)`
...eta/processors/admin/SaveGraphVersionProcessor.cpp	`66.66% <66.66%> (ø)`
src/clients/meta/MetaClient.cpp	`76.96% <80.64%> (+0.15%)`	⬆️
... and 87 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 607330d...206eb69. Read the comment docs.

czpmango · 2022-04-20T02:04:36Z

src/graph/visitor/test/VisitorTestBase.h

+  AggregateExpression *aggExpr(const std::string &name, Expression *arg, bool distinct) {
+    return AggregateExpression::make(pool, name, arg, distinct);
+  }
+
+  VertexExpression *vertexExpr(const std::string &name) {
+    return VertexExpression::make(pool, name);
+  }
+
+  PredicateExpression *predExpr(const std::string &name = "",
+                                const std::string &innerVar = "",
+                                Expression *collection = nullptr,
+                                Expression *filter = nullptr) {
+    return PredicateExpression::make(pool, name, innerVar, collection, filter);
+  }
+
+  ListComprehensionExpression *lcExpr(const std::string &innerVar = "",
+                                      Expression *collection = nullptr,
+                                      Expression *filter = nullptr,
+                                      Expression *mapping = nullptr) {
+    return ListComprehensionExpression::make(pool, innerVar, collection, filter, mapping);
+  }
+
+  ReduceExpression *reduceExpr(const std::string &accumulator = "",
+                               Expression *initial = nullptr,
+                               const std::string &innerVar = "",
+                               Expression *collection = nullptr,
+                               Expression *mapping = nullptr) {
+    return ReduceExpression::make(pool, accumulator, initial, innerVar, collection, mapping);
+  }
+
+  SubscriptRangeExpression *srExpr(Expression *list = nullptr,
+                                   Expression *lo = nullptr,
+                                   Expression *hi = nullptr) {
+    return SubscriptRangeExpression::make(pool, list, lo, hi);
+  }
+


I need the method

Just wrap Expression::make?

emmm, just like other method.

czpmango · 2022-04-20T02:10:00Z

src/graph/visitor/test/ExtractGroupSuiteVisitorTest.cpp

+  }
+};
+
+TEST_F(ExtractGroupSuiteVisitorTest, TestConstantExpression) {


You should test some nested expression scenarios.

Yes. I do. TEST_F(ExtractGroupSuiteVisitorTest, TestUnaryExpression2)

Add test case:
(abs(v.age)+1)+count(v.name)

czpmango · 2022-04-20T02:12:02Z

Any tck case?

Shylock-Hg · 2022-04-20T02:13:17Z

Add test in tck?

CPWstatic

Excellent!! But remember that we should add tck test for a bug fix like this one.

CPWstatic · 2022-04-20T02:11:01Z

src/graph/util/ExpressionUtils.h

@@ -73,6 +73,10 @@ class ExpressionUtils {
  // rewrite Agg to VarProp
  static Expression* rewriteAgg2VarProp(const Expression* expr);

+  // review the subExprs which are parts of expr.


review -> rewrite
rewrite subExprs to VariablePropertyExpression

CPWstatic · 2022-04-20T02:20:34Z

src/graph/visitor/ExtractGroupSuiteVisitor.cpp

+  auto specialExprs =
+      ExpressionUtils::collectAll(expr, {Expression::Kind::kVar, Expression::Kind::kAttribute});
+  for (auto *s : specialExprs) {
+    if (s->kind() == Expression::Kind::kVar &&
+        static_cast<const VariableExpression *>(s)->isInner()) {
+      return;
+    }
+    if (s->kind() == Expression::Kind::kAttribute) {
+      auto *left = static_cast<const AttributeExpression *>(s)->left();
+      if (left->kind() == Expression::Kind::kVar &&
+          static_cast<const VariableExpression *>(left)->isInner()) {
+        return;
+      }
+    }
+  }


Please add some comments here, i believe that these cases happend to only PredicateExpression, ListComprehensionExpression, ReduceExpression

czpmango · 2022-04-20T06:12:49Z

src/graph/visitor/test/ExtractGroupSuiteVisitorTest.cpp

+TEST_F(ExtractGroupSuiteVisitorTest, TestArithmeticExpression2) {
+  auto* left = constantExpr(0);
+  auto* right = aggExpr("count", constantExpr(1), false);
+  auto* e = minusExpr(left, right);
+
+  ExtractGroupSuiteVisitor visitor;
+  e->accept(&visitor);
+
+  GroupSuite expect;
+  expect.groupKeys.push_back(left);
+  expect.groupItems.push_back(left);
+  expect.groupItems.push_back(right);
+
+  ASSERT_EQ(true, check(visitor.groupSuite(), expect));
+}


That's incorrect.
v.age+count(v.name) key: v.age items: v.age,count(v.name)
1+count(v.name) item: count(v.name)

You mean, that constantExpr doesn't need to put groupkeys and groupItems?

czpmango · 2022-04-20T06:13:11Z

src/graph/visitor/test/ExtractGroupSuiteVisitorTest.cpp

+TEST_F(ExtractGroupSuiteVisitorTest, TestRelationalExpression) {
+  auto* left = constantExpr(0);
+  auto* right = aggExpr("count", constantExpr(1), false);
+  auto* e = neExpr(left, right);
+
+  ExtractGroupSuiteVisitor visitor;
+  e->accept(&visitor);
+
+  GroupSuite expect;
+  expect.groupKeys.push_back(left);
+  expect.groupItems.push_back(left);
+  expect.groupItems.push_back(right);
+
+  ASSERT_EQ(true, check(visitor.groupSuite(), expect));
+}


czpmango · 2022-04-20T06:15:27Z

src/graph/visitor/test/ExtractGroupSuiteVisitorTest.cpp

+TEST_F(ExtractGroupSuiteVisitorTest, TestLogicalExpression2) {
+  auto* left = constantExpr(0);
+  auto* right = aggExpr("count", constantExpr(1), false);
+  auto* e = orExpr(left, right);
+
+  ExtractGroupSuiteVisitor visitor;
+  e->accept(&visitor);
+
+  GroupSuite expect;
+  expect.groupKeys.push_back(left);
+  expect.groupItems.push_back(left);
+  expect.groupItems.push_back(right);
+
+  ASSERT_EQ(true, check(visitor.groupSuite(), expect));
+}


czpmango · 2022-04-20T06:29:02Z

src/graph/visitor/ExtractGroupSuiteVisitor.cpp

+    if (s->kind() == Expression::Kind::kAttribute) {
+      auto *left = static_cast<const AttributeExpression *>(s)->left();
+      if (left->kind() == Expression::Kind::kVar &&
+          static_cast<const VariableExpression *>(left)->isInner()) {
+        return;
+      }


Why need this?

It is for PredicateExpression, ListComprehensionExpression or ReduceExpression. It will check the innerVar.

Why collect AttributeExpression, but still check VariableExpression?
Is this code redundant?

czpmango · 2022-04-20T06:30:47Z

src/graph/validator/MatchValidator.cpp

+          if (!ExpressionUtils::checkAggExpr(static_cast<const AggregateExpression *>(item)).ok()) {
+            return Status::SemanticError("Aggregate function nesting is not allowed: `%s'",
+                                         colExpr->toString().c_str());
+          }


NG_RETURN_IF_ERROR(ExpressionUtils::checkAggExpr(static_cast<const AggregateExpression *>(item)))

src/graph/util/ExpressionUtils.cpp

czpmango · 2022-04-20T07:00:20Z

tests/tck/features/aggregate/Agg.feature

@@ -505,6 +505,23 @@ Feature: Basic Aggregate and GroupBy
    Then the result should be, in order, with relax comparison:
      | b    |
      | True |
+    When executing query:


Add test case:

MATCH (v:player{name:"Tim Duncan"})--(n:team) return [n in collect(v.player.age) where n>40| n]

czpmango · 2022-04-20T07:02:15Z

src/graph/visitor/ExtractGroupSuiteVisitor.cpp

+    if (s->kind() == Expression::Kind::kVar &&
+        static_cast<const VariableExpression *>(s)->isInner()) {
+      return;
+    }


Does this mean that no keys and items will be added if any subexpression is inner var?

en. Is it incorrect?

zhaohaifei added ready-for-testing PR: ready for the CI test wip Solution: work in progress labels Apr 13, 2022

Sophie-Xie added the cherry-pick-v3.1 PR: need cherry-pick to this version label Apr 13, 2022

zhaohaifei changed the title ~~fix bug~~ fix bug：Using the same statement to return same vertex different properties, the results show BAD TYPE. Apr 14, 2022

zhaohaifei force-pushed the bug_4076 branch from b0c1547 to 160cc14 Compare April 14, 2022 10:28

czpmango reviewed Apr 14, 2022

View reviewed changes

zhaohaifei force-pushed the bug_4076 branch from aca5382 to ea9e050 Compare April 18, 2022 05:23

zhaohaifei added ready for review and removed wip Solution: work in progress labels Apr 19, 2022

czpmango reviewed Apr 19, 2022

View reviewed changes

Sophie-Xie requested a review from CPWstatic April 19, 2022 11:40

CPWstatic previously approved these changes Apr 19, 2022

View reviewed changes

zhaohaifei dismissed CPWstatic’s stale review via 2fcccd8 April 19, 2022 13:12

zhaohaifei force-pushed the bug_4076 branch from 33453af to 2fcccd8 Compare April 19, 2022 13:12

czpmango reviewed Apr 20, 2022

View reviewed changes

CPWstatic reviewed Apr 20, 2022

View reviewed changes

zhaohaifei added 9 commits April 20, 2022 10:51

fix bug

fc01567

temp commit

710bbda

temp commit

66641da

temp commit

7ee80c5

temp commit

9bd9f84

temp commit

0c9e80e

temp commit

89e00fe

temp commit

2a45540

complete code

661cbdf

complete

96a4974

CPWstatic previously approved these changes Apr 20, 2022

View reviewed changes

czpmango reviewed Apr 20, 2022

View reviewed changes

src/graph/util/ExpressionUtils.cpp Show resolved Hide resolved

czpmango reviewed Apr 20, 2022

View reviewed changes

complete

ddabee3

zhaohaifei dismissed CPWstatic’s stale review via ddabee3 April 20, 2022 12:47

zhaohaifei requested review from czpmango and CPWstatic April 20, 2022 13:01

CPWstatic previously approved these changes Apr 20, 2022

View reviewed changes

Sophie-Xie removed the cherry-pick-v3.1 PR: need cherry-pick to this version label Apr 21, 2022

zhaohaifei added 2 commits April 22, 2022 17:54

temp commit

1b015f0

temp commit

cce25b7

zhaohaifei dismissed CPWstatic’s stale review via cce25b7 April 22, 2022 09:56

zhaohaifei force-pushed the bug_4076 branch from 90b88a6 to cce25b7 Compare April 22, 2022 09:56

zhaohaifei added 2 commits April 25, 2022 11:52

temp commit

758a2b4

temp commit

206eb69

Sophie-Xie added this to the v3.2.0 milestone Apr 25, 2022

czpmango approved these changes Apr 25, 2022

View reviewed changes

Merge branch 'master' into bug_4076

290e6ee

CPWstatic approved these changes Apr 26, 2022

View reviewed changes

Sophie-Xie merged commit 4026d75 into vesoft-inc:master Apr 26, 2022

zhaohaifei deleted the bug_4076 branch April 26, 2022 03:51

This was referenced May 5, 2022

Weekly Report 2022-04-30 vesoft-inc/nebula-community#106

Closed

Weekly Report 2022-04-30 vesoft-inc/nebula-community#107

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix bug：Using the same statement to return same vertex different properties, the results show BAD TYPE. #4151

fix bug：Using the same statement to return same vertex different properties, the results show BAD TYPE. #4151

zhaohaifei commented Apr 13, 2022 •

edited

Loading

czpmango Apr 14, 2022

czpmango commented Apr 19, 2022

czpmango Apr 19, 2022 •

edited

Loading

codecov-commenter commented Apr 19, 2022 •

edited

Loading

czpmango Apr 20, 2022

zhaohaifei Apr 20, 2022

zhaohaifei Apr 20, 2022

czpmango Apr 20, 2022

zhaohaifei Apr 20, 2022

czpmango Apr 20, 2022

zhaohaifei Apr 20, 2022

czpmango Apr 20, 2022

czpmango commented Apr 20, 2022

Shylock-Hg commented Apr 20, 2022

CPWstatic left a comment

CPWstatic Apr 20, 2022

CPWstatic Apr 20, 2022

czpmango Apr 20, 2022

zhaohaifei Apr 20, 2022

czpmango Apr 20, 2022

czpmango Apr 20, 2022

czpmango Apr 20, 2022

zhaohaifei Apr 20, 2022

czpmango Apr 20, 2022

czpmango Apr 20, 2022

czpmango Apr 20, 2022

czpmango Apr 20, 2022

zhaohaifei Apr 20, 2022

fix bug：Using the same statement to return same vertex different properties, the results show BAD TYPE. #4151

fix bug：Using the same statement to return same vertex different properties, the results show BAD TYPE. #4151

Conversation

zhaohaifei commented Apr 13, 2022 • edited Loading

What type of PR is this?

What problem(s) does this PR solve?

Issue(s) number:

Description:

How do you solve it?

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

Checklist:

Release notes:

Choose a reason for hiding this comment

czpmango commented Apr 19, 2022

czpmango Apr 19, 2022 • edited Loading

Choose a reason for hiding this comment

codecov-commenter commented Apr 19, 2022 • edited Loading

Codecov Report

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

czpmango commented Apr 20, 2022

Shylock-Hg commented Apr 20, 2022

CPWstatic 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

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

zhaohaifei commented Apr 13, 2022 •

edited

Loading

czpmango Apr 19, 2022 •

edited

Loading

codecov-commenter commented Apr 19, 2022 •

edited

Loading