Akshat-Jain commented on PR #16376:
URL: https://github.com/apache/druid/pull/16376#issuecomment-2093338887
> Looking at this change again, I am still not convinced that this needs to
be done (at the moment anyway), especially now that the other PR is not even
using the queryContextMap anymore.
>
> Also, I am not entirely convinced that returning a mutable map is the
right approach. e.g. the `PlannerContext.queryContextMap()` is being passed
into a bunch of different objects as is, e.g. in
`DruidQuery.toScanAndSortQuery()` where we pass the map into a
`WindowOperatorQuery`. Passing the original mutable map into this class might
have unforeseen side effects if this new object decides to change the contents
of the map.
>
> So, I think it is better to try to fix this once we have a real
requirement so that we know exactly what needs to be done. Just trying to
conform to the javadoc doesn't seem reason enough to me. We might as well just
fix up the javadoc to not say immutable.
@kfaraz This PR is certainly not related to the other PR, like I mentioned
before. The other PR just uncovered this intricacy and ran into issues because
of it.
We've moved away from query context approach in the other PR, so this PR
certainly isn't blocking anything. So I agree that this change isn't really
needed right now.
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. There's a separate method for returning immutable query context
for any use-cases like `WindowOperatorQuery` that you mentioned:
```java
/**
* Return the query context as an immutable object. Use this form
* when querying the context as it provides type-safe accessors.
*/
public QueryContext queryContext()
{
return QueryContext.of(queryContext);
}
```
But since this PR isn't really needed right now, I'll close this PR for now.
--
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]
