Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Adds planning and evaluation support for CTEs #1738

Merged
merged 1 commit into from
Feb 14, 2025

Conversation

johnedquinn
Copy link
Member

@johnedquinn johnedquinn commented Feb 7, 2025

Relevant Issues

Description

  • Adds planning and evaluation support for CTEs
  • On a basic level, this PR:
    • Adds an internal plan node, called With that lies at the top of an ExprQuerySet (Rex). When we go to type this sub-tree, we adjust our environment to include the with clause's elements. In an ideal world, we would immediately type on the conversion from the AST to plan, and we wouldn't need this fairly odd logic, but it is what it is (for now).
    • Related to the above, updates the definition of the internal environment Scope to contain a list of WithListElements. Then, when attempting to resolve variables locally, the scope is able to find the WithListElement's representation (an ExprQuerySet) and return its value. This essentially replaces references to the CTE variables with their definitions. This is what PostgreSQL does when a CTE is referenced once.
  • This approach has one main pro: we don't need to update the public plan. Eventually, if we'd like, we may alter this approach with a variety of designs, namely:
    • A wrapper over a relation -- indicating the name of the CTE for further inspection.
    • The current approach, which is what PostgreSQL uses when the CTE is only referenced once.
    • A common CTE available to a sub-plan -- primarily for optimization purposes (memoization of the rows). This is what PostgreSQL uses when the CTE is referenced multiple times.
  • This PR does NOT add support for RECURSIVE CTEs nor does it provide support for the with list element's column list. Please see the screenshots at the bottom of this description.

Release Plan

  • There are only API additions related to a new kind of PError (one that should have existed before). The new error (DEGREE_VIOLATION_SCALAR_SUBQUERY) is used when the degree of a scalar subquery is not 1. This allows for comprehensive testing of the WITH clause. It is also useful to the end-user.
  • There are NO API changes/additions in relation to the plan.
  • This will be released as part of v1.1.0. There are NO API changes. Only feature additions.
  • As for the release of v1.1.0, I'd like to gain some clarity on whether the with element's optional column list is expected for planning/evaluation. If this is needed, there are a few changes that we'll need to make.

Examples

Below is an example of a CTE embedded within a CTE:

Screenshot 2025-02-06 at 10 12 51 AM

Below is an example of the errors you may encounter:

Screenshot 2025-02-07 at 9 47 34 AM

License Information

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@johnedquinn johnedquinn force-pushed the v1-with-clause-eval branch 2 times, most recently from 26a8d22 to 2ee7990 Compare February 7, 2025 18:05
Adds PError for scalar subquery degree violation
@johnedquinn johnedquinn requested a review from jpschorr February 7, 2025 18:09
@johnedquinn johnedquinn changed the title [DRAFT] Adds planning and evaluation support for CTEs Adds planning and evaluation support for CTEs Feb 7, 2025
@johnedquinn johnedquinn marked this pull request as ready for review February 7, 2025 18:10
@jpschorr
Copy link
Contributor

I believe this is the final PR for adding the currently-planned level of CTE support. We should probably update the Changelog.

Copy link
Contributor

@jpschorr jpschorr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good! I just have a couple of questions and non-blocking comments.

WITH
x AS (SELECT VALUE t FROM << 1, 2, 3 >> t),
y AS (SELECT VALUE x FROM x)
SELECT * FROM y; -- y should not be able to be referenced.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

-- y should not be able to be referenced.

y is fine, no? It's the x that should not be able to be referenced inside y?

@@ -242,6 +242,14 @@ internal class PlanTransform(private val flags: Set<PlannerFlag>) {
return operators.aggregate(input, calls, groups)
}

override fun visitRelOpWith(node: Rel.Op.With, ctx: PType): Any {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why return Any and not org.partiql.plan.rel.Rel?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the end, it doesn't matter. These all get cast to Rels.

@johnedquinn johnedquinn merged commit 86e8897 into partiql:main Feb 14, 2025
7 checks passed
@johnedquinn johnedquinn deleted the v1-with-clause-eval branch February 14, 2025 18:18
@johnedquinn
Copy link
Member Author

Merging this, and going to add the minor updates to the release commit.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants