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

ExtractFilterExprVisitor implementation bug #3372

Closed
czpmango opened this issue Nov 29, 2021 · 0 comments · Fixed by #3462
Closed

ExtractFilterExprVisitor implementation bug #3372

czpmango opened this issue Nov 29, 2021 · 0 comments · Fixed by #3462
Assignees
Labels
type/bug Type: something is unexpected
Milestone

Comments

@czpmango
Copy link
Contributor

czpmango commented Nov 29, 2021

Describe the bug (required)
Some expression processing by ExtractFilterExprVisitor may have semantic errors.

Your Environments (required)
Version: v2.6 and previous versions

How To Reproduce(required)
At present, it can only be reproduced by unit testing, bcz the ngql statement cannot trigger the error situation(ExtractFilterExprVisitor is only used by PushFilterDownGetNbrsRule and all expressions that produce semantic errors will not trigger this optimization rule).
Add the following code to ExtractFilterExprVisitorTest.cpp to reproduce this bug.
As you can see below, the expression semantics becomes ($^.player.name or false) and $$.player.age after the processing of ExtractFilterExprVisitor, which is not expected.

// FIXME: This unit test is invalid. It should be fixed and the correct result is in the comment
TEST_F(ExtractFilterExprVisitorTest, TestFilterSemanicError) {
  // $^.player.name or (false and $$.player.age)
  /*
   * Response:
   *   pushedExpr: $^.player.name or false
   *   remainedExpr: $$.player.age
   * Expected:
   *   pushedExpr: $^.player.name or false
   *   remainedExpr: $^.player.name or $$.player.age
   */
  auto dstExpr = DestPropertyExpression::make(pool_.get(), "player", "age");
  auto srcExpr = SourcePropertyExpression::make(pool_.get(), "player", "name");
  auto operand = andExpr(dstExpr, constantExpr(false));
  auto expr = orExpr(srcExpr, operand);
  ExtractFilterExprVisitor visitor(pool_.get());
  expr->accept(&visitor);
  ASSERT_TRUE(visitor.ok());
  auto rmexpr = std::move(visitor).remainedExpr();
  UNUSED(rmexpr);
  // FIXME: ASSERT_TRUE(false) is just for bug reproducing
  ASSERT_TRUE(false) << "remained expression: " << rmexpr->toString()
                     << "\npushed expression: " << expr->toString();
}

Expected behavior

pushedExpr: $^.player.name or false
remainedExpr: $^.player.name or $$.player.age

Additional context
The above example seems to be meaningless bcz the original expression can be simplified to $^.player.name(more details refer to the optimization rule BooleanSimplification of spark catalyst), then remainedExpr will be nullptr, which will generate one less Filter plan node.
So, what needs to be considered is whether the order of applying the optimization rules will affect the generation of the final execution plan.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type/bug Type: something is unexpected
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants