Repository navigation
fix(sql): preserve RECURSIVE keyword when rebuilding WITH clause - #12041
Open
bhargaviHQ wants to merge 5 commits into
Open
bhargaviHQ wants to merge 5 commits into
bhargaviHQ wants to merge 5 commits into
Conversation
The translate() and add_query_to_expr() methods in SQLGlotCompiler discarded the With node's recursive flag when reconstructing CTEs. Capture the flag before the pop and restore it after the reduce. Fixes ibis-project#11949
github-actions
Bot
added
tests
…ITH RECURSIVE support
deepyaman
self-requested a review
October 10, 2026 06:07
Collaborator
|
@bhargaviHQ Sorry, I know this has been out there for a while, but I'm happy to review it. |
deepyaman
added a commit
that referenced
this pull request
Oct 12, 2026
Restrict pytest to the tests relevant to #12041 so the BigQuery job finishes within its 60-minute limit and prints its failure summary, and so the other backends turn around faster. Jobs that select none of these tests (doctests, core) will exit with "no tests collected". Assisted-by: Claude Opus 5.5 <noreply@anthropic.com>
deepyaman
requested changes
Oct 12, 2026
Collaborator
There was a problem hiding this comment.
Can you take a look at #12153 and see what you think of the slightly updated approach? Happy to discuss if you have any thoughts/concerns. I also ran CI across all backends, including the cloud ones that currently work.
Edit: Just to be clear, I only opened #12153 to validate some ideas/run cloud backends; I don't intend to merge that, but rather you update this PR based on whatever makes sense.
Contributor
Author
|
Thanks @deepyaman for running this across all the backends. The updated approach makes sense, I will update this PR with it and the tests from #12153. |
…ive-cte-11949-clean
Co-authored-by: Deepyaman Datta <deepyaman.datta@utexas.edu>
Contributor
Author
|
Updated with the changes from #12153. |
…ive-cte-11949-clean
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description of changes
translate()andadd_query_to_expr()rebuild theWITHclause from its CTEs and drop the originalWithnode, which lost theRECURSIVEflag. The flag is now passed towith_(recursive=...)when the clause is rebuilt. Inadd_query_to_expr()it's kept if either the user's query or the ibis expression had it, so a recursive CTE coming from the table passed toTable.sql()also works.CTE aliases are now built as
TableAliasinstead of a bare identifier, because sqlglot's Trino/Presto generator readsalias.columnswhen the clause is recursive. The same change is applied to SingleStoreDB's copy ofadd_query_to_expr(). On MSSQL this makes CTE projections explicit ([t0].[key] AS [key]), so one snapshot changed, with the same meaning.Known limitations: BigQuery doesn't allow a nested
WITH RECURSIVE, and on Trino/Athena a recursive query over a plain table still renders the CTE alias as"t"("*"), which already failed on main.Issues closed