Add support for recursive CTEs #7581

matthewgapp · 2023-09-17T06:26:12Z

This PR implements support for recursive CTEs based on @isidentical's design here #462 (comment)

Caveats:

It does not attempt to place safeguards against infinite recursion. That could be implemented as a follow-up PR. It would be great to have some syntax like OPTON (MAXRECURSION 100) to prevent infinite recursion. Since the sql parser crate doesn’t support OPTIONS (MAXRECURSION) I might create a max-recursion parameter to the logical and physical plans that can be inserted manually once the logical plan is produced via SQL as a near-term workaround to protect against infinite recursion. I'll do that in a separate follow-on PR though.

It's slower than it needs to be because of the execution plan inserts coalesce and repartition execution steps within the recursive term. This optimization is solved via this follow-on PR. There is also room to omit gathering of statistics for each execution iteration because setting up statistics on each execution iteration is expensive.

Which issue does this PR close?

Closes #462

Rationale for this change

Datafusion should support recursive CTEs so that it can express calculations that depend on the results of previous iterations.

What changes are included in this PR?

Are these changes tested?

Yes, they're tested via the SQL tests. The SQL tests use CSVs introduced by this PR apache/arrow-testing#93

Are there any user-facing changes?

matthewgapp · 2023-09-25T04:39:00Z

Started adding tests, will continue to add more.

matthewgapp · 2023-10-13T14:35:38Z

I've been out for the last couple weeks because life got busy. I'll be back on this soon to push it over the finish line.

matthewgapp · 2023-11-18T21:45:02Z

Rebased, plan to finish this out today/tomorrow.

matthewgapp · 2023-11-19T01:05:49Z

@alamb Finally got around to cleaning this up. I think this is ready for initial review. Happy to hear what other tests/safeguards you'd like to see in the initial version. But would be great to get in so that I can continue layering on improvements without the PR blowing up into too much of a larger change.

datafusion/physical-plan/src/recursive_query.rs

Lint fixes

in the physical-plan

datafusion/core/src/physical_planner.rs

wip: fixes after rebase but tpcds_physical_q54 keeps overflowing its stack

matthewgapp · 2024-01-09T06:28:52Z

datafusion/sql/tests/sql_integration.rs

@@ -1387,22 +1387,6 @@ fn select_interval_out_of_range() {
    );
 }

