From 9fcf53a3a1e979fdc666fe86c59b30251d1fedab Mon Sep 17 00:00:00 2001 From: Sayan Pal Date: Mon, 12 Oct 2026 02:09:08 +0530 Subject: [PATCH] fix: return the constant instead of the first child when simplifying conjunctions ConjunctionSimplificationRule replaced `expr AND false` and `expr OR true` with the first child of the conjunction instead of the constant, so it was only correct when the constant happened to be the first child. For example, `select * from t where 1 = 2 and id = 1` returned the row with id = 1. - replace the conjunction with the boolean constant it short-circuits to - replace a conjunction whose children were all removed with `true` (AND) or `false` (OR); an empty conjunction always evaluated to true - set change_made whenever the expression is modified - continue from the iterator returned by vector::erase Fixes #637 --- .../conjunction_simplification_rule.cpp | 24 +-- .../conjunction_simplification_rule_test.cpp | 149 ++++++++++++++++++ 2 files changed, 164 insertions(+), 9 deletions(-) create mode 100644 unittest/observer/conjunction_simplification_rule_test.cpp diff --git a/src/observer/sql/optimizer/conjunction_simplification_rule.cpp b/src/observer/sql/optimizer/conjunction_simplification_rule.cpp index 5ba4e4148..78af759c7 100644 --- a/src/observer/sql/optimizer/conjunction_simplification_rule.cpp +++ b/src/observer/sql/optimizer/conjunction_simplification_rule.cpp @@ -51,28 +51,34 @@ RC ConjunctionSimplificationRule::rewrite(unique_ptr &expr, bool &ch if (conjunction_expr->conjunction_type() == ConjunctionExpr::Type::AND) { if (constant_value == true) { - child_exprs.erase(iter); + iter = child_exprs.erase(iter); + change_made = true; } else { // always be false - unique_ptr child_expr = std::move(child_exprs.front()); - child_exprs.clear(); - expr = std::move(child_expr); + expr = make_unique(Value(false)); + change_made = true; return rc; } } else { // conjunction_type == OR if (constant_value == true) { // always be true - unique_ptr child_expr = std::move(child_exprs.front()); - child_exprs.clear(); - expr = std::move(child_expr); + expr = make_unique(Value(true)); + change_made = true; return rc; } else { - child_exprs.erase(iter); + iter = child_exprs.erase(iter); + change_made = true; } } } - if (child_exprs.size() == 1) { + + if (child_exprs.empty()) { + // every child was removed, e.g. `true AND true` or `false OR false` + bool value = (conjunction_expr->conjunction_type() == ConjunctionExpr::Type::AND); + expr = make_unique(Value(value)); + change_made = true; + } else if (child_exprs.size() == 1) { LOG_TRACE("conjunction expression has only 1 child"); unique_ptr child_expr = std::move(child_exprs.front()); child_exprs.clear(); diff --git a/unittest/observer/conjunction_simplification_rule_test.cpp b/unittest/observer/conjunction_simplification_rule_test.cpp new file mode 100644 index 000000000..2cba79caa --- /dev/null +++ b/unittest/observer/conjunction_simplification_rule_test.cpp @@ -0,0 +1,149 @@ +/* Copyright (c) 2021 OceanBase and/or its affiliates. All rights reserved. +miniob is licensed under Mulan PSL v2. +You can use this software according to the terms and conditions of the Mulan PSL v2. +You may obtain a copy of Mulan PSL v2 at: + http://license.coscl.org.cn/MulanPSL2 +THIS SOFTWARE IS PROVIDED ON AN "AS IS" BASIS, WITHOUT WARRANTIES OF ANY KIND, +EITHER EXPRESS OR IMPLIED, INCLUDING BUT NOT LIMITED TO NON-INFRINGEMENT, +MERCHANTABILITY OR FIT FOR A PARTICULAR PURPOSE. +See the Mulan PSL v2 for more details. */ + +// +// Created by Sayan Pal on 2026/10/12. +// + +#include + +#include "sql/expr/expression.h" +#include "sql/optimizer/conjunction_simplification_rule.h" +#include "gtest/gtest.h" + +using namespace std; +using namespace common; + +// The rule does not fold comparisons, so this is not a constant to it, but it still evaluates to `value` +static unique_ptr non_constant_expr(bool value) +{ + CompOp comp = value ? CompOp::EQUAL_TO : CompOp::NOT_EQUAL; + return make_unique(comp, make_unique(Value(1)), make_unique(Value(1))); +} + +static unique_ptr bool_constant_expr(bool value) { return make_unique(Value(value)); } + +template +static unique_ptr make_conjunction(ConjunctionExpr::Type type, Children... children) +{ + vector> child_exprs; + (child_exprs.push_back(std::move(children)), ...); + return make_unique(type, child_exprs); +} + +static bool simplify(unique_ptr &expr) +{ + ConjunctionSimplificationRule rule; + bool change_made = false; + EXPECT_EQ(RC::SUCCESS, rule.rewrite(expr, change_made)); + return change_made; +} + +static void expect_bool_constant(const unique_ptr &expr, bool expected) +{ + ASSERT_EQ(ExprType::VALUE, expr->type()); + EXPECT_EQ(expected, static_cast(expr.get())->get_value().get_boolean()); +} + +TEST(ConjunctionSimplificationRule, and_with_false_is_false) +{ + // (1 = 1) AND false => false + unique_ptr expr = + make_conjunction(ConjunctionExpr::Type::AND, non_constant_expr(true), bool_constant_expr(false)); + EXPECT_TRUE(simplify(expr)); + expect_bool_constant(expr, false); +} + +TEST(ConjunctionSimplificationRule, or_with_true_is_true) +{ + // (1 <> 1) OR true => true + unique_ptr expr = + make_conjunction(ConjunctionExpr::Type::OR, non_constant_expr(false), bool_constant_expr(true)); + EXPECT_TRUE(simplify(expr)); + expect_bool_constant(expr, true); +} + +TEST(ConjunctionSimplificationRule, all_children_removed) +{ + // true AND true => true + unique_ptr and_expr = + make_conjunction(ConjunctionExpr::Type::AND, bool_constant_expr(true), bool_constant_expr(true)); + EXPECT_TRUE(simplify(and_expr)); + expect_bool_constant(and_expr, true); + + // false OR false => false + unique_ptr or_expr = + make_conjunction(ConjunctionExpr::Type::OR, bool_constant_expr(false), bool_constant_expr(false)); + EXPECT_TRUE(simplify(or_expr)); + expect_bool_constant(or_expr, false); +} + +TEST(ConjunctionSimplificationRule, identity_constant_is_removed) +{ + // (1 = 1) AND true AND (1 <> 1) => (1 = 1) AND (1 <> 1) + unique_ptr left = non_constant_expr(true); + unique_ptr right = non_constant_expr(false); + Expression *left_ptr = left.get(); + Expression *right_ptr = right.get(); + unique_ptr expr = + make_conjunction(ConjunctionExpr::Type::AND, std::move(left), bool_constant_expr(true), std::move(right)); + EXPECT_TRUE(simplify(expr)); + ASSERT_EQ(ExprType::CONJUNCTION, expr->type()); + vector> *children = &static_cast(expr.get())->children(); + ASSERT_EQ(2, children->size()); + EXPECT_EQ(left_ptr, (*children)[0].get()); + EXPECT_EQ(right_ptr, (*children)[1].get()); + + // (1 = 1) OR false OR (1 <> 1) => (1 = 1) OR (1 <> 1) + left = non_constant_expr(true); + right = non_constant_expr(false); + left_ptr = left.get(); + right_ptr = right.get(); + expr = make_conjunction(ConjunctionExpr::Type::OR, std::move(left), bool_constant_expr(false), std::move(right)); + EXPECT_TRUE(simplify(expr)); + ASSERT_EQ(ExprType::CONJUNCTION, expr->type()); + children = &static_cast(expr.get())->children(); + ASSERT_EQ(2, children->size()); + EXPECT_EQ(left_ptr, (*children)[0].get()); + EXPECT_EQ(right_ptr, (*children)[1].get()); +} + +TEST(ConjunctionSimplificationRule, single_child_left_replaces_conjunction) +{ + // (1 <> 1) AND true => (1 <> 1) + unique_ptr child = non_constant_expr(false); + Expression *child_ptr = child.get(); + unique_ptr expr = + make_conjunction(ConjunctionExpr::Type::AND, std::move(child), bool_constant_expr(true)); + EXPECT_TRUE(simplify(expr)); + EXPECT_EQ(child_ptr, expr.get()); +} + +TEST(ConjunctionSimplificationRule, no_constant_no_change) +{ + // (1 = 1) AND (1 <> 1) has nothing to simplify + unique_ptr expr = + make_conjunction(ConjunctionExpr::Type::AND, non_constant_expr(true), non_constant_expr(false)); + Expression *origin = expr.get(); + EXPECT_FALSE(simplify(expr)); + EXPECT_EQ(origin, expr.get()); + EXPECT_EQ(2, static_cast(expr.get())->children().size()); +} + +int main(int argc, char **argv) +{ + + // 分析gtest程序的命令行参数 + testing::InitGoogleTest(&argc, argv); + + // 调用RUN_ALL_TESTS()运行所有测试用例 + // main函数返回RUN_ALL_TESTS()的运行结果 + return RUN_ALL_TESTS(); +}