kfaraz commented on PR #16376: URL: https://github.com/apache/druid/pull/16376#issuecomment-2094567302
> I raised this PR as I felt that this was just a minor unintentional intricacy left unnoticed, which someone would eventually have to deal with. > Changing the javadoc doesn't seem ideal to me, as it's intended to return a mutable map. I agree, if the mutability of the map is really needed, someone will eventually have to deal with it. But given the current code flow, I am not sure if it is even a requirement. If you are interested in fixing this, try to go through all the usages of `queryContextMap()` and see if the returned map is ever actually being updated. If not, we might as well get rid of this method to avoid confusion later. You could update the changes here itself and reopen this PR. If it turns out from your investigation that we really do need the map to be mutable, then exposing the internal `Map<String, Object>` for any caller to mutate as they see fit is not appropriate. `PlannerContext` itself must expose methods such as `putInQueryContext()` or `removeFromQueryContext()` and thus be aware of any updates made to the internal map. Alternatively, the method `queryContextMap()` could return a `MutableQueryContext` object which itself exposes `put()` and `remove()` methods. Exposing the internal map implementation makes it very difficult to trace back the places which are actually mutating the map. -- 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]
