You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@druid.apache.org by GitBox <gi...@apache.org> on 2022/09/30 20:11:51 UTC
[GitHub] [druid] paul-rogers commented on a diff in pull request #13071: Redesign QueryContext class
paul-rogers commented on code in PR #13071:
URL: https://github.com/apache/druid/pull/13071#discussion_r984912115
##########
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:
Great suggestion. Done.
##########
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:
Ah, this is a tricky one! So, a recent commit renamed this method. Since it is now deprecated, and presumably used only by old code, I put the name back to what it was before that commit.
##########
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:
Good catch. Removed.
##########
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:
Great question. This is a relatively new method, so my thought is that it is safe to remove. It would be hard to retain as the shape of the returned `QueryContext` class itself is different. By removing this method (and replacing it with `context()`) we create a clear signal to early adopters that the class did change, and can't be used to modify the context any longer. Also, all the methods to get values, which extensions might have called, have also moved.
So, question: is this going to be a problem for extensions? Is there an expectation that old extensions, without recompiling and making a few simple changes) continue to work? If so, then this whole approach is called into question: we'd have to keep the old code and duplicate it in the new version, or just keep the old code and live with the mess. Thoughts?
--
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