[GitHub] [druid] abhishekagarwal87 commented on a diff in pull request #13071: Redesign QueryContext class

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

abhishekagarwal87 commented on code in PR #13071:
URL: https://github.com/apache/druid/pull/13071#discussion_r983450945


##########
processing/src/main/java/org/apache/druid/query/Query.java:
##########
@@ -96,64 +97,60 @@
   DateTimeZone getTimezone();
 
   /**
-   * Use {@link #getQueryContext()} instead.
+   * Returns the context as an (immutable) map.
    */
-  @Deprecated
   Map<String, Object> getContext();
 
   /**
-   * Returns QueryContext for this query. This type distinguishes between user provided, system default, and system
-   * generated query context keys so that authorization may be employed directly against the user supplied context
-   * values.
-   *
-   * This method is marked @Nullable, but is only so for backwards compatibility with Druid versions older than 0.23.
-   * Callers should check if the result of this method is null, and if so, they are dealing with a legacy query
-   * implementation, and should fall back to using {@link #getContext()} and {@link #withOverriddenContext(Map)} to
-   * manipulate the query context.
-   *
-   * Note for query context serialization and deserialization.
-   * Currently, once a query is serialized, its queryContext can be different from the original queryContext
-   * after the query is deserialized back. If the queryContext has any {@link QueryContext#defaultParams} or
-   * {@link QueryContext#systemParams} in it, those will be found in {@link QueryContext#userParams}
-   * after it is deserialized. This is because {@link BaseQuery#getContext()} uses
-   * {@link QueryContext#getMergedParams()} for serialization, and queries accept a map for deserialization.
+   * Returns the query context as a {@link QueryContext}, which provides
+   * convenience methods for accessing typed context values. The returned
+   * instance is a view on top of the context provided by {@link #getContext()}.
    */
-  @Nullable
-  default QueryContext getQueryContext()
+  default QueryContext context()
   {
-    return null;
+    return QueryContext.of(getContext());
   }
 
   /**
    * Get context value and cast to ContextType in an unsafe way.
    *
-   * For safe conversion, it's recommended to use following methods instead
+   * For safe conversion, it's recommended to use following methods instead:
+   * <p>
+   * {@link QueryContext#getBoolean(String)} <br/>
+   * {@link QueryContext#getString(String)} <br/>
+   * {@link QueryContext#getInt(String)} <br/>
+   * {@link QueryContext#getLong(String)} <br/>
+   * {@link QueryContext#getFloat(String)} <br/>
+   * {@link QueryContext#getEnum(String, Class, Enum)} <br/>
+   * {@link QueryContext#getHumanReadableBytes(String, HumanReadableBytes)}
    *
-   * {@link QueryContext#getAsBoolean(String)}
-   * {@link QueryContext#getAsString(String)}
-   * {@link QueryContext#getAsInt(String)}
-   * {@link QueryContext#getAsLong(String)}
-   * {@link QueryContext#getAsFloat(String, float)}
-   * {@link QueryContext#getAsEnum(String, Class, Enum)}
-   * {@link QueryContext#getAsHumanReadableBytes(String, HumanReadableBytes)}
+   * @deprecated use {@code queryContext().get<Type>()} instead
    */
+  @Deprecated
+  @SuppressWarnings("unchecked")
   @Nullable
   default <ContextType> ContextType getContextValue(String key)
   {
-    if (getQueryContext() == null) {
-      return null;
-    } else {
-      return (ContextType) getQueryContext().get(key);
-    }
+    return (ContextType) context().get(key);
   }
 
+  /**
+   * @deprecated use {@code queryContext().get<Type>(defaultValue)} instead
+   */
+  @SuppressWarnings("unchecked")
+  @Deprecated
+  default <ContextType> ContextType getContextValue(String key, ContextType defaultValue)

Review Comment:
   did this method exist before? 



##########
processing/src/main/java/org/apache/druid/query/Query.java:
##########
@@ -96,64 +97,60 @@
   DateTimeZone getTimezone();
 
   /**
-   * Use {@link #getQueryContext()} instead.
+   * Returns the context as an (immutable) map.
    */
