alamb commented on code in PR #15980:
URL: https://github.com/apache/datafusion/pull/15980#discussion_r2078420969
##########
datafusion/expr/src/expr.rs:
##########
@@ -1775,6 +1775,27 @@ impl Expr {
| Expr::SimilarTo(Like { expr, pattern, .. }) => {
rewrite_placeholder(pattern.as_mut(), expr.as_ref(),
schema)?;
}
+ Expr::InSubquery(InSubquery {
+ expr,
+ subquery,
+ negated: _,
+ }) => {
+ let subquery_schema = subquery.subquery.schema();
+ let fields = subquery_schema.fields();
+
+ // only supports subquery with exactly 1 field
+ if let [first_field] = &fields[..] {
Review Comment:
What happens if there is more than one field? Will it not rewrite any
placeholders?
##########
datafusion/expr/src/logical_plan/plan.rs:
##########
@@ -1494,6 +1494,14 @@ impl LogicalPlan {
let mut param_types: HashMap<String, Option<DataType>> =
HashMap::new();
self.apply_with_subqueries(|plan| {
+ if let LogicalPlan::Limit(Limit { fetch: Some(e), .. }) = plan {
Review Comment:
I don't understand why the expression in the LIMIT needs special handling.
It seems like it should be handled by `LogicalPlan::apply_expressions` here
https://github.com/apache/datafusion/blob/41e7aed3a943134c40d1b18cb9d424b358b5e5b1/datafusion/expr/src/logical_plan/tree_node.rs#L462-L461
Also this code doesn't seem to handle the `offset` clause, only the `fetch`
##########
datafusion/expr/src/logical_plan/plan.rs:
##########
@@ -1507,6 +1515,9 @@ impl LogicalPlan {
(_, Some(dt)) => {
param_types.insert(id.clone(),
Some(dt.clone()));
}
+ (Some(Some(_)), None) => {
+ // we have already inferred the datatype
Review Comment:
When can this happen? Like what situations would one instance of the
parameter like `$2` be resolved but another instance would not be 🤔
##########
datafusion/expr/src/logical_plan/plan.rs:
##########
@@ -1494,6 +1494,14 @@ impl LogicalPlan {
let mut param_types: HashMap<String, Option<DataType>> =
HashMap::new();
self.apply_with_subqueries(|plan| {
+ if let LogicalPlan::Limit(Limit { fetch: Some(e), .. }) = plan {
Review Comment:
However, when I removed this special case the tests you have added
definitely fail 🤔 Something really strange is going on
--
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
