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

resolve cte before resolving tables #1485

Merged
merged 8 commits into from
Dec 20, 2022
Merged

resolve cte before resolving tables #1485

merged 8 commits into from
Dec 20, 2022

Conversation

jycor
Copy link
Contributor

@jycor jycor commented Dec 19, 2022

Changes:

  • resolve the CTE before validating procedures
  • handle SubqueryAlias

Fixes: dolthub/dolt#4480

Copy link
Contributor

@max-hoffman max-hoffman left a comment

Choose a reason for hiding this comment

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

a) I'd try resolving the procedure in resolveCommonTableExpressions, it might be a bit more robust to future changes than manually doing the With in resolveProcedureVariables.
b) the exchange switch in join_order_builder signals an issue somewhere upstream. An exchange node means the tree has already been reordered. We can't reorder a query that has previously been planned.

Copy link
Contributor

@max-hoffman max-hoffman left a comment

Choose a reason for hiding this comment

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

Mostly OK, this exposes issues with stored procedure subqueries generally, when we get to those we'll revert the With node changes here and just do subqueries properly.

@jycor jycor merged commit 267e13c into main Dec 20, 2022
@Hydrocharged Hydrocharged deleted the james/stored branch December 21, 2022 15:40
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.

Analyzer bug when using CTEs in stored procedures
2 participants