oceanbase / oceanbase/miniob

[BUG] The conjunction simplification returns wrong answer

Open
#637 1 comment 0 reactions 0 assignees View on GitHub

Nobody has claimed this yet.

bug
Dominant language
C++
Stars
4.4k
Forks
1.6k
PR merge metrics
No merged PRs in 30d

Description

Describe the bug
A clear and concise description of what the bug is.
Same to https://github.com/oceanbase/miniob/issues/440 .
That issue was tagged with question, however, it is a bug.
In the conjunction simplification, Expr1 AND False AND ... would be rewrote to Expr1, the first expression in the conjunction expression list, if Expr1 is not a ValueExpr. However, Expr1 AND False AND ... should be False.

Environment
Environment Details sometimes important

  • OS Version: CentOS 7
  • CPU Arch(x86/arm): x86
  • Compiler: clang20
  • Others:

Fast Reproduce Steps(Required)
Steps to reproduce the behavior:
I wrote a test to reproduce this:

#include "gtest/gtest.h"
#include <memory>
#include "common/sys/rc.h"
#include "common/value.h"
#include "common/lang/vector.h"
#include "sql/expr/expression.h"
#include "sql/expr/tuple.h"
#include "sql/optimizer/conjunction_simplification_rule.h"
class ConjunctionSimplificationRuleTest : public testing::Test
{
protected:
  static void test_conjunction_simplification_rule(
      unique_ptr<Expression> &&left, unique_ptr<Expression> &&right, ConjunctionExpr::Type conjunction_type)
  {
    ValueListTuple empty_tuple;
    Value          left_value;
    Value          right_value;
    EXPECT_TRUE(OB_SUCC(left->get_value(empty_tuple, left_value)));
    EXPECT_TRUE(OB_SUCC(right->get_value(empty_tuple, right_value)));

    vector<unique_ptr<Expression>> children_exprs;
    children_exprs.push_back(std::move(left));
    children_exprs.push_back(std::move(right));
    unique_ptr<Expression> conjunction_expr = make_unique<ConjunctionExpr>(conjunction_type, children_exprs);

    Value expect_value;
    switch (conjunction_type) {
      case ConjunctionExpr::Type::AND: {
        expect_value.set_boolean(left_value.get_boolean() && right_value.get_boolean());
      } break;
      case ConjunctionExpr::Type::OR: {
        expect_value.set_boolean(left_value.get_boolean() || right_value.get_boolean());
      } break;
    }

    Value origin_value;
    EXPECT_TRUE(OB_SUCC(conjunction_expr->get_value(empty_tuple, origin_value)));
    EXPECT_EQ(expect_value.get_boolean(), origin_value.get_boolean());

    ConjunctionSimplificationRule rule;
    bool                          change_made;
    EXPECT_TRUE(OB_SUCC(rule.rewrite(conjunction_expr, change_made)));
    //EXPECT_TRUE(change_made);
    Value simplified_value;
    EXPECT_TRUE(OB_SUCC(conjunction_expr->get_value(empty_tuple, simplified_value)));
    EXPECT_EQ(expect_value.get_boolean(), simplified_value.get_boolean());
  }

  static unique_ptr<Expression> generate_true_expr() { return make_unique<ValueExpr>(Value(true)); }

  static unique_ptr<Expression> generate_false_expr() { return make_unique<ValueExpr>(Value(false)); }

  static unique_ptr<Expression> generate_true_cmp_expr()
  {
    const auto ONE = make_unique<ValueExpr>(Value(1));
    return make_unique<ComparisonExpr>(CompOp::EQUAL_TO, ONE->copy(), ONE->copy());
  }

  static unique_ptr<Expression> generate_false_cmp_expr()
  {
    const auto ONE = make_unique<ValueExpr>(Value(1));
    return make_unique<ComparisonExpr>(CompOp::NOT_EQUAL, ONE->copy(), ONE->copy());
  }
};

TEST_F(ConjunctionSimplificationRuleTest, true_and_false_test)
{
  test_conjunction_simplification_rule(generate_true_expr(), generate_false_expr(), ConjunctionExpr::Type::AND);
  test_conjunction_simplification_rule(generate_true_cmp_expr(), generate_false_expr(), ConjunctionExpr::Type::AND);
  test_conjunction_simplification_rule(generate_true_expr(), generate_false_cmp_expr(), ConjunctionExpr::Type::AND);
}