-#[test]
-fn recursive_ctes() {


jonahgao · 2024-01-09T12:59:00Z

datafusion/expr/src/logical_plan/plan.rs

@@ -112,6 +112,8 @@ pub enum LogicalPlan {
    /// produces 0 or 1 row. This is used to implement SQL `SELECT`
    /// that has no values in the `FROM` clause.
    EmptyRelation(EmptyRelation),
+    /// A named temporary relation with a schema.
+    NamedRelation(NamedRelation),


I am considering whether the NamedRelation and RecursiveQuery could be implemented as two TableSources, one being CTESelfRefTable and the other being CTERecursiveTable, and then use TableScan to read them.

Use CTESelfRefTable within the recursive term and CTERecursiveTable in the outer query.

But this idea is in its early stages and may be wrong.

@jonahgao, could you provide the rationale for your suggested strategy? I'm interested in understanding why it might be more effective than the current implementation. Performance is critical to our use case. And the implementation for recursion is very sensitive to performance considerations, as the setup for execution and stream management isn't amortized over all input record batches. Instead, it's incurred with each iteration. For instance, we've observed a substantial performance boost—up to 30 times faster—by eliminating certain intermediate nodes, like coalesce, from our plan (as evidenced in this PR). I've drafted another PR that appears to again double the speed of execution merely by omitting metric collection in recursive sub-graphs.

One rationale might be to make the implementation simpler -- if we could implement the recursive relation as a table provider, it would likely allow the changes to be more localized / smaller (e.g. maybe we could reuse MemTable::load to update the batches on each iteration)

Basically I understand the need to have LogicalPlan::RecursiveQuery but I don't (yet) understand the need to have the NamedRelation

NamedRelation is primarily a way to mirror batches back to the RecursiveQuery via its physical counterpart, ContinuanceExec

@matthewgapp Another rationale might be to support pushing down filters to the working table, which may be useful if we support spilling the working table to disk in the future. I think the performance should not be affected, the execution of physical plans is almost the same as it is now.

I implemented a demo on this branch and in this commit. GitHub does not allow forking a repository twice, so I directly pushed it to another repository for convenience.

In this demo, I attempted to replace the NamedRelation with a TableProvider, namely CteWorkTable. The benefit of this is that it can avoid maintaining a new logical plan.

Another change is that I used a structure called WorkTable to connect the RecursiveQueryExec and the WorkTableExec (it was previously ContinuanceExec). The advantage of this is that it avoids maintaining some external context information, such as relation_handlers in TaskContext, and the ctx in create_initial_plan.

The WorkTable is a shared table, it will be scanned by the WorkTableExec during the execution of the recursive term, and after the execution is completed, the results will be written back to it.

Wow, tyty! I was in the process of implementing the shared table and my implementation turned out very similar to yours although I ended up working around the crate dependency graph constraints a bit differently by introducing a couple new traits. But I did end up exposing a method on the context to generate a table. I like your approach better.

I tested out your poc and performance remains about the same between my previous implementation and your new worktable approach! (which makes sense).

I'm going to go ahead and work based on your POC toward the list of PRs that Andrew wants to get this landed.

Thank you for your work and for the nexting contributions! @matthewgapp

alamb

Thank you for your patience @matthewgapp -- I was finally able to find the few hours I needed to load all the context into my head to review this PR properly

I left some high level comments on the design (specifically how to simplify it / potentially keep the implementation more isolated). However overall I found this PR easy to read and follow. Nice work

BTW if anyone else needs a reminder how recursive CTEs work, here is the postgres documentation: https://www.postgresql.org/docs/11/queries-with.html#id-1.5.6.12.5.4

datafusion/expr/src/logical_plan/builder.rs

alamb · 2024-01-09T22:11:58Z

datafusion/expr/src/logical_plan/plan.rs

@@ -112,6 +112,8 @@ pub enum LogicalPlan {
    /// produces 0 or 1 row. This is used to implement SQL `SELECT`
    /// that has no values in the `FROM` clause.
    EmptyRelation(EmptyRelation),
+    /// A named temporary relation with a schema.
+    NamedRelation(NamedRelation),


One rationale might be to make the implementation simpler -- if we could implement the recursive relation as a table provider, it would likely allow the changes to be more localized / smaller (e.g. maybe we could reuse MemTable::load to update the batches on each iteration)

datafusion/physical-plan/src/continuance.rs

datafusion/physical-plan/src/recursive_query.rs

datafusion/sqllogictest/test_files/cte.slt

alamb · 2024-01-09T22:31:21Z

datafusion/physical-plan/src/recursive_query.rs

+        // Downstream plans should not expect any partitioning.
+        let partition = 0;
+
+        self.recursive_stream = Some(


Since this calls execute again recursively, if we used a TableProvider the underlying TableProvider::execute would be called again too

alamb · 2024-01-09T22:33:39Z

datafusion/expr/src/logical_plan/plan.rs

@@ -112,6 +112,8 @@ pub enum LogicalPlan {
    /// produces 0 or 1 row. This is used to implement SQL `SELECT`
    /// that has no values in the `FROM` clause.
    EmptyRelation(EmptyRelation),
+    /// A named temporary relation with a schema.
+    NamedRelation(NamedRelation),


Basically I understand the need to have LogicalPlan::RecursiveQuery but I don't (yet) understand the need to have the NamedRelation

alamb · 2024-01-09T22:36:16Z

cc @Dandandan for your thoughts

alamb · 2024-01-10T18:07:46Z

I thought more about this PR last night and in the shower. First of all, I think the direction it is headed is great and would make a good addition to DataFusion. Thank you @matthewgapp both for the code as well as for your patience

In terms of implementation, I think there are a few items that we need prior to making this feature available for everyone

More test coverage (I hilighted some areas)
Memory limits (so a recursive CTE where the intermediate relation grows without limit will not cause the process to be OOM killed). This is to prevent a denial of service (DOS) attack vector against systems that use DataFusion by exploding memory

Some nice to have (but not strictly required) items:

Simpler API (as @jonahgao and I have suggested)
A config flag to enable/disable support (as a way to protect against DOS)

In order to avoid a massive PR, my suggestion is to implement this feature in stages:

Add a config flag to enable/disable CTE support, default to disable
Add parser / LogicalPlan support in one PR
Add execution support in a second PR
Add memory limiting in another PR
Iterate / update tests
When we are happy that everything is working well, change the switches default to enabled by default

matthewgapp · 2024-01-11T00:55:51Z

I thought more about this PR last night and in the shower. First of all, I think the direction it is headed is great and would make a good addition to DataFusion. Thank you @matthewgapp both for the code as well as for your patience

In terms of implementation, I think there are a few items that we need prior to making this feature available for everyone

More test coverage (I hilighted some areas)

Memory limits (so a recursive CTE where the intermediate relation grows without limit will not cause the process to be OOM killed). This is to prevent a denial of service (DOS) attack vector against systems that use DataFusion by exploding memory

Some nice to have (but not strictly required) items:

Simpler API (as @jonahgao and I have suggested)

A config flag to enable/disable support (as a way to protect against DOS)

In order to avoid a massive PR, my suggestion is to implement this feature in stages:

Add a config flag to enable/disable CTE support, default to disable

Add parser / LogicalPlan support in one PR

Add execution support in a second PR

Add memory limiting in another PR

Iterate / update tests

When we are happy that everything is working well, change the switches default to enabled by default

Thanks @alamb! Think this impl plan makes sense. I'll start knocking out the PRs. Also going to have a go at reworking the existing PR to use MemTable in the two plans (as @jonahgao mentioned here #7581 (comment)). I'll link the resulting PRs to this one.

alamb · 2024-01-31T20:45:55Z

I think this PR has been superseded by other PRs on #462 (comment)

I think all that remains for CTE work is make sure it doesn't explode with memory and turn it on by default 🎉 (that is tracked by #462)

github-actions bot added sql SQL Planner logical-expr Logical plan and expressions optimizer Optimizer rules core Core DataFusion crate labels Sep 17, 2023

matthewgapp mentioned this pull request Sep 17, 2023

Add support for recursive CTEs #462

Closed

github-actions bot added the physical-expr Physical Expressions label Sep 17, 2023

matthewgapp force-pushed the matt/recursive-cte branch from deedb21 to 8b4a646 Compare September 17, 2023 19:53

matthewgapp changed the title ~~Recursive CTE (refreshed)~~ Add support for recursive CTEs (refreshed) Sep 18, 2023

matthewgapp changed the title ~~Add support for recursive CTEs (refreshed)~~ Add support for recursive CTEs Sep 18, 2023

github-actions bot removed the physical-expr Physical Expressions label Sep 19, 2023

matthewgapp force-pushed the matt/recursive-cte branch 3 times, most recently from 815c72f to f9614d0 Compare September 19, 2023 03:14

github-actions bot added the sqllogictest SQL Logic Tests (.slt) label Sep 25, 2023

matthewgapp mentioned this pull request Sep 25, 2023

Add csvs for recursive-cte sqllogictests apache/arrow-testing#93

Open

matthewgapp force-pushed the matt/recursive-cte branch from f5d7eba to e8d78c8 Compare November 18, 2023 21:44

matthewgapp marked this pull request as ready for review November 19, 2023 01:04

This was referenced Nov 20, 2023

DataFusion weekly project plan (Andrew Lamb) - Nov 20, 2023 #8274

Closed

DataFusion weekly project plan (Andrew Lamb) - Nov 27, 2023 #8329

Closed

matthewgapp mentioned this pull request Dec 19, 2023

DataFusion weekly project plan (Andrew Lamb) - Dec 18, 2023 #8577

Closed

7 tasks

alamb mentioned this pull request Dec 26, 2023

DataFusion weekly project plan (Andrew Lamb) - Dec 25, 2023 #8655

Closed

7 tasks

jonahgao reviewed Jan 5, 2024

View reviewed changes

datafusion/physical-plan/src/recursive_query.rs Show resolved Hide resolved

matthewgapp added 4 commits January 8, 2024 20:50

partway through porting over isidentical's work

4418ad2

Continuing implementation with fixes and improvements

dac7f22

Lint fixes

ensure that repartitions are not added immediately after RecursiveExec

daa995c

in the physical-plan

add trivial sqllogictest

aa6d74a

matthewgapp added 5 commits January 8, 2024 20:52

more recursive tests

5839dd1

remove test that asserts recursive cte should fail

d8af7fb

additional cte test

515312c

wip: remove tokio from physical plan dev deps

f9faa05

format cargo tomls

38e95dd

matthewgapp force-pushed the matt/recursive-cte branch from d7721f1 to e0724dd Compare January 9, 2024 05:48

matthewgapp commented Jan 9, 2024

View reviewed changes

datafusion/core/src/physical_planner.rs Show resolved Hide resolved

fix issue where CTE could not be referenced more than 1 time

219de0c

wip: fixes after rebase but tpcds_physical_q54 keeps overflowing its stack

matthewgapp force-pushed the matt/recursive-cte branch from 7dba5b6 to 219de0c Compare January 9, 2024 06:27

matthewgapp commented Jan 9, 2024

View reviewed changes

matthewgapp mentioned this pull request Jan 9, 2024

DataFusion weekly project plan (Andrew Lamb) - Jan 8, 2024 #8786

Closed

7 tasks

jonahgao reviewed Jan 9, 2024

View reviewed changes

alamb reviewed Jan 9, 2024

View reviewed changes

alamb mentioned this pull request Jan 11, 2024

Recursive CTEs: Stage 1 - add config flag #8828

Merged

alamb closed this Jan 31, 2024

jonahgao mentioned this pull request Mar 12, 2024

Enable recursive CTE support by default #9554

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for recursive CTEs #7581

Add support for recursive CTEs #7581

matthewgapp commented Sep 17, 2023 •

edited

Loading

matthewgapp commented Sep 25, 2023

matthewgapp commented Oct 13, 2023

matthewgapp commented Nov 18, 2023 •

edited

Loading

matthewgapp commented Nov 19, 2023

matthewgapp Jan 9, 2024

jonahgao Jan 9, 2024

matthewgapp Jan 9, 2024

alamb Jan 9, 2024

alamb Jan 9, 2024

matthewgapp Jan 10, 2024

jonahgao Jan 11, 2024

matthewgapp Jan 11, 2024

jonahgao Jan 12, 2024

alamb left a comment

alamb Jan 9, 2024

alamb Jan 9, 2024

alamb Jan 9, 2024

alamb commented Jan 9, 2024

alamb commented Jan 10, 2024

matthewgapp commented Jan 11, 2024

alamb commented Jan 31, 2024

Add support for recursive CTEs #7581

Add support for recursive CTEs #7581

Conversation

matthewgapp commented Sep 17, 2023 • edited Loading

Caveats:

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?

matthewgapp commented Sep 25, 2023

matthewgapp commented Oct 13, 2023

matthewgapp commented Nov 18, 2023 • edited Loading

matthewgapp commented Nov 19, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alamb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alamb commented Jan 9, 2024

alamb commented Jan 10, 2024

matthewgapp commented Jan 11, 2024

alamb commented Jan 31, 2024

matthewgapp commented Sep 17, 2023 •

edited

Loading

matthewgapp commented Nov 18, 2023 •

edited

Loading