Plan `LATERAL` subqueries #11456

aalexandrov · 2024-07-13T22:16:20Z

Which issue does this PR close?

Closes the planning part of #10048. The query from the issue will now fail in the optimization phase similar to #6140.

Rationale for this change

Adds an unsupported fragment of the SQL syntax.

What changes are included in this PR?

Extends the planner with support for LATERAL subqueries. See the tests added in sql_integration.rs for details.

Are these changes tested?

Yes, I added tests in sql_integration.rs.

Are there any user-facing changes?

If there are user-facing changes then we may require documentation to be updated before approving the PR.

If we document the supported SQL fragment in the user-facing docs as BNF-style diagrams we might want to adapt those.

I don't think so.

ozankabak · 2024-07-14T12:14:16Z

Thank you! IMO, missing lateral and correlated subquery support is one of the crucial things to fix in DF. Any work in this area is much appreciated.

aalexandrov · 2024-07-14T14:31:08Z

datafusion/sql/src/relation/mod.rs

+        // the `for` loop in `plan_table_with_joins`.
+        let old_from_schema = planner_context.set_outer_from_schema(None).unwrap();
+        let new_query_schema = match planner_context.outer_query_schema() {
+            Some(lhs) => Some(Arc::new(lhs.join(&old_from_schema)?)),


We probably want to replace the lhs.join(<rhs>) call here with a version of a join where identical column references on the right side override their lhs counterparts (probably lhs.merge(<rhs>) will suffice).

See #11464 for details.

I added a second commit to the PR that fixes the issue mentioned above (see the commit message for details).

aalexandrov · 2024-07-14T14:31:25Z

datafusion/sql/src/planner.rs

+    /// extends the FROM schema, returning the existing one, if any
+    pub fn extend_outer_from_schema(&mut self, schema: &DFSchemaRef) -> Result<()> {
+        self.outer_from_schema = match self.outer_from_schema.as_ref() {
+            Some(from_schema) => Some(Arc::new(from_schema.join(schema)?)),


~~See my other comment.~~

Actually, looking at the Postgres behavior it seems that keeping join here is better (the j1_id column reference is then reported as ambiguous):

SELECT j1_string, j2_string FROM j1 AS x JOIN j1 as y USING(j1_string) LEFT JOIN LATERAL (SELECT * FROM j2 WHERE j1_id < j2_id) AS j2 ON(true);

alamb · 2024-07-14T15:51:44Z

Thank you @aalexandrov -- I will review this PR carefully, but likely will not have a chance until tomorrow or Tuesday

jonahgao · 2024-07-16T15:05:26Z

datafusion/sql/src/relation/mod.rs

+        // factors, and those can never be the first factor in a FROM list. This
+        // means we arrived here through the `for` loop in `plan_from_tables` or
+        // the `for` loop in `plan_table_with_joins`.
+        let old_from_schema = planner_context.set_outer_from_schema(None).unwrap();


I tested the following query, and it panicked. It's valid in PostgreSQL.

DataFusion CLI v40.0.0 > select * from lateral (select 1) t; thread 'main' panicked at datafusion/datafusion/sql/src/relation/mod.rs:162:75: called `Option::unwrap()` on a `None` value

Somehow I was thinking that LATERAL is not valid / accepted by the parser for the first element in a SELECT list. I fixed the issue and added a regression test in sql_integration.rs.

alamb · 2024-07-16T19:51:11Z

This PR is very much on my list to review, but I fear I may not get to it today (I have a bunch of PRs waiting upstream in arrow-rs I need to attend to)

alamb · 2024-07-22T19:21:53Z

Still on my list... .

I am not sure if @jackwener or @mingmwang might have time to review this (or if they are familiar with this particular rewrite)

alamb

Thanks @aalexandrov -- sorry for not getting a chance to review this PR earlier (I have been quite busy with other stuff and JOINs are pretty low on my priority list unfortunately)

The basic code in this PR looks good to me-- but I don't know enough about the semantics of lateral joins to know if it is

Can you please add some tests to sqllogictest that shows that this code gets the same code in postgres and in DataFusion

Here are the instructions: https://github.com/apache/datafusion/tree/main/datafusion/sqllogictest

The instructions for postgres compatibility tests are here
https://github.com/apache/datafusion/tree/main/datafusion/sqllogictest#running-tests-postgres-compatibility

Ideally you should be able to extend one of the existing test files in https://github.com/apache/datafusion/tree/main/datafusion/sqllogictest/test_files

aalexandrov · 2024-08-17T08:42:02Z

Can you please add some tests to sqllogictest that shows that this code gets the same code in postgres and in DataFusion.

This PR only focuses on extending datafusion/sql/src/planner.rs so we can get logical plans for the LATERAL syntax.

I was hoping to merge that first before taking a stab at the required extensions in datafusion/sql/src/planner.rs that would allow us to run those queries. Once we have those I will be able to add some SLT tests and fix any newly uncovered bugs in both files.

alamb · 2024-08-17T10:54:53Z

I was hoping to merge that first before taking a stab at the required extensions in datafusion/sql/src/planner.rs that would allow us to run those queries. Once we have those I will be able to add some SLT tests and fix any newly uncovered bugs in both files.

Implementing in phases makes sense to me and is consistent with what we have done in other places

However, I still think we should add basic .slt test coverage (even if they simply error our with "unsupported subquery") is to document, with code, what the expected behavior is if someone runs this kind of query.

I think a runtime "not yet implemented" error is fine, but going into infinite loops or internal errors is probably less good.

In order to compute the `set_outer_from_schema` argument we currently use `DFSchema::join`. When we combine the current outer FROM schema with the current outer query schema columns from the latter should override columns from the first, so the correct way is to use `DFSchema::merge`. To witness the fix, note that the query in the fixed test case isn't planned as expected without the accompanying changes.

aalexandrov · 2024-08-19T13:41:26Z

I added some tests to check the behavior of the SQL planner and assert the current runtime behavior (unimplemented because physical planning doesn't know what to do with correlated column references) to joins.slt.

alamb

Thank you @aalexandrov - this code and tests look good to me.

I again apologize for not having a chance to review this PR sooner.

All in all, I think this is a good step forward.

I took the liberty of merging this PR up from main to resolve a conflict and fixed some expected tests

cc @jonahgao in case you are interested in reviewing the SQL planning aspects

alamb · 2024-08-20T17:02:27Z

🚀

alamb · 2024-08-20T17:02:41Z

Thanks again for bearing with us @aalexandrov -- this is a great step

aalexandrov · 2024-08-26T19:56:13Z

Thanks, @alamb. Can you point me to the unnesting rules that already exist? I might try to extend them to cover some of the more basic queries first.

alamb · 2024-08-26T19:58:40Z

Thanks, @alamb. Can you point me to the unnesting rules that already exist? I might try to extend them to cover some of the more basic queries first.

The optimizer rules are here: https://docs.rs/datafusion/latest/datafusion/optimizer/optimizer/index.html

I think in particular two that are relevant are:

github-actions bot added documentation Improvements or additions to documentation sql SQL Planner development-process Related to development process of DataFusion labels Jul 13, 2024

aalexandrov mentioned this pull request Jul 13, 2024

Feature request: Support for lateral joins #10048

Open

aalexandrov force-pushed the plan_from_lateral branch from 7cf77ec to 5cb0c79 Compare July 14, 2024 09:17

github-actions bot removed documentation Improvements or additions to documentation development-process Related to development process of DataFusion labels Jul 14, 2024

aalexandrov force-pushed the plan_from_lateral branch from 5cb0c79 to f54781e Compare July 14, 2024 10:03

aalexandrov changed the title ~~Plan from lateral~~ Plan LATERAL subqueries Jul 14, 2024

ozankabak requested review from Dandandan and alamb July 14, 2024 12:14

aalexandrov mentioned this pull request Jul 14, 2024

DFSchema::check_names allows ambiguous references. #11464

Closed

aalexandrov marked this pull request as draft July 14, 2024 12:50

aalexandrov force-pushed the plan_from_lateral branch from f54781e to ee34089 Compare July 14, 2024 14:25

aalexandrov marked this pull request as ready for review July 14, 2024 14:28

aalexandrov commented Jul 14, 2024

View reviewed changes

alamb mentioned this pull request Jul 14, 2024

DataFusion weekly project plan (Andrew Lamb) - July 8, 2024 #11334

Closed

9 tasks

aalexandrov force-pushed the plan_from_lateral branch 2 times, most recently from c0a65a4 to db91644 Compare July 15, 2024 09:11

alamb mentioned this pull request Jul 15, 2024

DataFusion weekly project plan (Andrew Lamb) - July 15, 2024 #11474

Closed

7 tasks

jonahgao reviewed Jul 16, 2024

View reviewed changes

aalexandrov force-pushed the plan_from_lateral branch from db91644 to 79aab9d Compare July 17, 2024 08:28

alamb mentioned this pull request Jul 22, 2024

DataFusion weekly project plan (Andrew Lamb) - July 22, 2024 #11601

Closed

5 tasks

This was referenced Aug 1, 2024

DataFusion weekly project plan (Andrew Lamb) - July 29, 2024 #11710

Closed

DataFusion weekly project plan (Andrew Lamb) - Aug 5, 2024 #11826

Closed

alamb reviewed Aug 13, 2024

View reviewed changes

aalexandrov added 2 commits August 19, 2024 16:38

Planner: support LATERAL subqueries

a214b7d

aalexandrov force-pushed the plan_from_lateral branch from 79aab9d to 4274826 Compare August 19, 2024 13:40

github-actions bot added the sqllogictest SQL Logic Tests (.slt) label Aug 19, 2024

alamb added 2 commits August 19, 2024 14:19

Merge remote-tracking branch 'apache/main' into plan_from_lateral

e531fa1

Update plans

7416f6e

alamb approved these changes Aug 19, 2024

View reviewed changes

alamb merged commit 6e34280 into apache:main Aug 20, 2024
24 checks passed

alamb mentioned this pull request Sep 10, 2024

[DISCUSS] Document criteria for adding new features / what belongs in core DataFusion (e.g. sql syntax, functions, etc) #12357

Open

eejbyfeldt mentioned this pull request Sep 25, 2024

Disallow duplicated qualified field names #12608

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Plan `LATERAL` subqueries #11456

Plan `LATERAL` subqueries #11456

aalexandrov commented Jul 13, 2024

ozankabak commented Jul 14, 2024

aalexandrov Jul 14, 2024 •

edited

Loading

aalexandrov Jul 14, 2024

aalexandrov Jul 14, 2024 •

edited

Loading

alamb commented Jul 14, 2024

jonahgao Jul 16, 2024

aalexandrov Jul 17, 2024

alamb commented Jul 16, 2024

alamb commented Jul 22, 2024

alamb left a comment

aalexandrov commented Aug 17, 2024 •

edited

Loading

alamb commented Aug 17, 2024

aalexandrov commented Aug 19, 2024

alamb left a comment

alamb commented Aug 20, 2024

alamb commented Aug 20, 2024

aalexandrov commented Aug 26, 2024

alamb commented Aug 26, 2024

Plan LATERAL subqueries #11456

Plan LATERAL subqueries #11456

Conversation

aalexandrov commented Jul 13, 2024

Which issue does this PR close?

Rationale for this change

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

ozankabak commented Jul 14, 2024

aalexandrov Jul 14, 2024 • edited Loading

Choose a reason for hiding this comment

aalexandrov Jul 14, 2024

Choose a reason for hiding this comment

aalexandrov Jul 14, 2024 • edited Loading

Choose a reason for hiding this comment

alamb commented Jul 14, 2024

jonahgao Jul 16, 2024

Choose a reason for hiding this comment

aalexandrov Jul 17, 2024

Choose a reason for hiding this comment

alamb commented Jul 16, 2024

alamb commented Jul 22, 2024

alamb left a comment

Choose a reason for hiding this comment

aalexandrov commented Aug 17, 2024 • edited Loading

alamb commented Aug 17, 2024

aalexandrov commented Aug 19, 2024

alamb left a comment

Choose a reason for hiding this comment

alamb commented Aug 20, 2024

alamb commented Aug 20, 2024

aalexandrov commented Aug 26, 2024

alamb commented Aug 26, 2024

Plan `LATERAL` subqueries #11456

Plan `LATERAL` subqueries #11456

aalexandrov Jul 14, 2024 •

edited

Loading

aalexandrov Jul 14, 2024 •

edited

Loading

aalexandrov commented Aug 17, 2024 •

edited

Loading