alamb commented on code in PR #10835:
URL: https://github.com/apache/datafusion/pull/10835#discussion_r1644995595
##########
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 think it does -- I will do so as a follow on PR
##########
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:
that is a good invariant -- I did not know that. Changed to an assert in in
8dd62566a
##########
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:
👍 Done in 550e4455b
--
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
