fix: failed to execute sql with subquery #5542
Conversation
There was a problem hiding this comment.
Thank you @MichaelScofield !
This looks great to me.
Can you please add one SQL level test to verify that the plumbing is all hooked up correctly.
I recommend using in https://github.com/apache/arrow-datafusion/tree/main/datafusion/core/tests/sqllogictests
| @@ -390,6 +390,22 @@ impl<'a, S: SimplifyInfo> ExprRewriter for Simplifier<'a, S> { | |||
| lit(negated) | |||
| } | |||
|
|
|||
| // expr IN ((subquery)) -> expr IN (subquery), see ##5529 | |||
| @@ -2849,6 +2866,24 @@ mod tests { | |||
| simplify(in_list(col("c1"), vec![lit(1), lit(2)], true)), | |||
| col("c1").not_eq(lit(2)).and(col("c1").not_eq(lit(1))) | |||
| ); | |||
|
|
|||
| let subquery = Arc::new(test_table_scan_with_name("test").unwrap()); | |||
There was a problem hiding this comment.
Can you please also add a negative test (with two subqueries)?
| } if list.len() == 1 | ||
| && matches!(list.first(), Some(Expr::ScalarSubquery { .. })) => | ||
| { | ||
| let Expr::ScalarSubquery(subquery) = list.remove(0) else { unreachable!() }; |
There was a problem hiding this comment.
Struggling to get why we unreachable here
There was a problem hiding this comment.
The condition in L399 ensures that only ScalarSubquery is allowed, making other branches unreachable. Although there is an unstable rust feature available to simplify this, it cannot be used at the moment.
github-actions
bot
added
core
|
I've added 3 test cases in sqllogictest. The cases are from our original intention to test subquery with "(( ))". PTAL @alamb |
|
Benchmark runs are scheduled for baseline = 9261aa3 and contender = 1a22f9f. 1a22f9f 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 #5529 .
Rationale for this change
fix: failed to run "where x in ((select ...))"
What changes are included in this PR?
rewrite
Expr::InListwith only one subquery toExpr::InSubqueryduring logical plan optimizationAre these changes tested?
yes
Are there any user-facing changes?
no