support for non-correlated subqueries #3287
Conversation
|
Thanks for looking at this! It's good to have another person touching this code. I'm happy to help in any way if I can! |
|
@andygrove PTAL |
|
I tested the SQL in #3266 and that works locally. As well as the query in TPCH 15. |
|
If query 15 works, I'd suggest adding a test here https://github.com/apache/arrow-datafusion/blob/7aed4d697fa24053d515babfd7678855451c6736/datafusion/core/tests/sql/subqueries.rs#L31 |
| @@ -702,4 +727,31 @@ mod tests { | |||
| assert_optimized_plan_eq(&DecorrelateScalarSubquery::new(), &plan, expected); | |||
| Ok(()) | |||
| } | |||
|
|
|||
| /// Test for non-correlated scalar subquery with no filters | |||
There was a problem hiding this comment.
Should we rename this if it also does uncorrelated subqueries now? Maybe scalar_subquery_to_join?
There was a problem hiding this comment.
You mean rename the whole rule? Probably so .. good idea. You want to do that as part of this issue?
There was a problem hiding this comment.
I don't want to, but we probably should. Maybe we can just doc it for now with a TODO and address it in a future PR?
There was a problem hiding this comment.
It touches a bunch of code, but yes, your suggested name is much more clear. I'll do a separate PR for it this week if this gets merged in.
|
|
||
| /// Test for non-correlated scalar subquery with no filters | ||
| #[test] | ||
| fn scalar_subquery_non_correlated_no_filters() -> Result<()> { |
There was a problem hiding this comment.
Looks like 1 branch was added, and 1 test. LGTM!
Codecov Report
@@ Coverage Diff @@
## master #3287 +/- ##
=======================================
Coverage 85.92% 85.93%
=======================================
Files 294 294
Lines 53469 53483 +14
=======================================
+ Hits 45945 45960 +15
+ Misses 7524 7523 -1
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
There was a problem hiding this comment.
Thanks @kmitchener.
Changes look reasonable to me, but I am mostly approving based on the review from @avantgardnerio, who wrote the original code. Thanks for the review @avantgardnerio!
|
Benchmark runs are scheduled for baseline = 256ea91 and contender = fb2f0db. fb2f0db 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 #3266.
Rationale for this change
What changes are included in this PR?
Handle case where subquery doesn't have a filter.
Are there any user-facing changes?