Skip to content

Unions of recursive cte queries - regression with merger of pull request #99 #107

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

Closed
wants to merge 4 commits into from

Conversation

pgammans
Copy link
Contributor

@pgammans pgammans commented Apr 3, 2025

Hi @millerdev,

Two new test case for union of cte queries.
The main one is testcase test_recursive_union_query_with_cte which show the regression caused with the merger of pull request #99 as per the comments by @rgammans the second to ensure we don't break normal unions with CTE models

Unfortunately, this testcase is not the most optimal CTE but shows the issue without adding any new test models.
If pull #99 is reverted this test case then passes, with #99 it cases this testcase to fail raising a OperationalError from the DB driver

@pgammans
Copy link
Contributor Author

pgammans commented Apr 3, 2025

The last commit fixes the issue but i like to look at this in relation to pull #59

@millerdev
Copy link
Contributor

@pgammans thank you for the tests and fix!

with #99 it cases this testcase to fail raising a OperationalError from the DB driver

I did not understand this part. Does that mean there is still a problem with this PR, or are we good to merge this and create a new release?

@pgammans
Copy link
Contributor Author

pgammans commented Apr 4, 2025

@millerdev
This PR was more an example the regression caused by the merge of #99 and not directly a solution.

The test cases in commit e2e70e0 show the regression due to merging #99 this test would pass if the commits in #99 were reverted, then next commit was just to ensure i do not break normal non cte unions.

Commit 54a6788 the 4th commit is just fixed the test case for use on PostgreSQL not SQLite3

This said the commit b7f4b5e is start on fixing the issues so the testcases from #99 and the ones here pass.
After looking at this issue I think the code change in #99 is correct but that when we do the union (and possibly other operations) of a CTE query and another query we end up with the cte on both the CTEQuery attached to the resultant union QuerySet and one of on of the CTEQuerys sub combined_queries the commit b7f4b5e is that start of a possible fix.

with #99 the SQL generated from the test is where before you only have 1 with statement

WITH RECURSIVE "leaf_cte" AS (
     SELECT ....)
WITH RECURSIVE "leaf_cte" AS (
     SELECT ....)
SELECT .... ````

The 1st  come from the query the second comes from the first query in that queries `combined_queries`

@millerdev
Copy link
Contributor

@pgammans, #113 should be a more complete fix. Let me know if you are able to test it. I think we can now close this PR. And thank you for the contribution of test cases and the start to a fix!

@millerdev
Copy link
Contributor

Closing. Please create a new PR if there is anything important here that was not incorporated into #113

@millerdev millerdev closed this May 27, 2025
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