minor(sqlparser): encapsulate PlanerContext, reduce some clones #5814
Conversation
This file contains 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
alamb
commented
Mar 31, 2023
| self.outer_query_schema.as_ref() | ||
| } | ||
|
|
||
| /// sets the outer query schema, returning the existing one, if |
There was a problem hiding this comment.
I think having accessors also is a good place to put documentation about what they do
| @@ -33,7 +33,7 @@ impl<'a, S: ContextProvider> SqlToRel<'a, S> { | |||
| // normalize name and alias | |||
| let table_ref = self.object_name_to_table_reference(name)?; | |||
| let table_name = table_ref.to_string(); | |||
| let cte = planner_context.ctes.get(&table_name); | |||
| let cte = planner_context.get_cte(&table_name); | |||
There was a problem hiding this comment.
this is the use of ctes -- and 4 lines below it is immediately clone()d
avantgardnerio
approved these changes
Apr 3, 2023
|
Thanks @avantgardnerio ! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Which issue does this PR close?
related to #5637 , but really a Friday afternoon project that has bothered me for a whle
Rationale for this change
The PlannerContext in the SQL planner has a map of
LogicalPlansand it is cloned during planning (which deep clones the values 😱 ). Also, the LogicalPlan is cloned on use anyways.What changes are included in this PR?
PlannerContextnon pubAre these changes tested?
Covered by existing tests
Are there any user-facing changes?
No