Replace placeholders in ScalarSubqueries #5216
Conversation
github-actions
bot
added
logical-expr
|
@NGA-TRAN PTAL as well. |
| Ok(Expr::Literal(value.clone())) | ||
| } else { | ||
| Ok(expr) | ||
| Expr::ScalarSubquery(qry) => { |
There was a problem hiding this comment.
👍
I double checked this recursion doesn't happen automatically during the walk
Though now that I write that, perhaps it should 🤔
There was a problem hiding this comment.
Where are you suggesting the recursion goes? self.expressions()? I'm happy to file a new PR to do it a better way...
There was a problem hiding this comment.
I was thinking that the recursion into ScalarSubquery would happen as part of the ExprRewriter itself -- aka https://github.com/apache/arrow-datafusion/blob/e222bd627b6e7974133364fed4600d74b4da6811/datafusion/expr/src/expr_rewriter.rs#L132
Expr::ScalarSubquery(_) => self.clone(), Instead of self.clone() it would be something like you have in this PR
Expr::ScalarSubquery(qry) => self.clone(),
let rewritten_subquery = somehow_rewrite_all_exprs_in_query(rewriter)
Expr::ScalarSubquery(rewritten_subquery)
}
|
Benchmark runs are scheduled for baseline = f0c6719 and contender = b4cf60a. b4cf60a is a master commit associated with this PR. Results will be available as each benchmark for each run completes. |
Which issue does this PR close?
Closes #5215.
Rationale for this change
Described in issue.
What changes are included in this PR?
Replace parameters in scalar subqueries.
Are these changes tested?
With an integration test, yes.
Are there any user-facing changes?
Scalar subqueries with parameters should work.