peter-toth commented on code in PR #10835:
URL: https://github.com/apache/datafusion/pull/10835#discussion_r1644664005
##########
datafusion/expr/src/logical_plan/plan.rs:
##########
@@ -2230,6 +2200,40 @@ impl Filter {
}
false
}
+
+ /// Remove aliases from a predicate for use in a `Filter`
+ ///
+ /// filter predicates should not contain aliased expressions so we remove
+ /// any aliases.
+ ///
+ /// before this logic was added we would have aliases within filters such
as
+ /// for benchmark q6:
+ ///
+ /// ```sql
+ /// lineitem.l_shipdate >= Date32(\"8766\")
+ /// AND lineitem.l_shipdate < Date32(\"9131\")
+ /// AND CAST(lineitem.l_discount AS Decimal128(30, 15)) AS
lineitem.l_discount >=
+ /// Decimal128(Some(49999999999999),30,15)
+ /// AND CAST(lineitem.l_discount AS Decimal128(30, 15)) AS
lineitem.l_discount <=
+ /// Decimal128(Some(69999999999999),30,15)
+ /// AND lineitem.l_quantity < Decimal128(Some(2400),15,2)
+ /// ```
+ pub fn remove_aliases(predicate: Expr) -> Result<Transformed<Expr>> {
Review Comment:
I wonder if it make sense to move this method into `Expr`?
##########
datafusion/optimizer/src/common_subexpr_eliminate.rs:
##########
@@ -161,129 +174,209 @@ impl CommonSubexprEliminate {
/// common sub-expressions that were used
fn rewrite_expr(
&self,
- exprs_list: &[&[Expr]],
+ exprs_list: Vec<Vec<Expr>>,
arrays_list: &[&[IdArray]],
- input: &LogicalPlan,
+ input: LogicalPlan,
expr_stats: &ExprStats,
config: &dyn OptimizerConfig,
- ) -> Result<(Vec<Vec<Expr>>, LogicalPlan)> {
+ ) -> Result<Transformed<(Vec<Vec<Expr>>, LogicalPlan)>> {
+ let mut transformed = false;
let mut common_exprs = CommonExprs::new();
+
let rewrite_exprs = self.rewrite_exprs_list(
exprs_list,
arrays_list,
expr_stats,
&mut common_exprs,
&config.alias_generator(),
)?;
+ transformed |= rewrite_exprs.transformed;
- let mut new_input = self
- .try_optimize(input, config)?
- .unwrap_or_else(|| input.clone());
+ let new_input = self.rewrite(input, config)?;
+ transformed |= new_input.transformed;
+ let mut new_input = new_input.data;
if !common_exprs.is_empty() {
new_input = build_common_expr_project_plan(new_input,
common_exprs)?;
+ transformed = true;
Review Comment:
I'm not sure we need this because if `!common_exprs.is_empty()` is true then
`rewrite_exprs.transformed` must also be true a bit above.
##########
datafusion/expr/src/logical_plan/plan.rs:
##########
@@ -2230,6 +2200,40 @@ impl Filter {
}
false
}
+
+ /// Remove aliases from a predicate for use in a `Filter`
+ ///
+ /// filter predicates should not contain aliased expressions so we remove
+ /// any aliases.
+ ///
+ /// before this logic was added we would have aliases within filters such
as
+ /// for benchmark q6:
+ ///
+ /// ```sql
+ /// lineitem.l_shipdate >= Date32(\"8766\")
+ /// AND lineitem.l_shipdate < Date32(\"9131\")
+ /// AND CAST(lineitem.l_discount AS Decimal128(30, 15)) AS
lineitem.l_discount >=
+ /// Decimal128(Some(49999999999999),30,15)
+ /// AND CAST(lineitem.l_discount AS Decimal128(30, 15)) AS
lineitem.l_discount <=
+ /// Decimal128(Some(69999999999999),30,15)
+ /// AND lineitem.l_quantity < Decimal128(Some(2400),15,2)
+ /// ```
+ pub fn remove_aliases(predicate: Expr) -> Result<Transformed<Expr>> {
+ predicate.transform_down(|expr| {
+ match expr {
+ Expr::Exists { .. } | Expr::ScalarSubquery(_) |
Expr::InSubquery(_) => {
+ // subqueries could contain aliases so we don't recurse
into those
+ Ok(Transformed::new(expr, false, TreeNodeRecursion::Jump))
+ }
+ Expr::Alias(_) => Ok(Transformed::new(
+ expr.unalias(),
Review Comment:
Since we know that expr is an `Expr::Alias` maybe we could use
`Expr::Alias(alias) => *alias.expr` instead of calling `.unalias()`, that
matches the expr again.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscr...@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscr...@datafusion.apache.org
For additional commands, e-mail: github-h...@datafusion.apache.org
