From 2592809b843feeee75b101edd8548d155d9de5a4 Mon Sep 17 00:00:00 2001 From: Bugen Zhao Date: Fri, 5 May 2023 17:31:33 +0800 Subject: [PATCH 1/5] do not const eval impure funcs Signed-off-by: Bugen Zhao --- src/frontend/src/expr/mod.rs | 70 +++++++++++++------ .../plan_expr_rewriter/const_eval_rewriter.rs | 7 +- .../optimizer/plan_node/logical_over_agg.rs | 4 +- .../src/optimizer/plan_node/logical_source.rs | 8 +-- .../rule/always_false_filter_rule.rs | 12 +--- .../optimizer/rule/over_agg_to_topn_rule.rs | 2 + src/frontend/src/utils/condition.rs | 8 +-- 7 files changed, 64 insertions(+), 47 deletions(-) diff --git a/src/frontend/src/expr/mod.rs b/src/frontend/src/expr/mod.rs index 4adc10d62d0ef..6dedf656f77f1 100644 --- a/src/frontend/src/expr/mod.rs +++ b/src/frontend/src/expr/mod.rs @@ -276,12 +276,21 @@ impl ExprImpl { Ok(backend_expr.eval_row(input).await?) } - /// Evaluate a constant expression. - pub fn eval_row_const(&self) -> RwResult { - assert!(self.is_const()); - self.eval_row(&OwnedRow::empty()) - .now_or_never() - .expect("constant expression should not be async") + /// Try to evaluate an expression if it's a constant expression by `ExprImpl::is_const`. + /// + /// Returns... + /// - `None` if it's not a constant expression, + /// - `Some(Ok(_))` if constant evaluation succeeds, + /// - `Some(Err(_))` if there's an error while evaluating a constant expression. + pub fn eval_row_const(&self) -> Option> { + if self.is_const() { + self.eval_row(&OwnedRow::empty()) + .now_or_never() + .expect("constant expression should not be async") + .into() + } else { + None + } } } @@ -539,26 +548,41 @@ impl ExprImpl { } /// Checks whether this is a constant expr that can be evaluated over a dummy chunk. - /// Equivalent to `!has_input_ref && !has_agg_call && !has_subquery && - /// !has_correlated_input_ref` but checks them in one pass. + /// + /// The expression tree should only consist of literals and **pure** function calls. pub fn is_const(&self) -> bool { - struct Has { - has: bool, - } - impl ExprVisitor<()> for Has { - fn merge(_: (), _: ()) {} - - fn visit_expr(&mut self, expr: &ExprImpl) { - match expr { - ExprImpl::Literal(_inner) => {} - ExprImpl::FunctionCall(inner) => self.visit_function_call(inner), - _ => self.has = true, + let can_eval_const = { + struct Has { + has: bool, + } + impl ExprVisitor<()> for Has { + fn merge(_: (), _: ()) {} + + fn visit_expr(&mut self, expr: &ExprImpl) { + match expr { + ExprImpl::Literal(_inner) => {} + ExprImpl::FunctionCall(inner) => self.visit_function_call(inner), + ExprImpl::CorrelatedInputRef(_) + | ExprImpl::InputRef(_) + | ExprImpl::AggCall(_) + | ExprImpl::Subquery(_) + | ExprImpl::TableFunction(_) + | ExprImpl::WindowFunction(_) + | ExprImpl::UserDefinedFunction(_) + | ExprImpl::Parameter(_) + | ExprImpl::Now(_) => self.has = true, + } } } - } - let mut visitor = Has { has: false }; - visitor.visit_expr(self); - !visitor.has + + let mut visitor = Has { has: false }; + visitor.visit_expr(self); + !visitor.has + }; + + let is_pure = self.is_pure(); + + can_eval_const && is_pure } /// Returns the `InputRefs` of an Equality predicate if it matches diff --git a/src/frontend/src/optimizer/plan_expr_rewriter/const_eval_rewriter.rs b/src/frontend/src/optimizer/plan_expr_rewriter/const_eval_rewriter.rs index 4e836a3828010..5522c87a452af 100644 --- a/src/frontend/src/optimizer/plan_expr_rewriter/const_eval_rewriter.rs +++ b/src/frontend/src/optimizer/plan_expr_rewriter/const_eval_rewriter.rs @@ -24,10 +24,9 @@ impl ExprRewriter for ConstEvalRewriter { if self.error.is_some() { return expr; } - if expr.is_const() { - let data_type = expr.return_type(); - match expr.eval_row_const() { - Ok(datum) => Literal::new(datum, data_type).into(), + if let Some(result) = expr.eval_row_const() { + match result { + Ok(datum) => Literal::new(datum, expr.return_type()).into(), Err(e) => { self.error = Some(e); expr diff --git a/src/frontend/src/optimizer/plan_node/logical_over_agg.rs b/src/frontend/src/optimizer/plan_node/logical_over_agg.rs index 37e4d12e67b9d..67915f5c5ea88 100644 --- a/src/frontend/src/optimizer/plan_node/logical_over_agg.rs +++ b/src/frontend/src/optimizer/plan_node/logical_over_agg.rs @@ -148,7 +148,9 @@ impl LogicalOverAgg { } offset_expr .cast_implicit(DataType::Int64)? - .eval_row_const()? + .eval_row_const() + .transpose()? + .flatten() .map(|v| *v.as_int64() as usize) .unwrap_or(1usize) } else { diff --git a/src/frontend/src/optimizer/plan_node/logical_source.rs b/src/frontend/src/optimizer/plan_node/logical_source.rs index df658cd3a21ee..34f873fdb80cb 100644 --- a/src/frontend/src/optimizer/plan_node/logical_source.rs +++ b/src/frontend/src/optimizer/plan_node/logical_source.rs @@ -363,24 +363,24 @@ fn expr_to_kafka_timestamp_range( ExprImpl::FunctionCall(function_call) if function_call.inputs().len() == 2 => { match (&function_call.inputs()[0], &function_call.inputs()[1]) { (ExprImpl::InputRef(input_ref), literal) - if literal.is_const() + if let Some(datum) = literal.eval_row_const().transpose()? && schema.fields[input_ref.index].name == KAFKA_TIMESTAMP_COLUMN_NAME && literal.return_type() == DataType::Timestamptz => { Ok(Some(( - literal.eval_row_const()?.unwrap().into_int64() / 1000, + datum.unwrap().into_int64() / 1000, false, ))) } (literal, ExprImpl::InputRef(input_ref)) - if literal.is_const() + if let Some(datum) = literal.eval_row_const().transpose()? && schema.fields[input_ref.index].name == KAFKA_TIMESTAMP_COLUMN_NAME && literal.return_type() == DataType::Timestamptz => { Ok(Some(( - literal.eval_row_const()?.unwrap().into_int64() / 1000, + datum.unwrap().into_int64() / 1000, true, ))) } diff --git a/src/frontend/src/optimizer/rule/always_false_filter_rule.rs b/src/frontend/src/optimizer/rule/always_false_filter_rule.rs index 69316a273ec23..fd7c6fdc86422 100644 --- a/src/frontend/src/optimizer/rule/always_false_filter_rule.rs +++ b/src/frontend/src/optimizer/rule/always_false_filter_rule.rs @@ -27,17 +27,7 @@ impl Rule for AlwaysFalseFilterRule { .predicate() .conjunctions .iter() - .filter_map(|e| { - if e.is_const() { - if let Ok(v) = e.eval_row_const() { - Some(v) - } else { - None - } - } else { - None - } - }) + .filter_map(|e| e.eval_row_const().transpose().ok().flatten()) .any(|s| s.unwrap_or(ScalarImpl::Bool(true)) == ScalarImpl::Bool(false)); if always_false { Some(LogicalValues::create( diff --git a/src/frontend/src/optimizer/rule/over_agg_to_topn_rule.rs b/src/frontend/src/optimizer/rule/over_agg_to_topn_rule.rs index 423daa1ec1fc9..08abeb2e2f29a 100644 --- a/src/frontend/src/optimizer/rule/over_agg_to_topn_rule.rs +++ b/src/frontend/src/optimizer/rule/over_agg_to_topn_rule.rs @@ -124,6 +124,7 @@ fn handle_rank_preds(rank_preds: &[ExprImpl], window_func_pos: usize) -> Option< .cast_implicit(DataType::Int64) .ok()? .eval_row_const() + .unwrap() .ok()??; let v = *v.as_int64(); match cmp { @@ -139,6 +140,7 @@ fn handle_rank_preds(rank_preds: &[ExprImpl], window_func_pos: usize) -> Option< .cast_implicit(DataType::Int64) .ok()? .eval_row_const() + .unwrap() .ok()??; let v = *v.as_int64(); if let Some(eq) = eq && eq != v { diff --git a/src/frontend/src/utils/condition.rs b/src/frontend/src/utils/condition.rs index 8134575fc0b54..7f0de72c97003 100644 --- a/src/frontend/src/utils/condition.rs +++ b/src/frontend/src/utils/condition.rs @@ -456,7 +456,7 @@ impl Condition { } }; - let Some(new_cond) = new_expr.eval_row_const()? else { + let Some(new_cond) = new_expr.eval_row_const().unwrap()? else { // column = NULL, the result is always NULL. return Ok(false_cond()); }; @@ -479,7 +479,7 @@ impl Condition { let const_expr = const_expr .cast_implicit(input_ref.data_type.clone()) .unwrap(); - let value = const_expr.eval_row_const()?; + let value = const_expr.eval_row_const().unwrap()?; let Some(value) = value else { continue; }; @@ -537,7 +537,7 @@ impl Condition { } } }; - let Some(value) = new_expr.eval_row_const()? else { + let Some(value) = new_expr.eval_row_const().unwrap()? else { // column compare with NULL, the result is always NULL. return Ok(false_cond()); }; @@ -849,7 +849,7 @@ mod cast_compare { } _ => unreachable!(), }; - match const_expr.eval_row_const().map_err(|_| ())? { + match const_expr.eval_row_const().unwrap().map_err(|_| ())? { Some(scalar) => { let value = scalar.as_integral(); if value > upper_bound { From 0b2e6e916d539d016dc17f932b0814e989c6ace8 Mon Sep 17 00:00:00 2001 From: Bugen Zhao Date: Fri, 5 May 2023 17:34:34 +0800 Subject: [PATCH 2/5] remove pi Signed-off-by: Bugen Zhao --- proto/expr.proto | 3 --- src/frontend/src/expr/pure.rs | 1 - 2 files changed, 4 deletions(-) diff --git a/proto/expr.proto b/proto/expr.proto index f55f2b947b45a..b2ec87f93d142 100644 --- a/proto/expr.proto +++ b/proto/expr.proto @@ -168,9 +168,6 @@ message ExprNode { JSONB_TYPEOF = 602; JSONB_ARRAY_LENGTH = 603; - // Functions that return a constant value - PI = 610; - // Non-pure functions below (> 1000) // ------------------------ // Internal functions diff --git a/src/frontend/src/expr/pure.rs b/src/frontend/src/expr/pure.rs index 7f91273ec700d..1d0b9261e3139 100644 --- a/src/frontend/src/expr/pure.rs +++ b/src/frontend/src/expr/pure.rs @@ -151,7 +151,6 @@ impl ExprVisitor for ImpureAnalyzer { | expr_node::Type::JsonbAccessStr | expr_node::Type::JsonbTypeof | expr_node::Type::JsonbArrayLength - | expr_node::Type::Pi | expr_node::Type::Sind | expr_node::Type::Cosd | expr_node::Type::Cotd From 8b39f0300740406e1433bb85bafc32be9e0fb323 Mon Sep 17 00:00:00 2001 From: Bugen Zhao Date: Fri, 5 May 2023 18:11:19 +0800 Subject: [PATCH 3/5] add e2e test Signed-off-by: Bugen Zhao --- e2e_test/ddl/table/generated_columns.slt.part | 39 ++++++++++++++++++- 1 file changed, 38 insertions(+), 1 deletion(-) diff --git a/e2e_test/ddl/table/generated_columns.slt.part b/e2e_test/ddl/table/generated_columns.slt.part index 94074549871e9..b9932886551b0 100644 --- a/e2e_test/ddl/table/generated_columns.slt.part +++ b/e2e_test/ddl/table/generated_columns.slt.part @@ -40,7 +40,7 @@ drop table t2; statement error create table t2 (v1 int as v2+1, v2 int, v3 int as v1-1); -# Create a table with proctime. +# Test table with proctime. statement ok create table t3 (v1 int, v2 Timestamptz as proctime()); @@ -76,3 +76,40 @@ t statement ok drop table t3; + +# Test materialized view on source with proctime. +statement ok +create source t4 ( + v int, + t timestamptz as proctime() +) with ( + connector = 'datagen', + fields.v.kind = 'sequence', + fields.v.start = '1', + fields.v.end = '5', + datagen.rows.per.second='10000', + datagen.split.num = '1' +) row format json; + +statement ok +CREATE MATERIALIZED VIEW mv AS SELECT * FROM t4; + +sleep 2s + +statement ok +flush; + +query TT +select v, t >= date '2021-01-01' as later_than_2021 from mv; +---- +1 t +2 t +3 t +4 t +5 t + +statement ok +drop materialized view mv; + +statement ok +drop source t4; From 29dad2a96ab2c0b52e652f7b5f90add4b27c59bd Mon Sep 17 00:00:00 2001 From: Bugen Zhao Date: Fri, 5 May 2023 19:21:16 +0800 Subject: [PATCH 4/5] better naming Signed-off-by: Bugen Zhao --- src/frontend/src/expr/mod.rs | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/src/frontend/src/expr/mod.rs b/src/frontend/src/expr/mod.rs index 6dedf656f77f1..e7b65f992ea10 100644 --- a/src/frontend/src/expr/mod.rs +++ b/src/frontend/src/expr/mod.rs @@ -551,11 +551,11 @@ impl ExprImpl { /// /// The expression tree should only consist of literals and **pure** function calls. pub fn is_const(&self) -> bool { - let can_eval_const = { - struct Has { - has: bool, + let only_literal_and_func = { + struct HasOthers { + has_others: bool, } - impl ExprVisitor<()> for Has { + impl ExprVisitor<()> for HasOthers { fn merge(_: (), _: ()) {} fn visit_expr(&mut self, expr: &ExprImpl) { @@ -570,19 +570,19 @@ impl ExprImpl { | ExprImpl::WindowFunction(_) | ExprImpl::UserDefinedFunction(_) | ExprImpl::Parameter(_) - | ExprImpl::Now(_) => self.has = true, + | ExprImpl::Now(_) => self.has_others = true, } } } - let mut visitor = Has { has: false }; + let mut visitor = HasOthers { has_others: false }; visitor.visit_expr(self); - !visitor.has + !visitor.has_others }; let is_pure = self.is_pure(); - can_eval_const && is_pure + only_literal_and_func && is_pure } /// Returns the `InputRefs` of an Equality predicate if it matches From 1b3b74643f0b9996ca577abda827e80b2061aaf4 Mon Sep 17 00:00:00 2001 From: Bugen Zhao Date: Sat, 6 May 2023 13:58:44 +0800 Subject: [PATCH 5/5] rename to try_fold_const Signed-off-by: Bugen Zhao --- src/frontend/src/expr/mod.rs | 7 ++++++- .../plan_expr_rewriter/const_eval_rewriter.rs | 2 +- .../src/optimizer/plan_node/logical_over_agg.rs | 2 +- .../src/optimizer/plan_node/logical_source.rs | 4 ++-- .../src/optimizer/rule/always_false_filter_rule.rs | 2 +- .../src/optimizer/rule/over_agg_to_topn_rule.rs | 14 ++------------ src/frontend/src/utils/condition.rs | 8 ++++---- 7 files changed, 17 insertions(+), 22 deletions(-) diff --git a/src/frontend/src/expr/mod.rs b/src/frontend/src/expr/mod.rs index e7b65f992ea10..971bda5749973 100644 --- a/src/frontend/src/expr/mod.rs +++ b/src/frontend/src/expr/mod.rs @@ -282,7 +282,7 @@ impl ExprImpl { /// - `None` if it's not a constant expression, /// - `Some(Ok(_))` if constant evaluation succeeds, /// - `Some(Err(_))` if there's an error while evaluating a constant expression. - pub fn eval_row_const(&self) -> Option> { + pub fn try_fold_const(&self) -> Option> { if self.is_const() { self.eval_row(&OwnedRow::empty()) .now_or_never() @@ -292,6 +292,11 @@ impl ExprImpl { None } } + + /// Similar to `ExprImpl::try_fold_const`, but panics if the expression is not constant. + pub fn fold_const(&self) -> RwResult { + self.try_fold_const().expect("expression is not constant") + } } /// Implement helper functions which recursively checks whether an variant is included in the diff --git a/src/frontend/src/optimizer/plan_expr_rewriter/const_eval_rewriter.rs b/src/frontend/src/optimizer/plan_expr_rewriter/const_eval_rewriter.rs index 5522c87a452af..7a40bb3ee7307 100644 --- a/src/frontend/src/optimizer/plan_expr_rewriter/const_eval_rewriter.rs +++ b/src/frontend/src/optimizer/plan_expr_rewriter/const_eval_rewriter.rs @@ -24,7 +24,7 @@ impl ExprRewriter for ConstEvalRewriter { if self.error.is_some() { return expr; } - if let Some(result) = expr.eval_row_const() { + if let Some(result) = expr.try_fold_const() { match result { Ok(datum) => Literal::new(datum, expr.return_type()).into(), Err(e) => { diff --git a/src/frontend/src/optimizer/plan_node/logical_over_agg.rs b/src/frontend/src/optimizer/plan_node/logical_over_agg.rs index 67915f5c5ea88..7f672c6858fef 100644 --- a/src/frontend/src/optimizer/plan_node/logical_over_agg.rs +++ b/src/frontend/src/optimizer/plan_node/logical_over_agg.rs @@ -148,7 +148,7 @@ impl LogicalOverAgg { } offset_expr .cast_implicit(DataType::Int64)? - .eval_row_const() + .try_fold_const() .transpose()? .flatten() .map(|v| *v.as_int64() as usize) diff --git a/src/frontend/src/optimizer/plan_node/logical_source.rs b/src/frontend/src/optimizer/plan_node/logical_source.rs index 34f873fdb80cb..a91772e7e582f 100644 --- a/src/frontend/src/optimizer/plan_node/logical_source.rs +++ b/src/frontend/src/optimizer/plan_node/logical_source.rs @@ -363,7 +363,7 @@ fn expr_to_kafka_timestamp_range( ExprImpl::FunctionCall(function_call) if function_call.inputs().len() == 2 => { match (&function_call.inputs()[0], &function_call.inputs()[1]) { (ExprImpl::InputRef(input_ref), literal) - if let Some(datum) = literal.eval_row_const().transpose()? + if let Some(datum) = literal.try_fold_const().transpose()? && schema.fields[input_ref.index].name == KAFKA_TIMESTAMP_COLUMN_NAME && literal.return_type() == DataType::Timestamptz => @@ -374,7 +374,7 @@ fn expr_to_kafka_timestamp_range( ))) } (literal, ExprImpl::InputRef(input_ref)) - if let Some(datum) = literal.eval_row_const().transpose()? + if let Some(datum) = literal.try_fold_const().transpose()? && schema.fields[input_ref.index].name == KAFKA_TIMESTAMP_COLUMN_NAME && literal.return_type() == DataType::Timestamptz => diff --git a/src/frontend/src/optimizer/rule/always_false_filter_rule.rs b/src/frontend/src/optimizer/rule/always_false_filter_rule.rs index fd7c6fdc86422..02165232372e4 100644 --- a/src/frontend/src/optimizer/rule/always_false_filter_rule.rs +++ b/src/frontend/src/optimizer/rule/always_false_filter_rule.rs @@ -27,7 +27,7 @@ impl Rule for AlwaysFalseFilterRule { .predicate() .conjunctions .iter() - .filter_map(|e| e.eval_row_const().transpose().ok().flatten()) + .filter_map(|e| e.try_fold_const().transpose().ok().flatten()) .any(|s| s.unwrap_or(ScalarImpl::Bool(true)) == ScalarImpl::Bool(false)); if always_false { Some(LogicalValues::create( diff --git a/src/frontend/src/optimizer/rule/over_agg_to_topn_rule.rs b/src/frontend/src/optimizer/rule/over_agg_to_topn_rule.rs index 08abeb2e2f29a..21c84bab909e0 100644 --- a/src/frontend/src/optimizer/rule/over_agg_to_topn_rule.rs +++ b/src/frontend/src/optimizer/rule/over_agg_to_topn_rule.rs @@ -120,12 +120,7 @@ fn handle_rank_preds(rank_preds: &[ExprImpl], window_func_pos: usize) -> Option< for cond in rank_preds { if let Some((input_ref, cmp, v)) = cond.as_comparison_const() { assert_eq!(input_ref.index, window_func_pos); - let v = v - .cast_implicit(DataType::Int64) - .ok()? - .eval_row_const() - .unwrap() - .ok()??; + let v = v.cast_implicit(DataType::Int64).ok()?.fold_const().ok()??; let v = *v.as_int64(); match cmp { ExprType::LessThanOrEqual => ub = ub.map_or(Some(v), |ub| Some(ub.min(v))), @@ -136,12 +131,7 @@ fn handle_rank_preds(rank_preds: &[ExprImpl], window_func_pos: usize) -> Option< } } else if let Some((input_ref, v)) = cond.as_eq_const() { assert_eq!(input_ref.index, window_func_pos); - let v = v - .cast_implicit(DataType::Int64) - .ok()? - .eval_row_const() - .unwrap() - .ok()??; + let v = v.cast_implicit(DataType::Int64).ok()?.fold_const().ok()??; let v = *v.as_int64(); if let Some(eq) = eq && eq != v { tracing::error!( diff --git a/src/frontend/src/utils/condition.rs b/src/frontend/src/utils/condition.rs index 7f0de72c97003..dc844cc9d6746 100644 --- a/src/frontend/src/utils/condition.rs +++ b/src/frontend/src/utils/condition.rs @@ -456,7 +456,7 @@ impl Condition { } }; - let Some(new_cond) = new_expr.eval_row_const().unwrap()? else { + let Some(new_cond) = new_expr.fold_const()? else { // column = NULL, the result is always NULL. return Ok(false_cond()); }; @@ -479,7 +479,7 @@ impl Condition { let const_expr = const_expr .cast_implicit(input_ref.data_type.clone()) .unwrap(); - let value = const_expr.eval_row_const().unwrap()?; + let value = const_expr.fold_const()?; let Some(value) = value else { continue; }; @@ -537,7 +537,7 @@ impl Condition { } } }; - let Some(value) = new_expr.eval_row_const().unwrap()? else { + let Some(value) = new_expr.fold_const()? else { // column compare with NULL, the result is always NULL. return Ok(false_cond()); }; @@ -849,7 +849,7 @@ mod cast_compare { } _ => unreachable!(), }; - match const_expr.eval_row_const().unwrap().map_err(|_| ())? { + match const_expr.fold_const().map_err(|_| ())? { Some(scalar) => { let value = scalar.as_integral(); if value > upper_bound {