Make EXPLAIN PLAN queries honor the query context set via SET statements#17974
Make EXPLAIN PLAN queries honor the query context set via SET statements#17974abhishekrb19 merged 3 commits intomasterfrom
EXPLAIN PLAN queries honor the query context set via SET statements#17974Conversation
- EXPLAIN PLAN queries were ignoring the query context parameters from the SET statement. - This was because the planner config was eagerly built before the set statement list from the sql query is processed and the query context is extracted from it. - This patch now overrides the planner config in processStatementList. In order to do this override, the field is made non-final (similar to other non-final mutable members in PlannerContext).
clintropolis
left a comment
clintropolis
left a comment
There was a problem hiding this comment.
good find, thanks for fixing 👍
| /** | ||
| * Add additional query context parameters to planner config, overriding any existing values. | ||
| */ | ||
| public void addAllToPlannerConfig(Map<String, Object> toAdd) | ||
| { | ||
| this.plannerConfig = this.plannerConfig.withOverrides(toAdd); | ||
| } | ||
|
|
There was a problem hiding this comment.
How about just adding this logic to initializeContextFields or addAllToQueryContext instead of a new method? It seems like callers would always want to call both methods, so I don't see a good reason to keep them separate since it would make it easier to make mistakes.
To go with that, I think we could also adjust the PlannerContext constructor to no longer take the PlannerConfig argument since we can get it from the PlannerToolbox, and we don't need the Preconditions.checkNotNull that is currently in the constructor because PlannerToolbox also has that check, making the one in this constructor redundant. This would mean that PlannerContext.create would no longer need to also be calling withOverrides on the context before passing to the constructor
There was a problem hiding this comment.
Yeah, makes sense to add it to initializeContextFields. I've also renamed this private method to initializeContextFieldsAndPlannerConfig
|
Thanks for the quick review, @clintropolis |
3 participants
This is a follow-up patch to #17894 that adds support for honoring the query context for
EXPLAIN PLANqueries viaSETstatements.Description:
EXPLAIN PLANqueries were ignoring the query context parameters fromSETstatements.SETstatements from the SQL query were processed and the query context extracted inprocessStatementList.processStatementListoverrides the planner config in the end. In order to do this override, the field is made non-final (similar to other non-final mutable members inPlannerContext).This PR has: