From 1ccb3544ebf8dee63ef3613d8267b96928411585 Mon Sep 17 00:00:00 2001 From: Zihao Xu Date: Tue, 19 Mar 2024 01:10:01 -0400 Subject: [PATCH] feat(expr): add short circuit checker in `is_const` (#15758) --- .../tests/testdata/input/short_circuit.yaml | 64 +++++++++++++++++++ .../tests/testdata/output/expr.yaml | 2 +- .../tests/testdata/output/short_circuit.yaml | 46 +++++++++++++ src/frontend/src/expr/mod.rs | 30 ++++++++- 4 files changed, 139 insertions(+), 3 deletions(-) create mode 100644 src/frontend/planner_test/tests/testdata/input/short_circuit.yaml create mode 100644 src/frontend/planner_test/tests/testdata/output/short_circuit.yaml diff --git a/src/frontend/planner_test/tests/testdata/input/short_circuit.yaml b/src/frontend/planner_test/tests/testdata/input/short_circuit.yaml new file mode 100644 index 0000000000000..08b4b08471116 --- /dev/null +++ b/src/frontend/planner_test/tests/testdata/input/short_circuit.yaml @@ -0,0 +1,64 @@ +# reference: + +- id: create_table + sql: | + create table t1 (c1 INT, c2 INT, c3 INT); + expected_outputs: [] + +- id : short_circuit_or_pattern + before: + - create_table + sql: | + select true or 'abc'::int > 1; + expected_outputs: + - logical_plan + - batch_plan + +- id : short_circuit_and_pattern + before: + - create_table + sql: | + select false and 'abc'::int > 1; + expected_outputs: + - logical_plan + - batch_plan + +- id : short_circuit_or_pattern_with_table + before: + - create_table + sql: | + select true or 'abc'::int > c1 from t1; + expected_outputs: + - logical_plan + - batch_plan + +- id : short_circuit_and_pattern_with_table + before: + - create_table + sql: | + select false and 'abc'::int > c1 from t1; + expected_outputs: + - logical_plan + - batch_plan + +# should *not* be identified as const +# otherwise the *semantic* will be inconsistent +# ---- +# - id : short_circuit_or_panic_pattern +# before: +# - create_table +# sql: | +# select 'abc'::int > c1 or true from t1; +# expected_outputs: +# - logical_plan +# - batch_plan +# ---- +# - id : short_circuit_and_panic_pattern +# before: +# - create_table +# sql: | +# select 'abc'::int > c1 and false from t1; +# expected_outputs: +# - logical_plan +# - batch_plan +# ---- \ No newline at end of file diff --git a/src/frontend/planner_test/tests/testdata/output/expr.yaml b/src/frontend/planner_test/tests/testdata/output/expr.yaml index f2561fa7333c9..12d553f50b315 100644 --- a/src/frontend/planner_test/tests/testdata/output/expr.yaml +++ b/src/frontend/planner_test/tests/testdata/output/expr.yaml @@ -88,7 +88,7 @@ create table t (v1 int); SELECT 1 in (3, 0.5*2, min(v1)) from t; batch_plan: |- - BatchProject { exprs: [(true:Boolean OR (1:Int32 = min(min(t.v1)))) as $expr1] } + BatchProject { exprs: [true:Boolean] } └─BatchSimpleAgg { aggs: [min(min(t.v1))] } └─BatchExchange { order: [], dist: Single } └─BatchSimpleAgg { aggs: [min(t.v1)] } diff --git a/src/frontend/planner_test/tests/testdata/output/short_circuit.yaml b/src/frontend/planner_test/tests/testdata/output/short_circuit.yaml new file mode 100644 index 0000000000000..f870d5d09e9b6 --- /dev/null +++ b/src/frontend/planner_test/tests/testdata/output/short_circuit.yaml @@ -0,0 +1,46 @@ +# This file is automatically generated. See `src/frontend/planner_test/README.md` for more information. +- id: create_table + sql: | + create table t1 (c1 INT, c2 INT, c3 INT); +- id: short_circuit_or_pattern + before: + - create_table + sql: | + select true or 'abc'::int > 1; + logical_plan: |- + LogicalProject { exprs: [(true:Boolean OR ('abc':Varchar::Int32 > 1:Int32)) as $expr1] } + └─LogicalValues { rows: [[]], schema: Schema { fields: [] } } + batch_plan: 'BatchValues { rows: [[true:Boolean]] }' +- id: short_circuit_and_pattern + before: + - create_table + sql: | + select false and 'abc'::int > 1; + logical_plan: |- + LogicalProject { exprs: [(false:Boolean AND ('abc':Varchar::Int32 > 1:Int32)) as $expr1] } + └─LogicalValues { rows: [[]], schema: Schema { fields: [] } } + batch_plan: 'BatchValues { rows: [[false:Boolean]] }' +- id: short_circuit_or_pattern_with_table + before: + - create_table + sql: | + select true or 'abc'::int > c1 from t1; + logical_plan: |- + LogicalProject { exprs: [(true:Boolean OR ('abc':Varchar::Int32 > t1.c1)) as $expr1] } + └─LogicalScan { table: t1, columns: [t1.c1, t1.c2, t1.c3, t1._row_id] } + batch_plan: |- + BatchExchange { order: [], dist: Single } + └─BatchProject { exprs: [true:Boolean] } + └─BatchScan { table: t1, columns: [t1.c1], distribution: SomeShard } +- id: short_circuit_and_pattern_with_table + before: + - create_table + sql: | + select false and 'abc'::int > c1 from t1; + logical_plan: |- + LogicalProject { exprs: [(false:Boolean AND ('abc':Varchar::Int32 > t1.c1)) as $expr1] } + └─LogicalScan { table: t1, columns: [t1.c1, t1.c2, t1.c3, t1._row_id] } + batch_plan: |- + BatchExchange { order: [], dist: Single } + └─BatchProject { exprs: [false:Boolean] } + └─BatchScan { table: t1, columns: [t1.c1], distribution: SomeShard } diff --git a/src/frontend/src/expr/mod.rs b/src/frontend/src/expr/mod.rs index ef6405d62b7ae..893ae425b8513 100644 --- a/src/frontend/src/expr/mod.rs +++ b/src/frontend/src/expr/mod.rs @@ -17,7 +17,7 @@ use fixedbitset::FixedBitSet; use futures::FutureExt; use paste::paste; use risingwave_common::array::ListValue; -use risingwave_common::types::{DataType, Datum, JsonbVal, Scalar}; +use risingwave_common::types::{DataType, Datum, JsonbVal, Scalar, ScalarImpl}; use risingwave_expr::aggregate::AggKind; use risingwave_expr::expr::build_from_prost; use risingwave_pb::expr::expr_node::RexNode; @@ -614,6 +614,7 @@ impl ExprImpl { struct HasOthers { has_others: bool, } + impl ExprVisitor for HasOthers { fn visit_expr(&mut self, expr: &ExprImpl) { match expr { @@ -627,7 +628,14 @@ impl ExprImpl { | ExprImpl::Parameter(_) | ExprImpl::Now(_) => self.has_others = true, ExprImpl::Literal(_inner) => {} - ExprImpl::FunctionCall(inner) => self.visit_function_call(inner), + ExprImpl::FunctionCall(inner) => { + if !self.is_short_circuit(inner) { + // only if the current `func_call` is *not* a short-circuit + // expression, e.g., true or (...) | false and (...), + // shall we proceed to visit it. + self.visit_function_call(inner) + } + } ExprImpl::FunctionCallWithLambda(inner) => { self.visit_function_call_with_lambda(inner) } @@ -635,6 +643,24 @@ impl ExprImpl { } } + impl HasOthers { + fn is_short_circuit(&self, func_call: &FunctionCall) -> bool { + /// evaluate the first parameter of `Or` or `And` function call + fn eval_first(e: &ExprImpl, expect: bool) -> bool { + let Some(Ok(Some(scalar))) = e.try_fold_const() else { + return false; + }; + scalar == ScalarImpl::Bool(expect) + } + + match func_call.func_type { + ExprType::Or => eval_first(&func_call.inputs()[0], true), + ExprType::And => eval_first(&func_call.inputs()[0], false), + _ => false, + } + } + } + let mut visitor = HasOthers { has_others: false }; visitor.visit_expr(self); !visitor.has_others