[GitHub] [druid] paul-rogers commented on pull request #13022: Avoid ClassCastException when getting values from `QueryContext`

[GitBox <gi...@apache.org>]

paul-rogers commented on PR #13022:
URL: https://github.com/apache/druid/pull/13022#issuecomment-1242722294

   @FrankChen021, as it turns out, the `QueryContext` class introduced several problems. [PR 13049](https://github.com/apache/druid/pull/13049) proposes to rip out `QueryContext` and go back to using a map (along with a few adjustments to allow authorizing the context.) That PR will, unfortunately, conflict with this one.
   
   While using maps solves some problems, it requires awkward code such as:
   
   ```java
   x = QueryContexts.getSomething(context, key, default)
   ```
   
   One advantage of the old `QueryContext` was that we could do:
   
   ```java
   x = queryContext.getSomething(key, default);
   ```
   
   Here, you propose a solution: add those values to the query class. That's OK, but it makes the query class pretty complex: we have methods for many tasks, and now we add multiple methods for context values. Plus, there are places (such as the SQL layer) where we work with the context without also having a native query. So, we'd have to duplicate these methods in several places. This suggests that methods to work with the context belong on the context itself, not on the users of the context (such as `Query`).
   
   One solution, which I didn't include in PR 13049, is to define a new class: a typed wrapper on top of the map. Something like:
   
   ```java
   class QueryContext {
     Object get(String key);
     String getString(String key);
     String getString(String key, String defaultValue);
     int getInt(String key);
     int getInt(String key, int defaultValue);
     ...
   }
   ```
   
   For each of our supported types: String, Int, Long, etc. We'd then store an instance of `QueryContext` where we currently store the map.
   
   `QueryContexts` has a zillion `getSomething()` methods for the various parameters. The logical step would be to move those into the above `QueryContext` class, else we'll find ourselves either duplicating code, or still using `QueryContexts` methods as shown earlier.
   
   The problem that PR 13049 tries to solve is that there are times when the context is immutable, other times when it is mutable. And, those times trade off: the user's request is immutable, a mutable copy is made while planning the query, then the context must become immutable again once we start executing in multiple threads. So, we could have two forms: the above immutable form with only "getters", and a mutable form:
   
   ```java
   class MutableQueryContext {
     Object put(String key, Object value);
     Object remove(String key);
     ...
   }
   ```
   
   To do this, we'd actually want `QueryContext` to be an interface, with two concrete implementations: `ImmutableQueryContext` and `MutableQueryContext`. One reason for the two classes is that the classes will clearly label the intent of when when the context is mutable or not.
   
   This may still lead to a muddle, however. A `Query` will have a mutable context during the "planning" (`QueryRunner`-based) part of the native query execution, but immutable once we move to the execution (`Sequence`-based) part of the process.
   
   There is also the issue of compatibility with custom `Query` classes where folks have implemented the existing interfaces using a map. It is not entirely clear how often that is done and what we can change in this area. Any ideas?


-- 
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: commits-unsubscribe@druid.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org