TEST_F(ConjunctionSimplificationRuleTest, false_and_true_test)
{
  test_conjunction_simplification_rule(generate_false_expr(), generate_true_expr(), ConjunctionExpr::Type::AND);
  test_conjunction_simplification_rule(generate_false_expr(), generate_true_cmp_expr(), ConjunctionExpr::Type::AND);
  test_conjunction_simplification_rule(generate_false_cmp_expr(), generate_true_expr(), ConjunctionExpr::Type::AND);
}

TEST_F(ConjunctionSimplificationRuleTest, true_or_false_test)
{
  test_conjunction_simplification_rule(generate_true_expr(), generate_false_expr(), ConjunctionExpr::Type::OR);
  test_conjunction_simplification_rule(generate_true_cmp_expr(), generate_false_expr(), ConjunctionExpr::Type::OR);
  test_conjunction_simplification_rule(generate_true_expr(), generate_false_cmp_expr(), ConjunctionExpr::Type::OR);
}

TEST_F(ConjunctionSimplificationRuleTest, false_or_true_test)
{
  test_conjunction_simplification_rule(generate_false_expr(), generate_true_expr(), ConjunctionExpr::Type::OR);
  test_conjunction_simplification_rule(generate_false_expr(), generate_true_cmp_expr(), ConjunctionExpr::Type::OR);
  test_conjunction_simplification_rule(generate_false_cmp_expr(), generate_true_expr(), ConjunctionExpr::Type::OR);
}

int main(int argc, char **argv)
{
  // 分析gtest程序的命令行参数
  testing::InitGoogleTest(&argc, argv);

  // 调用RUN_ALL_TESTS()运行所有测试用例
  // main函数返回RUN_ALL_TESTS()的运行结果
  return RUN_ALL_TESTS();
}

Expected behavior
A clear and concise description of what you expected to happen.
Expect all tests passed.

Actual Behavior
What is the result? picture is allowed

[==========] Running 4 tests from 1 test suite.
[----------] Global test environment set-up.
[----------] 4 tests from ConjunctionSimplificationRuleTest
[ RUN      ] ConjunctionSimplificationRuleTest.true_and_false_test
/home/cqc/develop/miniob/unittest/observer/simplification_rule_test.cpp:46: Failure
Expected equality of these values:
  expect_value.get_boolean()
    Which is: false
  simplified_value.get_boolean()
    Which is: true
[  FAILED  ] ConjunctionSimplificationRuleTest.true_and_false_test (0 ms)
[ RUN      ] ConjunctionSimplificationRuleTest.false_and_true_test
[       OK ] ConjunctionSimplificationRuleTest.false_and_true_test (0 ms)
[ RUN      ] ConjunctionSimplificationRuleTest.true_or_false_test
[       OK ] ConjunctionSimplificationRuleTest.true_or_false_test (0 ms)
[ RUN      ] ConjunctionSimplificationRuleTest.false_or_true_test
/home/cqc/develop/miniob/unittest/observer/simplification_rule_test.cpp:46: Failure
Expected equality of these values:
  expect_value.get_boolean()
    Which is: true
  simplified_value.get_boolean()
    Which is: false
[  FAILED  ] ConjunctionSimplificationRuleTest.false_or_true_test (0 ms)
[----------] 4 tests from ConjunctionSimplificationRuleTest (2 ms total)

[----------] Global test environment tear-down
[==========] 4 tests from 1 test suite ran. (2 ms total)
[  PASSED  ] 2 tests.
[  FAILED  ] 2 tests, listed below:
[  FAILED  ] ConjunctionSimplificationRuleTest.true_and_false_test
[  FAILED  ] ConjunctionSimplificationRuleTest.false_or_true_test

 2 FAILED TESTS

Moreover, as expressions are modified, the change_made should be set to true.
Additional context
Add any other context about the problem here.

Contributor guide

Open the contributing guide

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. Open a pull request that references the issue number.

Research direction

Start with sql/optimizer/conjunction_simplification_rule.h and the simplification rule implementation, then review the reproduction in unittest/observer/simplification_rule_test.cpp. Verify AND and OR simplification with non-ValueExpr operands, preserve the correct boolean result, set change_made when expressions change, and run the relevant simplification-rule tests.

Written by the indexing model from the issue text.

Assessment

Tech stack
cpp
Domain
databases
Issue type
Bug
Difficulty
3/5
Estimated time
1-2 days
Activity status
Stale
Clarity
Mostly clear
Newbie friendliness
45/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.