Akshat-Jain commented on PR #16376: URL: https://github.com/apache/druid/pull/16376#issuecomment-2091270242
@kfaraz Thanks for the review! Answering all questions below: > Why is the change needed? The contract for `PlannerContext#queryContextMap` is misleading. It says that it returns a mutable map, but using Collections.emptyMap() for creating the planner ends up violating that. So either the contract should be re-defined, or we shouldn't use immutable map for creating the planner. > Without the change, why kind of use cases fail? In my PR for loading lookups selectively for MSQ tasks, I had added `plannerContext.queryContextMap().putIfAbsent(PlannerContext.CTX_LOOKUPS_TO_LOAD, new HashSet<>());` in `SqlResourceCollectorShuttle#visit` [here](https://github.com/apache/druid/pull/16358/files#diff-630eb92856c1b8dd3034980b486124e31952c8eef0a9195bbacb9bbe310a3023R84) since the expectation is that `plannerContext.queryContextMap()` returns a mutable map. But this ended up failing 500+ tests with `Unsupported operation` errors since those tests were going through the immutable map code flow. So the use-cases that would fail are trying to modify the query context map using `plannerContext.queryContextMap()` in the code flows where PlannerContext's queryContextMap has been initialized as an immutable map. > How have things been working so far? I guess there wasn't a use-case so far, or we worked around this intricacy somehow? I'm not entirely sure. And I fully agree that if it weren't for my other PR, this change isn't really "needed". It's just that I spent time figuring this out, and would like to prevent the same for any future dev. > and also add a test to verify the same. Any suggestions on what test can I add for this? I personally think my other PR goes through this code flow, so it would just get tested there. I'm not sure if a test to validate the mutability of a param is needed, or adds much value? Happy to know your thoughts on this though! > could you please include provide more context in the description? Sure, I'll add some of the details from this answer to the PR description. -- 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]
