clintropolis commented on code in PR #17974:
URL: https://github.com/apache/druid/pull/17974#discussion_r2071982621
##########
sql/src/main/java/org/apache/druid/sql/calcite/planner/PlannerContext.java:
##########
@@ -559,6 +559,14 @@ public void addAllToQueryContext(Map<String, Object> toAdd)
initializeContextFields();
}
+ /**
+ * 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);
+ }
+
Review 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
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
