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/11 20:49:44 UTC
[GitHub] [druid] paul-rogers opened a new pull request, #13071: Redesign QueryContext class
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: 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
[GitHub] [druid] abhishekagarwal87 commented on a diff in pull request #13071: Redesign QueryContext class
Posted by GitBox <gi...@apache.org>.
abhishekagarwal87 commented on code in PR #13071:
URL: https://github.com/apache/druid/pull/13071#discussion_r992243426
##########
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:
I think it's fine. I am just going to tag the PR "Backward incompatible"
##########
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:
Please add a note in your PR that can be used in release notes later.
--
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
[GitHub] [druid] abhishekagarwal87 commented on pull request #13071: Redesign QueryContext class
Posted by GitBox <gi...@apache.org>.
abhishekagarwal87 commented on PR #13071:
URL: https://github.com/apache/druid/pull/13071#issuecomment-1279666702
Thank you @paul-rogers. We can make the changes I suggested in a follow-up PR.
--
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
[GitHub] [druid] FrankChen021 commented on pull request #13071: Redesign QueryContext class
Posted by GitBox <gi...@apache.org>.
FrankChen021 commented on PR #13071:
URL: https://github.com/apache/druid/pull/13071#issuecomment-1243145157
I agree with your proposal on the new API form as
```java
long foo = query.queryContext().getLong(FOO_NAME, FOO_DEFAULT);
long bar = query.queryContext().getBar();
```
It makes the APIs much simpler and cleaner.
--
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
[GitHub] [druid] paul-rogers commented on pull request #13071: Redesign QueryContext class
Posted by GitBox <gi...@apache.org>.
paul-rogers commented on PR #13071:
URL: https://github.com/apache/druid/pull/13071#issuecomment-1276570480
@FrankChen021, thanks for the approval!
So, it turns out that I had to pull the work for [Issue #13120](https://github.com/apache/druid/issues/13120) into this PR in order to get the security IT to pass. Basically, that IT does context key security checks using JDBC. JDBC inserts a key, which fails the security check. This worked previously when we separated system and user keys. (In fact, this may be the reason that the prior PR did the split.)
This latest PR adds two config values, defined in the "release notes" section in the revised description. To make this work:
* The `AuthConfig` is not the one-stop shop to prepare the list of context keys to use for authorization checks.
* Three new set operations are added to `CollectionUtils` because Java, in its infinite wisdom, doesn't provide them.
* Two context keys moved from the SQL package to `QueryContexts` so that they are visible to `AuthConfig`.
* These two keys are "out-of-the-box freebies" for context security: they are always allowed because they are set by Druid itself in either the Router (query ID) or JDBC path (stringify arrays.)
* The code in the planner to gather the resources is tidied up a bit.
If we're good with the proposed changes, perhaps we can wrap up this PR and get it merged.
--
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
[GitHub] [druid] FrankChen021 commented on a diff in pull request #13071: Redesign QueryContext class
Posted by GitBox <gi...@apache.org>.
FrankChen021 commented on code in PR #13071:
URL: https://github.com/apache/druid/pull/13071#discussion_r990609600
##########
sql/src/main/java/org/apache/druid/sql/AbstractStatement.java:
##########
@@ -49,6 +57,14 @@ public abstract class AbstractStatement implements Closeable
protected final SqlToolbox sqlToolbox;
protected final SqlQueryPlus queryPlus;
protected final SqlExecutionReporter reporter;
+
+ /**
+ * Copy of the query context modified during planning. Modifications are
+ * valid in tasks that run in the request thread. Once the query forks
+ * child threads, then concurrent modifications to the query context will
+ * result in an undefined muddle of race conditions.
+ */
+ protected final Map<String, Object> queryContext;
Review Comment:
So this is mutable?
--
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
[GitHub] [druid] FrankChen021 commented on a diff in pull request #13071: Redesign QueryContext class
Posted by GitBox <gi...@apache.org>.
FrankChen021 commented on code in PR #13071:
URL: https://github.com/apache/druid/pull/13071#discussion_r990610166
##########
sql/src/main/java/org/apache/druid/sql/avatica/DruidConnection.java:
##########
@@ -127,7 +126,7 @@ public DruidJdbcPreparedStatement createPreparedStatement(
@SuppressWarnings("GuardedBy")
final PreparedStatement statement = sqlStatementFactory.preparedStatement(
- sqlQueryPlus.withContext(context.copy())
+ sqlQueryPlus.withContext(new HashMap<String, Object>(context))
Review Comment:
No need to make a copy because the ctor of `SqlQueryPlus` always make a copy.
--
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
[GitHub] [druid] abhishekagarwal87 commented on pull request #13071: Redesign QueryContext class
Posted by GitBox <gi...@apache.org>.
abhishekagarwal87 commented on PR #13071:
URL: https://github.com/apache/druid/pull/13071#issuecomment-1277051170
It will also make life easier for cluster admin to have a default list of unsecured context variables.
--
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
[GitHub] [druid] FrankChen021 commented on a diff in pull request #13071: Redesign QueryContext class
Posted by GitBox <gi...@apache.org>.
FrankChen021 commented on code in PR #13071:
URL: https://github.com/apache/druid/pull/13071#discussion_r990609668
##########
sql/src/main/java/org/apache/druid/sql/SqlQueryPlus.java:
##########
@@ -39,25 +39,31 @@
* SQL requests come from a variety of sources in a variety of formats. Use
* the {@link Builder} class to create an instance from the information
* available at each point in the code.
+ * <p>
+ * The query context has a complex lifecycle. The copy here should remain
+ * unchanged: this is the set of values which the user requested. Planning will
+ * add (and sometimes remove) values: that work should be done on a copy of the
+ * context so that we have a clean record of the user's original requested
+ * values.
*/
public class SqlQueryPlus
{
private final String sql;
- private final QueryContext queryContext;
+ private final Map<String, Object> queryContext;
Review Comment:
And this is immutable?
--
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
[GitHub] [druid] abhishekagarwal87 commented on a diff in pull request #13071: Redesign QueryContext class
Posted by 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
[GitHub] [druid] FrankChen021 commented on a diff in pull request #13071: Redesign QueryContext class
Posted by GitBox <gi...@apache.org>.
FrankChen021 commented on code in PR #13071:
URL: https://github.com/apache/druid/pull/13071#discussion_r985034097
##########
processing/src/main/java/org/apache/druid/query/scan/ScanQueryEngine.java:
##########
@@ -46,6 +45,7 @@
import org.joda.time.Interval;
import javax.annotation.Nullable;
+
Review Comment:
```suggestion
```
##########
server/src/main/java/org/apache/druid/server/QueryResource.java:
##########
@@ -386,14 +386,7 @@ private Query<?> readQuery(
String prevEtag = getPreviousEtag(req);
if (prevEtag != null) {
- if (baseQuery.getQueryContext() == null) {
- QueryContext context = new QueryContext(baseQuery.getContext());
- context.addSystemParam(HEADER_IF_NONE_MATCH, prevEtag);
-
- return baseQuery.withOverriddenContext(context.getMergedParams());
- } else {
- baseQuery.getQueryContext().addSystemParam(HEADER_IF_NONE_MATCH, prevEtag);
- }
+ baseQuery.getContext().put(HEADER_IF_NONE_MATCH, prevEtag);
Review Comment:
`getContext()` returns an immutable map according to current implementation, a call of `put` will result in exception
##########
processing/src/main/java/org/apache/druid/query/select/SelectQuery.java:
##########
@@ -34,6 +33,7 @@
import org.joda.time.Interval;
import javax.annotation.Nullable;
+
Review Comment:
```suggestion
```
##########
processing/src/main/java/org/apache/druid/query/search/SearchQueryQueryToolChest.java:
##########
@@ -46,6 +45,7 @@
import org.apache.druid.query.dimension.DimensionSpec;
import javax.annotation.Nullable;
+
Review Comment:
```suggestion
```
##########
processing/src/main/java/org/apache/druid/segment/VirtualColumns.java:
##########
@@ -48,6 +47,7 @@
import org.apache.druid.segment.virtual.VirtualizedColumnSelectorFactory;
import javax.annotation.Nullable;
+
Review Comment:
```suggestion
```
##########
server/src/main/java/org/apache/druid/client/DirectDruidClient.java:
##########
@@ -152,21 +153,23 @@ public Sequence<T> run(final QueryPlus<T> queryPlus, final ResponseContext conte
{
final Query<T> query = queryPlus.getQuery();
QueryToolChest<T, Query<T>> toolChest = warehouse.getToolChest(query);
- boolean isBySegment = QueryContexts.isBySegment(query);
+ boolean isBySegment = query.context().isBySegment();
final JavaType queryResultType = isBySegment ? toolChest.getBySegmentResultType() : toolChest.getBaseResultType();
final ListenableFuture<InputStream> future;
final String url = scheme + "://" + host + "/druid/v2/";
final String cancelUrl = url + query.getId();
try {
- log.debug("Querying queryId[%s] url[%s]", query.getId(), url);
+ log.debug("Querying queryId [%s] url [%s]", query.getId(), url);
final long requestStartTimeNs = System.nanoTime();
- final long timeoutAt = query.getQueryContext().getAsLong(QUERY_FAIL_TIME);
- final long maxScatterGatherBytes = QueryContexts.getMaxScatterGatherBytes(query);
+ final QueryContext queryContext = query.context();
+ // Will NPE if the value is not set.
Review Comment:
What does this mean? Where will the NPE be thrown?
##########
server/src/main/java/org/apache/druid/server/ClientQuerySegmentWalker.java:
##########
@@ -60,6 +59,7 @@
import org.joda.time.Interval;
import javax.annotation.Nullable;
+
Review Comment:
```suggestion
```
##########
server/src/main/java/org/apache/druid/server/QueryResource.java:
##########
@@ -78,6 +77,7 @@
import javax.ws.rs.core.Response;
import javax.ws.rs.core.Response.Status;
import javax.ws.rs.core.StreamingOutput;
+
Review Comment:
```suggestion
```
##########
processing/src/main/java/org/apache/druid/query/timeseries/TimeseriesQueryEngine.java:
##########
@@ -49,6 +48,7 @@
import org.joda.time.Interval;
import javax.annotation.Nullable;
+
Review Comment:
```suggestion
```
##########
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()
+ {
+ return context;
+ }
/**
- * Cache of params merged.
+ * Check if the given key is set. If the client will then fetch the value,
+ * consider using one of the {@code get<Type>(String key)} methods instead:
+ * they each return {@code null} if the value is not set.
+ */
+ public boolean containsKey(String key)
+ {
+ return context.containsKey(key);
+ }
+
+ /**
+ * Return a value as a generic {@code Object}, returning {@code null} if the
+ * context value is not set.
*/
@Nullable
- private Map<String, Object> mergedParams;
+ public Object get(String key)
+ {
+ return context.get(key);
+ }
- public QueryContext()
+ /**
+ * Return a value as a generic {@code Object}, returning the default value if the
+ * context value is not set.
+ */
+ public Object get(String key, Object defaultValue)
{
- this(null);
+ final Object val = get(key);
+ return val == null ? defaultValue : val;
}
- public QueryContext(@Nullable Map<String, Object> userParams)
+ /**
+ * Return a value as an {@code String}, returning {@link null} if the
+ * context value is not set.
+ *
+ * @throws BadQueryContextException for an invalid value
+ */
+ @Nullable
+ public String getString(String key)
{
- this(
- new TreeMap<>(),
- userParams == null ? new TreeMap<>() : new TreeMap<>(userParams),
- new TreeMap<>()
- );
+ return getString(key, null);
}
- private QueryContext(
- final Map<String, Object> defaultParams,
- final Map<String, Object> userParams,
- final Map<String, Object> systemParams
- )
+ public String getString(String key, String defaultValue)
{
- this.defaultParams = defaultParams;
- this.userParams = userParams;
- this.systemParams = systemParams;
- this.mergedParams = null;
+ return QueryContexts.parseString(context, key, defaultValue);
}
- private void invalidateMergedParams()
+ /**
+ * Return a value as an {@code Boolean}, returning {@link null} if the
+ * context value is not set.
+ *
+ * @throws BadQueryContextException for an invalid value
+ */
+ public Boolean getBoolean(final String key)
{
- this.mergedParams = null;
+ return QueryContexts.getAsBoolean(key, get(key));
}
- public boolean isEmpty()
+ /**
+ * Return a value as an {@code boolean}, returning the default value if the
+ * context value is not set.
+ *
+ * @throws BadQueryContextException for an invalid value
+ */
+ public boolean getBoolean(final String key, final boolean defaultValue)
{
- return defaultParams.isEmpty() && userParams.isEmpty() && systemParams.isEmpty();
+ return QueryContexts.parseBoolean(context, key, defaultValue);
}
- public void addDefaultParam(String key, Object val)
+ /**
+ * Return a value as an {@code Integer}, returning {@link null} if the
+ * context value is not set.
+ *
+ * @throws BadQueryContextException for an invalid value
+ */
+ public Integer getInt(final String key)
+ {
+ return QueryContexts.getAsInt(key, get(key));
+ }
+
+ /**
+ * Return a value as an {@code int}, returning the default value if the
+ * context value is not set.
+ *
+ * @throws BadQueryContextException for an invalid value
+ */
+ public int getInt(final String key, final int defaultValue)
{
- invalidateMergedParams();
- defaultParams.put(key, val);
+ return QueryContexts.parseInt(context, key, defaultValue);
}
- public void addDefaultParams(Map<String, Object> defaultParams)
+ /**
+ * Return a value as an {@code Long}, returning {@link null} if the
+ * context value is not set.
+ *
+ * @throws BadQueryContextException for an invalid value
+ */
+ public Long getLong(final String key)
{
- invalidateMergedParams();
- this.defaultParams.putAll(defaultParams);
+ return QueryContexts.getAsLong(key, get(key));
}
- public void addSystemParam(String key, Object val)
+ /**
+ * Return a value as an {@code long}, returning the default value if the
+ * context value is not set.
+ *
+ * @throws BadQueryContextException for an invalid value
+ */
+ public long getLong(final String key, final long defaultValue)
{
- invalidateMergedParams();
- this.systemParams.put(key, val);
+ return QueryContexts.parseLong(context, key, defaultValue);
}
- public Object removeUserParam(String key)
+ /**
+ * Return a value as an {@code Float}, returning {@link null} if the
+ * context value is not set.
+ *
+ * @throws BadQueryContextException for an invalid value
+ */
+ @SuppressWarnings("unused")
+ public Float getFloat(final String key)
{
- invalidateMergedParams();
- return userParams.remove(key);
+ return QueryContexts.getAsFloat(key, get(key));
}
/**
- * Returns only the context parameters the user sets.
- * The returned map does not include the parameters that have been removed via {@link #removeUserParam}.
+ * Return a value as an {@code long}, returning the default value if the
Review Comment:
```suggestion
* Return a value as an {@code float}, returning the default value if the
```
##########
server/src/main/java/org/apache/druid/server/QueryLifecycle.java:
##########
@@ -61,7 +62,9 @@
import javax.annotation.Nullable;
import javax.servlet.http.HttpServletRequest;
+
Review Comment:
```suggestion
```
##########
sql/src/main/java/org/apache/druid/sql/calcite/planner/QueryHandler.java:
##########
@@ -537,8 +536,7 @@ protected PlannerResult planWithDruidConvention() throws ValidationException
@Nullable
private RelRoot possiblyWrapRootWithOuterLimitFromContext(RelRoot root)
{
- Object outerLimitObj = handlerContext.queryContext().get(PlannerContext.CTX_SQL_OUTER_LIMIT);
- Long outerLimit = DimensionHandlerUtils.convertObjectToLong(outerLimitObj, true);
+ Long outerLimit = handlerContext.queryContext().getLong(PlannerContext.CTX_SQL_OUTER_LIMIT);
Review Comment:
Original code calls `DimensionHandlerUtils.convertObjectToLong` to convert an object to Long. When the input is type of String, `DimensionHandlerUtils.getExactLongFromDecimalString` is called to parse the string. This differs from `queryContext().getLong` which calls `Numbers.parseLong`.
I'm not sure if there's parse difference between these two parse method.
--
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
[GitHub] [druid] abhishekagarwal87 commented on a diff in pull request #13071: Redesign QueryContext class
Posted by GitBox <gi...@apache.org>.
abhishekagarwal87 commented on code in PR #13071:
URL: https://github.com/apache/druid/pull/13071#discussion_r994148369
##########
server/src/main/java/org/apache/druid/server/security/AuthConfig.java:
##########
@@ -82,6 +81,44 @@ public AuthConfig(
@JsonProperty
private final boolean authorizeQueryContextParams;
+ /**
+ * The set of query context keys that are allowed, even when security is
+ * enabled. A null value is the same as an empty set.
+ */
+ @JsonProperty
+ private final Set<String> unsecuredContextKeys;
+
+ /**
+ * The set of query context keys to secure, when context security is
+ * enabled. Null has a special meaning: it means to ignore this set.
+ * Else, only the keys in this set are subject to security. If set,
+ * the unsecured list is ignored.
+ */
+ @JsonProperty
+ private final Set<String> securedContextKeys;
+
+ @JsonCreator
+ public AuthConfig(
+ @JsonProperty("authenticatorChain") List<String> authenticatorChain,
+ @JsonProperty("authorizers") List<String> authorizers,
+ @JsonProperty("unsecuredPaths") List<String> unsecuredPaths,
+ @JsonProperty("allowUnauthenticatedHttpOptions") boolean allowUnauthenticatedHttpOptions,
+ @JsonProperty("authorizeQueryContextParams") boolean authorizeQueryContextParams,
+ @JsonProperty("unsecuredContextKeys") Set<String> unsecuredContextKeys,
+ @JsonProperty("securedContextKeys") Set<String> securedContextKeys
+ )
+ {
+ this.authenticatorChain = authenticatorChain;
+ this.authorizers = authorizers;
+ this.unsecuredPaths = unsecuredPaths == null ? Collections.emptyList() : unsecuredPaths;
+ this.allowUnauthenticatedHttpOptions = allowUnauthenticatedHttpOptions;
+ this.authorizeQueryContextParams = authorizeQueryContextParams;
+ this.unsecuredContextKeys = unsecuredContextKeys == null
+ ? Collections.emptySet()
+ : unsecuredContextKeys;
+ this.securedContextKeys = securedContextKeys;
Review Comment:
should this be initialized as empty map like unsecuredContextKeys?
--
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
[GitHub] [druid] FrankChen021 commented on pull request #13071: Redesign QueryContext class
Posted by GitBox <gi...@apache.org>.
FrankChen021 commented on PR #13071:
URL: https://github.com/apache/druid/pull/13071#issuecomment-1261641343
> @FrankChen021, thanks for the encouraging comment! Since this PR builds on the work you recently did, can you perhaps give it a review? This one touches many files and so is at risk of conflicts, so we might want to get it in if things look OK. Thanks!
Sure. I will review it on this weekends.
--
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
[GitHub] [druid] FrankChen021 commented on a diff in pull request #13071: Redesign QueryContext class
Posted by GitBox <gi...@apache.org>.
FrankChen021 commented on code in PR #13071:
URL: https://github.com/apache/druid/pull/13071#discussion_r990611048
##########
extensions-core/multi-stage-query/src/test/java/org/apache/druid/msq/sql/MSQModeTest.java:
##########
@@ -22,34 +22,36 @@
import com.google.common.collect.ImmutableMap;
import org.apache.druid.java.util.common.ISE;
import org.apache.druid.msq.indexing.error.MSQWarnings;
-import org.apache.druid.query.QueryContext;
import org.junit.Assert;
import org.junit.Test;
+import java.util.Collections;
+import java.util.Map;
+
public class MSQModeTest
{
@Test
public void testPopulateQueryContextWhenNoSupercedingValuePresent()
{
- QueryContext originalQueryContext = new QueryContext();
+ Map<String, Object> originalQueryContext = Collections.emptyMap();
Review Comment:
```suggestion
Map<String, Object> originalQueryContext = new HashMap();
```
Assigned with `Collections.emptyMap` causes the UT failed.
--
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
[GitHub] [druid] abhishekagarwal87 merged pull request #13071: Redesign QueryContext class
Posted by GitBox <gi...@apache.org>.
abhishekagarwal87 merged PR #13071:
URL: https://github.com/apache/druid/pull/13071
--
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
[GitHub] [druid] paul-rogers commented on a diff in pull request #13071: Redesign QueryContext class
Posted by GitBox <gi...@apache.org>.
paul-rogers commented on code in PR #13071:
URL: https://github.com/apache/druid/pull/13071#discussion_r991586283
##########
sql/src/main/java/org/apache/druid/sql/AbstractStatement.java:
##########
@@ -49,6 +57,14 @@ public abstract class AbstractStatement implements Closeable
protected final SqlToolbox sqlToolbox;
protected final SqlQueryPlus queryPlus;
protected final SqlExecutionReporter reporter;
+
+ /**
+ * Copy of the query context modified during planning. Modifications are
+ * valid in tasks that run in the request thread. Once the query forks
+ * child threads, then concurrent modifications to the query context will
+ * result in an undefined muddle of race conditions.
+ */
+ protected final Map<String, Object> queryContext;
Review Comment:
Yes. Revised the comment to make the semantics a bit clearer. See if the revised text helps.
##########
processing/src/main/java/org/apache/druid/query/QueryContext.java:
##########
@@ -29,227 +33,545 @@
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.
+ * Code builds up a map of context values from serialization or during
+ * planning. Once that map is handed to the {@code QueryContext}, that map
+ * is effectively immutable.
+ * <p>
+ * The implementation uses a {@link TreeMap} so that the serialized form of a query
+ * lists context values in a deterministic order. Jackson will call
+ * {@code getContext()} on the query, which will call {@link #asMap()} here,
+ * which returns the sorted {@code TreeMap}.
+ * <p>
+ * The {@code TreeMap} is a mutable class. We'd prefer an immutable class, but
+ * we can choose either ordering or immutability. Since the semantics of the context
+ * is that it is immutable once it is placed in a query. Code should NEVER get the
+ * context map from a query and modify it, even if the actual implementation
+ * allows it.
*/
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)
+ {
+ // There is no semantic difference between an empty and a null context.
+ // Ensure that a context always exists to avoid the need to check for
+ // a null context. Jackson serialization will omit empty contexts.
+ this.context = context == null ? Collections.emptyMap() : new TreeMap<>(context);
Review Comment:
Thanks for this suggestion. I verified that `Collections.unmodifiableMap(.)` creates a wrapper around the `TreeMap`, which should preserve the key order which is needed for deterministic serialization.
##########
sql/src/main/java/org/apache/druid/sql/avatica/DruidConnection.java:
##########
@@ -97,7 +96,7 @@ public DruidJdbcStatement createStatement(SqlStatementFactory sqlStatementFactor
final DruidJdbcStatement statement = new DruidJdbcStatement(
connectionId,
statementId,
- context.copy(),
+ new HashMap<String, Object>(context),
Review Comment:
Thanks for calling this to my attention. I did a scrub of the JDBC code to streamline the way we handle contexts. Please take a look to see if I missed anything.
##########
sql/src/main/java/org/apache/druid/sql/SqlQueryPlus.java:
##########
@@ -39,25 +39,31 @@
* SQL requests come from a variety of sources in a variety of formats. Use
* the {@link Builder} class to create an instance from the information
* available at each point in the code.
+ * <p>
+ * The query context has a complex lifecycle. The copy here should remain
+ * unchanged: this is the set of values which the user requested. Planning will
+ * add (and sometimes remove) values: that work should be done on a copy of the
+ * context so that we have a clean record of the user's original requested
+ * values.
*/
public class SqlQueryPlus
{
private final String sql;
- private final QueryContext queryContext;
+ private final Map<String, Object> queryContext;
Review Comment:
Clarified that the context here is immutable. Used your `Collections.unmodifableMap(.)` trick to make it so. Doing this eliminates one more area of ambiguity: it is up to the planner to make a copy before it starts modifying the context. Explained that this rule is needed so that we do security checks on the user's original keys, but not on planner-inserted keys.
--
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
[GitHub] [druid] FrankChen021 commented on a diff in pull request #13071: Redesign QueryContext class
Posted by GitBox <gi...@apache.org>.
FrankChen021 commented on code in PR #13071:
URL: https://github.com/apache/druid/pull/13071#discussion_r990609934
##########
sql/src/main/java/org/apache/druid/sql/avatica/DruidConnection.java:
##########
@@ -97,7 +96,7 @@ public DruidJdbcStatement createStatement(SqlStatementFactory sqlStatementFactor
final DruidJdbcStatement statement = new DruidJdbcStatement(
connectionId,
statementId,
- context.copy(),
+ new HashMap<String, Object>(context),
Review Comment:
Can we move the copy into the ctor of `DruidJdbcStatement` so that no one forgets to make a copy?
--
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
[GitHub] [druid] paul-rogers commented on pull request #13071: Redesign QueryContext class
Posted by GitBox <gi...@apache.org>.
paul-rogers commented on PR #13071:
URL: https://github.com/apache/druid/pull/13071#issuecomment-1261640574
@FrankChen021, thanks for the encouraging comment! Since this PR builds on the work you recently did, can you perhaps give it a review? This one touches many files and so is at risk of conflicts, so we might want to get it in if things look OK. Thanks!
--
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
[GitHub] [druid] paul-rogers commented on a diff in pull request #13071: Redesign QueryContext class
Posted by GitBox <gi...@apache.org>.
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
[GitHub] [druid] FrankChen021 commented on a diff in pull request #13071: Redesign QueryContext class
Posted by GitBox <gi...@apache.org>.
FrankChen021 commented on code in PR #13071:
URL: https://github.com/apache/druid/pull/13071#discussion_r990607549
##########
processing/src/main/java/org/apache/druid/query/Query.java:
##########
@@ -96,64 +97,53 @@
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()}.
+ * <p>
+ * The default implementation is for backward compatibility. Derived classes should
+ * store and return the {@link QueryContext} directly.
*/
- @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().getBoolean()} instead.
+ */
+ @Deprecated
default boolean getContextBoolean(String key, boolean defaultValue)
Review Comment:
There's still a reference in `SinkQuerySegmentWalker` to this deprecated interface
--
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
[GitHub] [druid] FrankChen021 commented on a diff in pull request #13071: Redesign QueryContext class
Posted by GitBox <gi...@apache.org>.
FrankChen021 commented on code in PR #13071:
URL: https://github.com/apache/druid/pull/13071#discussion_r990609380
##########
processing/src/main/java/org/apache/druid/query/QueryContext.java:
##########
@@ -29,227 +33,545 @@
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.
+ * Code builds up a map of context values from serialization or during
+ * planning. Once that map is handed to the {@code QueryContext}, that map
+ * is effectively immutable.
+ * <p>
+ * The implementation uses a {@link TreeMap} so that the serialized form of a query
+ * lists context values in a deterministic order. Jackson will call
+ * {@code getContext()} on the query, which will call {@link #asMap()} here,
+ * which returns the sorted {@code TreeMap}.
+ * <p>
+ * The {@code TreeMap} is a mutable class. We'd prefer an immutable class, but
+ * we can choose either ordering or immutability. Since the semantics of the context
+ * is that it is immutable once it is placed in a query. Code should NEVER get the
+ * context map from a query and modify it, even if the actual implementation
+ * allows it.
*/
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)
+ {
+ // There is no semantic difference between an empty and a null context.
+ // Ensure that a context always exists to avoid the need to check for
+ // a null context. Jackson serialization will omit empty contexts.
+ this.context = context == null ? Collections.emptyMap() : new TreeMap<>(context);
Review Comment:
```suggestion
this.context = context == null ? Collections.emptyMap() : Collections.unmodifiableMap(new TreeMap<>(context));
```
--
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
[GitHub] [druid] abhishekagarwal87 commented on pull request #13071: Redesign QueryContext class
Posted by GitBox <gi...@apache.org>.
abhishekagarwal87 commented on PR #13071:
URL: https://github.com/apache/druid/pull/13071#issuecomment-1278874073
There are legit CI failures.
--
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
[GitHub] [druid] paul-rogers commented on pull request #13071: Redesign QueryContext class
Posted by GitBox <gi...@apache.org>.
paul-rogers commented on PR #13071:
URL: https://github.com/apache/druid/pull/13071#issuecomment-1276591628
PR is updated to pull the work for [Issue #13120](https://github.com/apache/druid/issues/13120). That change is needed to get the security IT to pass. Basically, that IT does context key security checks using JDBC. JDBC inserts a key, which fails the security check. This worked previously when we separated system and user keys. (In fact, this may be the reason that the prior PR did the split.)
This latest PR adds two config values, defined in the "release notes" section in the revised description. To make this work:
* The `AuthConfig` is not the one-stop shop to prepare the list of context keys to use for authorization checks.
* Three new set operations are added to `CollectionUtils` because Java, in its infinite wisdom, doesn't provide them.
* Two context keys moved from the SQL package to `QueryContexts` so that they are visible to `AuthConfig`.
* These two keys are "out-of-the-box freebies" for context security: they are always allowed because they are set by Druid itself in either the Router (query ID) or JDBC path (stringify arrays.)
* The code in the planner to gather the resources is tidied up a bit.
We'll want to review this final change before we merge.
--
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
[GitHub] [druid] abhishekagarwal87 commented on a diff in pull request #13071: Redesign QueryContext class
Posted by GitBox <gi...@apache.org>.
abhishekagarwal87 commented on code in PR #13071:
URL: https://github.com/apache/druid/pull/13071#discussion_r994148369
##########
server/src/main/java/org/apache/druid/server/security/AuthConfig.java:
##########
@@ -82,6 +81,44 @@ public AuthConfig(
@JsonProperty
private final boolean authorizeQueryContextParams;
+ /**
+ * The set of query context keys that are allowed, even when security is
+ * enabled. A null value is the same as an empty set.
+ */
+ @JsonProperty
+ private final Set<String> unsecuredContextKeys;
+
+ /**
+ * The set of query context keys to secure, when context security is
+ * enabled. Null has a special meaning: it means to ignore this set.
+ * Else, only the keys in this set are subject to security. If set,
+ * the unsecured list is ignored.
+ */
+ @JsonProperty
+ private final Set<String> securedContextKeys;
+
+ @JsonCreator
+ public AuthConfig(
+ @JsonProperty("authenticatorChain") List<String> authenticatorChain,
+ @JsonProperty("authorizers") List<String> authorizers,
+ @JsonProperty("unsecuredPaths") List<String> unsecuredPaths,
+ @JsonProperty("allowUnauthenticatedHttpOptions") boolean allowUnauthenticatedHttpOptions,
+ @JsonProperty("authorizeQueryContextParams") boolean authorizeQueryContextParams,
+ @JsonProperty("unsecuredContextKeys") Set<String> unsecuredContextKeys,
+ @JsonProperty("securedContextKeys") Set<String> securedContextKeys
+ )
+ {
+ this.authenticatorChain = authenticatorChain;
+ this.authorizers = authorizers;
+ this.unsecuredPaths = unsecuredPaths == null ? Collections.emptyList() : unsecuredPaths;
+ this.allowUnauthenticatedHttpOptions = allowUnauthenticatedHttpOptions;
+ this.authorizeQueryContextParams = authorizeQueryContextParams;
+ this.unsecuredContextKeys = unsecuredContextKeys == null
+ ? Collections.emptySet()
+ : unsecuredContextKeys;
+ this.securedContextKeys = securedContextKeys;
Review Comment:
should this be initialized as empty map like unsecuredContextKeys?
##########
server/src/main/java/org/apache/druid/server/security/AuthConfig.java:
##########
@@ -107,6 +144,36 @@ public boolean authorizeQueryContextParams()
return authorizeQueryContextParams;
}
+ /**
+ * Filter the user-supplied context keys based on the context key security
+ * rules. If context key security is disabled, then allow all keys. Else,
+ * apply the three key lists defined here.
+ * <ul>
+ * <li>Allow Druid-defined keys.</li>
+ * <li>Allow anything not in the secured context key list.</li>
+ * <li>Allow anything in the config-defined unsecured key list.</li>
+ * </ul>
+ * In the typical case, a site defines either the secured key list
+ * (to handle a few keys that are <i>are not</i> allowed) or the unsecured key
+ * list (to enumerate a few that <i>are</i> allowed.) If both lists
+ * are given, think of the secured list as exceptions to the unsecured
+ * key list.
+ *
+ * @return the list of secured keys to check via authentication
+ */
+ public Set<String> filterContextKeys(final Set<String> userKeys)
Review Comment:
nit - could we call this method `contextKeysToAuthorize` or `filterContextKeysToAuthorize`?
--
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
[GitHub] [druid] paul-rogers commented on pull request #13071: Redesign QueryContext class
Posted by GitBox <gi...@apache.org>.
paul-rogers commented on PR #13071:
URL: https://github.com/apache/druid/pull/13071#issuecomment-1265794737
@FrankChen021, thanks much for your detailed review! Very helpful. To answer your specific questions:
1. Sorry for the force-push. On many projects, the build is done on the PR itself. Any conflicts are resolved at the end of the review process. In Druid, each PR build is first rebased on master. Failure to rebase causes the build to fail and we get no feedback on the code. As a result, when there is a merge conflict, we are forced to rebase, which requires a force push. I wonder, is there some trick that others use to avoid this issue?
2. Sorry for the extra spaces in imports. That's just how Eclipse works. I'll make a note to manually remove such newlines in the future.
3. See below.
4. See below.
Your point 3 suggests that callers always see `QueryContext` and point 4 worries about the cost of creating `QueryContext` per call. Looking more carefully at the prior implementation, there is a path to make this work. That fix appears in the latest commit. The result is we eliminate the copies that were your concern in point 4.
Point 3 also suggests that callers always see `QueryContext` instead of the map. I agree that is the goal. The one caveat is that, for backward compatibility, the original map-based `getContext()` must exist and be a JSON property so that the JSON form of a query is unchanged.
Thank you for pointing out the potential bug with updating a query context in place in a query. The general rule, after this change, should be:
* The query context is created, as a map, before the query is constructed.
* Once the query is constructed, the context is accessed via `QueryContext` and is immutable. (Again, the one exception is that the map is presented to Jackson for JSON serialization.)
* If the context must change (such as while processing a native query), we must make a copy of the context then make a copy of the entire query with the new context. This ensures that the query is immutable and avoids the kinds of race conditions which first brought my attention to this area.
--
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
[GitHub] [druid] FrankChen021 commented on a diff in pull request #13071: Redesign QueryContext class
Posted by GitBox <gi...@apache.org>.
FrankChen021 commented on code in PR #13071:
URL: https://github.com/apache/druid/pull/13071#discussion_r990610166
##########
sql/src/main/java/org/apache/druid/sql/avatica/DruidConnection.java:
##########
@@ -127,7 +126,7 @@ public DruidJdbcPreparedStatement createPreparedStatement(
@SuppressWarnings("GuardedBy")
final PreparedStatement statement = sqlStatementFactory.preparedStatement(
- sqlQueryPlus.withContext(context.copy())
+ sqlQueryPlus.withContext(new HashMap<String, Object>(context))
Review Comment:
No need to make a copy of context because the ctor of `SqlQueryPlus` always make a copy.
--
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