paul-rogers opened a new pull request, #13071: URL: https://github.com/apache/druid/pull/13071
Alternative solution for [PR #13022](https://github.com/apache/druid/pull/13022). ### Background Recent work uncovered two issues with the way Druid handles query contexts: * #13049 fixes a race condition with JDBC updates * #13022 fixes type-unsafe value conversions During the investigation of the above it became clear that the semantics around the query context are a bit unclear and could be improved. ### Overview This PR includes those improvements: * The original three-map `QueryContext` is replaced by a "facade" on top of a single map. * The original `QueryContext` was created to allow authorizing user context keys. This PR solves that issue by making a copy of the keys separate from the context, allowing the SQL engine to modify the context without creating conflicts with the security feature. * The uses of `QueryContext` as a holder for a context are removed; replaced with a direct reference to the map itself. * Modification to the context in the planning layer occurs directly on the context map; it is no longer done via the old `QueryContext` class since there is no real value in creating a layer on top of the map. * #13022 adds a number of type-safe method to the `Query` interface. However, doing so clutters up that interface, and we still need the `QueryContexts` methods to get at specific context values. See the discussion in #13022 for details. This PR, by contrast, puts all context access methods on the revised `QueryContext` facade, making for cleaner code. * All type-unsafe context value accesses are replaced by the new `QueryContext` type-safe methods. * The old type-unsafe methods are marked deprecated. No code in Druid-proper references them, though we must assume that extensions may continue to use them. * All specific methods in `QueryContexts` which take a `Query` parameter are replaced by methods on the revised `QueryContext` class. (This that took a context map are unchanged.) ### Specifics The original `QueryContext` class was first removed: all references to that class were modified to refer to the context map instead. A new class of the same name was created. The only creator of the new class is `Query.queryContext()`. The new class holds onto the context map associated with the query, and provides the many type-safe and value-specific methods. Since `QueryContext` is simply a facade, it is cheap to create an instance as needed. This lets us convert code of the form: ```java long foo = QueryContexts.getAsLong(query, FOO_NAME, FOO_DEFAULT); long bar = QueryContexts.getBar(query); ``` To ```java long foo = query.queryContext().getLong(FOO_NAME, FOO_DEFAULT); long bar = query.queryContext().getBar(); ``` When a bit of code makes multiple references to context values (as above), we can also do: ``` QueryContext queryContext = query.queryContext(); long foo = queryContext.getLong(FOO_NAME, FOO_DEFAULT); long bar = queryContext.getBar(); ``` The following methods in `Query` are marked deprecated: ```java interface Query... { @Deprecated <ContextType> ContextType getContextValue(String key); @Deprecated <ContextType> ContextType getContextValue(String key, ContextType defaultValue); @Deprecated default boolean getContextBoolean(String key, boolean defaultValue); ``` Although the above methods are deprecated, they are reimplemented as `default` methods. As a result, the methods of the same names are removed from subclasses since they are redundant. The `Query.getQueryContext()` method is removed as a way of marking that the old `QueryContext` is no longer available. The old method didn't really even work: the different context types were lost when merging into a single map. A new `queryContext()` method returns the facade version. ### Alternatives The discussions in #13049 and #13022 suggested other alternatives: * Work only with the context map and the `QueryContexts` methods as shown above. This works fine, and is one of the steps taken in #13049 that lead to this PR, but is a bit cumbersome. * Modify the old `QueryContext` to be thread-safe as done in the first commit for #13049 . It turns out this is not really needed. The work on this PR revealed that, except for the JDBC issue which Gian identified, the current code does correctly do all context updates in the "main" request thread, and only reads in the worker threads. * Add methods to the `Query` method as in #13022. Doing so clutters the `Query` interface, and does not help in the SQL layer where we work with contexts before the native `Query` is created. Unless we really want to clutter the `Query` interface, we'd still need the value-specific methods in `QueryContexts`. * Leave well enough alone: status quo. While the existing code works, as noted above, the semantics are a bit messy which creates confusion for readers. Tidying up the code has no benefit to the computer, but is helpful for us poor humans who have to understand the code. ### Conflicts This PR will conflict with #13049 and #13022. That's find: the other two solve short-term problems; this one is a bit cleanup. Let's get the other two in first, then we'll rebase and adjust this one as needed. ### Conclusion The result is a solution that: * Solves the authorization issue that gave rise to the original `QueryContext` * Solves the synchronization issue with `QueryContext` by avoiding multiple maps. * Solves the type safety issue of PR #13022 by converting `QueryContext` to be a read-only facade. <hr> This PR has: - [X] been self-reviewed. - [ ] been tested in a test Druid cluster. -- 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]
