From 45c29bfeff3e50c2299f53168abeaead58a993aa Mon Sep 17 00:00:00 2001
From: Denis Smirnov <sd@picodata.io>
Date: Tue, 26 Jul 2022 12:16:42 +0700
Subject: [PATCH] fix: merge tuple produced invalid compare expression
The error was produced by the assumption that (2, 1) > (1, 2) is
equivalent to (2 > 1) and (1 > 2). That assumption was incorrect,
so we don't merge AND chains for any operators other than Eq and
NotEq.
---
src/executor/tests.rs | 2 +-
src/ir/transformation/merge_tuples.rs | 119 +++++++++-----------
src/ir/transformation/merge_tuples/tests.rs | 13 ++-
test_app/test/integration/api_test.lua | 15 +++
4 files changed, 77 insertions(+), 72 deletions(-)
diff --git a/src/executor/tests.rs b/src/executor/tests.rs
index 5535322979..1da3e01aba 100644
--- a/src/executor/tests.rs
+++ b/src/executor/tests.rs
@@ -349,7 +349,7 @@ WHERE "t3"."id" = 2 AND "t8"."identification_number" = 2"#;
r#"FROM ("#,
r#"SELECT "test_space"."id" as "id", "test_space"."FIRST_NAME" as "FIRST_NAME""#,
r#"FROM "test_space""#,
- r#"WHERE (?) > ("test_space"."sys_op") and ("test_space"."sysFrom") >= (?)"#,
+ r#"WHERE ("test_space"."sysFrom") >= (?) and ("test_space"."sys_op") < (?)"#,
r#"UNION ALL"#,
r#"SELECT "test_space_hist"."id" as "id", "test_space_hist"."FIRST_NAME" as "FIRST_NAME""#,
r#"FROM "test_space_hist""#,
diff --git a/src/ir/transformation/merge_tuples.rs b/src/ir/transformation/merge_tuples.rs
index 5249b9f210..89c5154d09 100644
--- a/src/ir/transformation/merge_tuples.rs
+++ b/src/ir/transformation/merge_tuples.rs
@@ -1,4 +1,4 @@
-//! Merge tuples in a disjunction of boolean expressions
+//! Merge tuples in a disjunction of equality expressions
//! into a single tuple.
//!
//! Example:
@@ -36,10 +36,10 @@ fn call_as_plan(chain: &Chain, plan: &mut Plan) -> Result<usize, QueryPlannerErr
/// "AND" chain grouped by the operator type.
#[derive(Debug)]
pub struct Chain {
- // Left and right sides of the boolean expression
+ // Left and right sides of the equality (and non-equality) expressions
// grouped by the operator.
grouped: HashMap<Bool, (Vec<usize>, Vec<usize>)>,
- // Non-boolean expressions in the AND chain (true, false, null).
+ // Other boolean expressions in the AND chain (true, false, null, Lt, GtEq, etc).
other: Vec<usize>,
}
@@ -62,67 +62,65 @@ impl Chain {
pub fn insert(&mut self, plan: &mut Plan, expr_id: usize) -> Result<(), QueryPlannerError> {
let bool_expr = plan.get_expression_node(expr_id)?;
if let Expression::Bool { left, op, right } = bool_expr {
- let (left_id, right_id, group_op) = match *op {
- Bool::And | Bool::Or => {
- // We expect only AND/OR expressions.
- return Err(QueryPlannerError::CustomError(format!(
- "AND/OR expressions are not supported: {:?}",
- bool_expr
- )));
- }
- Bool::Eq | Bool::NotEq => {
- // Try to put expressions with references to the left side.
+ if let Bool::And | Bool::Or = op {
+ // We don't expect nested AND/OR expressions in DNF.
+ return Err(QueryPlannerError::CustomError(format!(
+ "AND/OR expressions are not supported: {:?}",
+ bool_expr
+ )));
+ }
+
+ // Merge expression into tuples only for equality (and non-equality) operators.
+ if let Bool::Eq | Bool::NotEq = op {
+ // Try to put expressions with references to the left side.
+ let (left_id, right_id, group_op) =
match (plan.is_ref(*left)?, plan.is_ref(*right)?) {
(false, true) => (*right, *left, op.clone()),
_ => (*left, *right, op.clone()),
+ };
+
+ // If boolean expression contains a reference to an additional
+ // sub-query, it should be added to the "other" list.
+ let left_sq = plan.get_sub_query_from_row_node(left_id)?;
+ let right_sq = plan.get_sub_query_from_row_node(right_id)?;
+ for sq_id in [left_sq, right_sq].iter().flatten() {
+ if plan.is_additional_child(*sq_id)? {
+ self.other.push(expr_id);
+ return Ok(());
}
}
- Bool::Gt | Bool::GtEq | Bool::In => (*left, *right, op.clone()),
- // Invert operator to unite expressions with Gt and GtEq.
- Bool::Lt => (*right, *left, Bool::Gt),
- Bool::LtEq => (*right, *left, Bool::GtEq),
- };
- // If boolean expression contains a reference to an additional
- // sub-query, it should be added to the "other" list.
- let left_sq = plan.get_sub_query_from_row_node(left_id)?;
- let right_sq = plan.get_sub_query_from_row_node(right_id)?;
- for sq_id in [left_sq, right_sq].iter().flatten() {
- if plan.is_additional_child(*sq_id)? {
- self.other.push(expr_id);
- return Ok(());
- }
- }
-
- match self.grouped.entry(group_op) {
- Entry::Occupied(mut entry) => {
- let (left, right) = entry.get_mut();
- let new_left_id = plan.expr_clone(left_id)?;
- let new_right_id = plan.expr_clone(right_id)?;
- plan.get_columns_or_self(new_left_id)?
- .iter()
- .for_each(|id| {
- left.push(*id);
- });
- plan.get_columns_or_self(new_right_id)?
- .iter()
- .for_each(|id| {
- right.push(*id);
- });
- }
- Entry::Vacant(entry) => {
- let new_left_id = plan.expr_clone(left_id)?;
- let new_right_id = plan.expr_clone(right_id)?;
- entry.insert((
- plan.get_columns_or_self(new_left_id)?,
- plan.get_columns_or_self(new_right_id)?,
- ));
+ match self.grouped.entry(group_op) {
+ Entry::Occupied(mut entry) => {
+ let (left, right) = entry.get_mut();
+ let new_left_id = plan.expr_clone(left_id)?;
+ let new_right_id = plan.expr_clone(right_id)?;
+ plan.get_columns_or_self(new_left_id)?
+ .iter()
+ .for_each(|id| {
+ left.push(*id);
+ });
+ plan.get_columns_or_self(new_right_id)?
+ .iter()
+ .for_each(|id| {
+ right.push(*id);
+ });
+ }
+ Entry::Vacant(entry) => {
+ let new_left_id = plan.expr_clone(left_id)?;
+ let new_right_id = plan.expr_clone(right_id)?;
+ entry.insert((
+ plan.get_columns_or_self(new_left_id)?,
+ plan.get_columns_or_self(new_right_id)?,
+ ));
+ }
}
+ return Ok(());
}
- } else {
- let new_expr_id = plan.expr_clone(expr_id)?;
- self.other.push(new_expr_id);
}
+
+ let new_expr_id = plan.expr_clone(expr_id)?;
+ self.other.push(new_expr_id);
Ok(())
}
@@ -142,16 +140,7 @@ impl Chain {
// To make serialization non-flaky, we extract operators
// in a deterministic order.
let mut grouped_top_id: Option<usize> = None;
- // No need for "And" and "Or" operators.
- let ordered_ops = &[
- Bool::Eq,
- Bool::Gt,
- Bool::GtEq,
- Bool::In,
- Bool::Lt,
- Bool::LtEq,
- Bool::NotEq,
- ];
+ let ordered_ops = &[Bool::Eq, Bool::NotEq];
for op in ordered_ops {
if let Some((left, right)) = self.grouped.get(op) {
let left_row_id = plan.nodes.add_row(left.clone(), None);
diff --git a/src/ir/transformation/merge_tuples/tests.rs b/src/ir/transformation/merge_tuples/tests.rs
index 479ea130fa..18559b5381 100644
--- a/src/ir/transformation/merge_tuples/tests.rs
+++ b/src/ir/transformation/merge_tuples/tests.rs
@@ -15,13 +15,13 @@ fn merge_tuples1() {
format!(
"{} {}",
r#"SELECT "t"."a" as "a" FROM "t""#,
- r#"WHERE ("t"."a", "t"."b") = (?, ?) and (?, "t"."a") > ("t"."c", ?)"#,
+ r#"WHERE ("t"."a", "t"."b") = (?, ?) and (?) < ("t"."a") and ("t"."c") < (?)"#,
),
vec![
Value::from(1_u64),
Value::from(2_u64),
- Value::from(3_u64),
Value::from(4_u64),
+ Value::from(3_u64),
],
);
@@ -38,14 +38,14 @@ fn merge_tuples2() {
"{} {} {}",
r#"SELECT "t"."a" as "a" FROM "t""#,
r#"WHERE (("t"."a", "t"."b") = (?, ?) and (?)"#,
- r#"or ("t"."c", "t"."a") >= (?, ?) and (?))"#,
+ r#"or (?) <= ("t"."a") and ("t"."c") >= (?) and (?))"#,
),
vec![
Value::from(1_u64),
Value::from(2_u64),
Value::Null,
- Value::from(3_u64),
Value::from(4_u64),
+ Value::from(3_u64),
Value::Boolean(true),
],
);
@@ -84,9 +84,10 @@ fn merge_tuples5() {
let expected = PatternWithParams::new(
format!(
"{} {}",
- r#"SELECT "t"."a" as "a" FROM "t""#, r#"WHERE ("t"."c", "t"."a", "t"."b") > (?, ?, ?)"#,
+ r#"SELECT "t"."a" as "a" FROM "t""#,
+ r#"WHERE ("t"."a", "t"."b") > (?, ?) and (?) < ("t"."c")"#,
),
- vec![Value::from(3_u64), Value::from(1_u64), Value::from(2_u64)],
+ vec![Value::from(1_u64), Value::from(2_u64), Value::from(3_u64)],
);
assert_eq!(sql_to_sql(input, &[], &merge_tuples), expected);
diff --git a/test_app/test/integration/api_test.lua b/test_app/test/integration/api_test.lua
index 4193ac1e67..b339549e85 100644
--- a/test_app/test/integration/api_test.lua
+++ b/test_app/test/integration/api_test.lua
@@ -558,6 +558,21 @@ g.test_decimal_double = function()
})
end
+g.test_compare = function()
+ local api = cluster:server("api-1").net_box
+
+ local r, err = api:call("query", { [[SELECT * FROM "t" where "id" < 2 and "a" > 5]], {} })
+
+ t.assert_equals(err, nil)
+ t.assert_equals(r, {
+ metadata = {
+ {name = "id", type = "integer"},
+ {name = "a", type = "number"},
+ },
+ rows = {},
+ })
+end
+
g.test_invalid_explain = function()
local api = cluster:server("api-1").net_box
--
GitLab