-  @Deprecated
   Map<String, Object> getContext();
 
   /**
-   * Returns QueryContext for this query. This type distinguishes between user provided, system default, and system
-   * generated query context keys so that authorization may be employed directly against the user supplied context
-   * values.
-   *
-   * This method is marked @Nullable, but is only so for backwards compatibility with Druid versions older than 0.23.
-   * Callers should check if the result of this method is null, and if so, they are dealing with a legacy query
-   * implementation, and should fall back to using {@link #getContext()} and {@link #withOverriddenContext(Map)} to
-   * manipulate the query context.
-   *
-   * Note for query context serialization and deserialization.
-   * Currently, once a query is serialized, its queryContext can be different from the original queryContext
-   * after the query is deserialized back. If the queryContext has any {@link QueryContext#defaultParams} or
-   * {@link QueryContext#systemParams} in it, those will be found in {@link QueryContext#userParams}
-   * after it is deserialized. This is because {@link BaseQuery#getContext()} uses
-   * {@link QueryContext#getMergedParams()} for serialization, and queries accept a map for deserialization.
+   * Returns the query context as a {@link QueryContext}, which provides
+   * convenience methods for accessing typed context values. The returned
+   * instance is a view on top of the context provided by {@link #getContext()}.
    */
-  @Nullable
-  default QueryContext getQueryContext()

Review Comment:
   `getQueryContext` seems to be removed entirely. would it not break extensions? 



##########
processing/src/main/java/org/apache/druid/query/Query.java:
##########
@@ -164,14 +161,12 @@ default boolean getContextBoolean(String key, boolean defaultValue)
    * @param key          The context key value being looked up
    * @param defaultValue The default to return if the key value doesn't exist or the context is null.
    * @return {@link HumanReadableBytes}
+   * @deprecated use {@code queryContext().getContextHumanReadableBytes()} instead.
    */
-  default HumanReadableBytes getContextAsHumanReadableBytes(String key, HumanReadableBytes defaultValue)
+  @Deprecated
+  default HumanReadableBytes getContextHumanReadableBytes(String key, HumanReadableBytes defaultValue)

Review Comment:
   I might be missing something. This method is renamed apart from being deprecated. How would we ensure backward compatibility? `As` is removed from method name. 



##########
processing/src/main/java/org/apache/druid/query/QueryContext.java:
##########
@@ -20,236 +20,540 @@
 package org.apache.druid.query;
 
 import org.apache.druid.java.util.common.HumanReadableBytes;
+import org.apache.druid.java.util.common.StringUtils;
+import org.apache.druid.java.util.common.granularity.Granularity;
+import org.apache.druid.query.QueryContexts.Vectorize;
+import org.apache.druid.segment.QueryableIndexStorageAdapter;
 
 import javax.annotation.Nullable;
 
 import java.util.Collections;
 import java.util.Map;
 import java.util.Objects;
-import java.util.TreeMap;
 
 /**
- * Holder for query context parameters. There are 3 ways to set context params today.
- *
- * - Default parameters. These are set mostly via {@link DefaultQueryConfig#context}.
- *   Auto-generated queryId or sqlQueryId are also set as default parameters. These default parameters can
- *   be overridden by user or system parameters.
- * - User parameters. These are the params set by the user. User params override default parameters but
- *   are overridden by system parameters.
- * - System parameters. These are the params set by the Druid query engine for internal use only.
- *
- * You can use {@code getX} methods or {@link #getMergedParams()} to compute the context params
- * merging 3 types of params above.
- *
- * Currently, this class is mainly used for query context parameter authorization,
- * such as HTTP query endpoints or JDBC endpoint. Its usage can be expanded in the future if we
- * want to track user parameters and separate them from others during query processing.
+ * Immutable holder for query context parameters with typed access methods.
  */
 public class QueryContext
 {
-  private final Map<String, Object> defaultParams;
-  private final Map<String, Object> userParams;
-  private final Map<String, Object> systemParams;
+  private static final QueryContext EMPTY = new QueryContext(null);
+
+  private final Map<String, Object> context;
+
+  public QueryContext(Map<String, Object> context)
+  {
+    this.context = context == null ? Collections.emptyMap() : Collections.unmodifiableMap(context);
+  }
+
+  public static QueryContext empty()
+  {
+    return EMPTY;
+  }
+
+  public static QueryContext of(Map<String, Object> context)
+  {
+    return new QueryContext(context == null ? Collections.emptyMap() : context);
+  }
+
+  public boolean isEmpty()
+  {
+    return context.isEmpty();
+  }
+
+  public Map<String, Object> getContext()

Review Comment:
   if its a new function, can we call it `asMap`



##########
server/src/main/java/org/apache/druid/server/QueryLifecycle.java:
##########
@@ -102,6 +103,8 @@
 
   @MonotonicNonNull
   private Query<?> baseQuery;
+  @MonotonicNonNull
+  private Map<String, Object> userContext;

Review Comment:
   can we store the keys themselves instead of storing 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: 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