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/03 04:30:31 UTC
[GitHub] [druid] FrankChen021 opened a new pull request, #13022: Avoid ClassCastException when getting values from `QueryContext`
FrankChen021 opened a new pull request, #13022:
URL: https://github.com/apache/druid/pull/13022
The problem was first reported by #12760 , and #12833 solves that specific issue.
This PR solves all potential problems by providing a group of type safe methods.
### Description
`QueryContext` holds values passed from user side in JSON format.
And currently Druid requires values in JSON must be strictly type matched.
For example, a property `maxOnDiskStorage` in Druid is defined as integer, it must be in the format as
```json
"context": {"maxOnDiskStorage": 100}
```
If the number 100 is serialized as string as
```json
"context": {"maxOnDiskStorage": "100"}
```
Druid throws a `ClassCastException` to reject the input query.
Actually, string-formatted number "100" can be parsed as number at Druid server side.
### Key changes
#12833 adds a method `getContextHumanReadableBytes` to get context value from any acceptable format safely. And this PR adds a group of methods to do so and changes all existing code reference to the new APIs.
Newly added methods are:
```java
getContextAsInt
getContextAsLong
getContextAsFloat
getContextAsString
getContextAsEnum
getContextAsBoolean
```
Original unsafe method `getContextValue` is still kept to allow us get objects which are stored by Druid server side temporarily.
Some javadoc is added to tell callers know which methods should be used instead.
This PR has:
- [X] been self-reviewed.
- [ ] using the [concurrency checklist](https://github.com/apache/druid/blob/master/dev/code-review/concurrency.md) (Remove this item if the PR doesn't have any relation to concurrency.)
- [ ] added documentation for new or modified features or behaviors.
- [X] added Javadocs for most classes and all non-trivial methods. Linked related entities via Javadoc links.
- [ ] added or updated version, license, or notice information in [licenses.yaml](https://github.com/apache/druid/blob/master/dev/license.md)
- [ ] added comments explaining the "why" and the intent of the code wherever would not be obvious for an unfamiliar reader.
- [X] added unit tests or modified existing tests to cover new code paths, ensuring the threshold for [code coverage](https://github.com/apache/druid/blob/master/dev/code-review/code-coverage.md) is met.
- [ ] added integration tests.
- [ ] 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] paul-rogers commented on pull request #13022: Avoid ClassCastException when getting values from `QueryContext`
Posted by GitBox <gi...@apache.org>.
paul-rogers commented on PR #13022:
URL: https://github.com/apache/druid/pull/13022#issuecomment-1242722294
@FrankChen021, as it turns out, the `QueryContext` class introduced several problems. [PR 13049](https://github.com/apache/druid/pull/13049) proposes to rip out `QueryContext` and go back to using a map (along with a few adjustments to allow authorizing the context.) That PR will, unfortunately, conflict with this one.
While using maps solves some problems, it requires awkward code such as:
```java
x = QueryContexts.getSomething(context, key, default)
```
One advantage of the old `QueryContext` was that we could do:
```java
x = queryContext.getSomething(key, default);
```
Here, you propose a solution: add those values to the query class. That's OK, but it makes the query class pretty complex: we have methods for many tasks, and now we add multiple methods for context values. Plus, there are places (such as the SQL layer) where we work with the context without also having a native query. So, we'd have to duplicate these methods in several places. This suggests that methods to work with the context belong on the context itself, not on the users of the context (such as `Query`).
One solution, which I didn't include in PR 13049, is to define a new class: a typed wrapper on top of the map. Something like:
```java
class QueryContext {
Object get(String key);
String getString(String key);
String getString(String key, String defaultValue);
int getInt(String key);
int getInt(String key, int defaultValue);
...
}
```
For each of our supported types: String, Int, Long, etc. We'd then store an instance of `QueryContext` where we currently store the map.
`QueryContexts` has a zillion `getSomething()` methods for the various parameters. The logical step would be to move those into the above `QueryContext` class, else we'll find ourselves either duplicating code, or still using `QueryContexts` methods as shown earlier.
The problem that PR 13049 tries to solve is that there are times when the context is immutable, other times when it is mutable. And, those times trade off: the user's request is immutable, a mutable copy is made while planning the query, then the context must become immutable again once we start executing in multiple threads. So, we could have two forms: the above immutable form with only "getters", and a mutable form:
```java
class MutableQueryContext {
Object put(String key, Object value);
Object remove(String key);
...
}
```
To do this, we'd actually want `QueryContext` to be an interface, with two concrete implementations: `ImmutableQueryContext` and `MutableQueryContext`. One reason for the two classes is that the classes will clearly label the intent of when when the context is mutable or not.
This may still lead to a muddle, however. A `Query` will have a mutable context during the "planning" (`QueryRunner`-based) part of the native query execution, but immutable once we move to the execution (`Sequence`-based) part of the process.
There is also the issue of compatibility with custom `Query` classes where folks have implemented the existing interfaces using a map. It is not entirely clear how often that is done and what we can change in this area. Any ideas?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] FrankChen021 commented on a diff in pull request #13022: Avoid ClassCastException when getting values from `QueryContext`
Posted by GitBox <gi...@apache.org>.
FrankChen021 commented on code in PR #13022:
URL: https://github.com/apache/druid/pull/13022#discussion_r965773240
##########
processing/src/main/java/org/apache/druid/query/QueryContexts.java:
##########
@@ -399,12 +398,12 @@ public static <T> Query<T> verifyMaxQueryTimeout(Query<T> query, long maxQueryTi
public static <T> long getMaxQueuedBytes(Query<T> query, long defaultValue)
{
- return parseLong(query, MAX_QUEUED_BYTES_KEY, defaultValue);
+ return query.getContextAsLong(MAX_QUEUED_BYTES_KEY, defaultValue);
}
public static <T> long getMaxScatterGatherBytes(Query<T> query)
{
- return parseLong(query, MAX_SCATTER_GATHER_BYTES_KEY, Long.MAX_VALUE);
+ return query.getContextAsLong(MAX_SCATTER_GATHER_BYTES_KEY, Long.MAX_VALUE);
Review Comment:
after this PR, only `getContextAsxxxx` series methods are used to get values.
--
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 #13022: Avoid ClassCastException when getting values from `QueryContext`
Posted by GitBox <gi...@apache.org>.
FrankChen021 commented on PR #13022:
URL: https://github.com/apache/druid/pull/13022#issuecomment-1242876576
> Here's a follow-on idea. Modifying a context map can perhaps be left to working directly with the map, since it is mostly just calling `put()` along with a few other items. So, perhaps we can just focus on reading the context. Maybe we define:
>
> ```java
> interface Query<...> {
> default QueryContext getQueryContext() { return new QueryContext(getContext()); }
> ```
>
> Then, `QueryContext` has all the many accessor methods mentioned above. That is, `QueryContext` is a transient facade on top of the map.
>
> With this approach, all the myriad `getFoo` and `parseFoo` methods in `QueryContexts` can move to `QueryContext`. In this approach, the goal of this PR (type safety) would be provided by the typed `get{Type}` methods on `QueryContext`.
>
> What do you think? If this will work, I can perhaps modify the PR I mentioned to include this approach.
Hi @paul-rogers
When I touch this part in this PR, I noticed there are two classes, `QueryContext` and `QueryContexts`, the latter one provides a group by getXXXX helpers that QueryContext can be used. And also, another group of `getContextXXXX` methods are defined in `Query` to get values from `QueryContext`.
As you can seen in this PR, if a new `getXXX` method is added, we have to add it in 3 classes to follow current pattern. These are too complicated. I didn't make changes to this part because it involves a huge refactoring.
Basically, I agree with you that there should be a facade `QueryContext` there(but I think QueryContext should not expose `Map` object to callers directly), and a series of `getXXXX` methods are defined in `QueryContext` to get values. And at last those helper motheds in `Query` are removed or deprecated.
My focus in this PR is on the safe type conversion at caller side. And the work has been done. How about let's merge this first and then resolve the conflicts after you stablize the core changes on your branch?
--
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 #13022: Avoid ClassCastException when getting values from `QueryContext`
Posted by GitBox <gi...@apache.org>.
paul-rogers commented on PR #13022:
URL: https://github.com/apache/druid/pull/13022#issuecomment-1242792449
Went ahead and added the `QueryContext` "facade" in PR #13049 as it is a cleaner solution to the race condition issue which that PR seeks to solve.
--
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 #13022: Avoid ClassCastException when getting values from `QueryContext`
Posted by GitBox <gi...@apache.org>.
paul-rogers commented on code in PR #13022:
URL: https://github.com/apache/druid/pull/13022#discussion_r965225575
##########
processing/src/main/java/org/apache/druid/query/Query.java:
##########
@@ -124,22 +124,130 @@ default QueryContext getQueryContext()
return null;
}
- <ContextType> ContextType getContextValue(String key);
+ /**
+ * Get context value. It's recommended to use following methods instead
+ * {@link #getContextBoolean(String, boolean)}
+ * {@link #getContextAsString(String)}
+ * {@link #getContextAsInt(String)}
+ * {@link #getContextAsLong(String)}
+ * {@link #getContextAsFloat(String, float)}
+ * {@link #getContextAsEnum(String, Class, Enum)}
+ * {@link #getContextAsHumanReadableBytes(String, HumanReadableBytes)}
+ */
+ @Nullable
+ default Object getContextValue(String key)
+ {
+ if (getQueryContext() != null) {
+ return getQueryContext().get(key);
+ } else {
+ return null;
+ }
+ }
- <ContextType> ContextType getContextValue(String key, ContextType defaultValue);
+ @Nullable
+ default String getContextAsString(String key)
+ {
+ if (getQueryContext() != null) {
+ return getQueryContext().getAsString(key);
+ } else {
+ return null;
+ }
+ }
- boolean getContextBoolean(String key, boolean defaultValue);
+ default String getContextAsString(String key, String defaultValue)
+ {
+ if (getQueryContext() != null) {
+ return getQueryContext().getAsString(key, defaultValue);
+ } else {
+ return defaultValue;
+ }
+ }
+
+ @Nullable
+ default Integer getContextAsInt(String key)
+ {
+ if (getQueryContext() != null) {
+ return getQueryContext().getAsInt(key);
+ } else {
+ return null;
+ }
+ }
+
+ default int getContextAsInt(String key, int defaultValue)
+ {
+ if (getQueryContext() != null) {
+ return getQueryContext().getAsInt(key, defaultValue);
+ } else {
+ return defaultValue;
+ }
+ }
+
+ @Nullable
+ default Long getContextAsLong(String key)
+ {
+ if (getQueryContext() != null) {
+ return getQueryContext().getAsLong(key);
+ } else {
+ return null;
+ }
+ }
+
+ default long getContextAsLong(String key, long defaultValue)
+ {
+ if (getQueryContext() != null) {
+ return getQueryContext().getAsLong(key, defaultValue);
+ } else {
+ return defaultValue;
+ }
+ }
+
+ default float getContextAsFloat(String key, float defaultValue)
+ {
+ if (getQueryContext() != null) {
+ return getQueryContext().getAsFloat(key, defaultValue);
+ } else {
+ return defaultValue;
+ }
+ }
+
+ default <E extends Enum<E>> E getContextAsEnum(String key, Class<E> clazz, E defaultValue)
+ {
+ if (getQueryContext() != null) {
+ return getQueryContext().getAsEnum(key, clazz, defaultValue);
+ } else {
+ return defaultValue;
+ }
+ }
+
+ @Nullable
+ default Boolean getContextAsBoolean(String key)
Review Comment:
Nit: use the same name for both "flavors" of methods: easier to remember.
##########
processing/src/main/java/org/apache/druid/query/QueryContext.java:
##########
@@ -168,7 +168,20 @@ public Object getOrDefault(String key, Object defaultValue)
@Nullable
public String getAsString(String key)
{
- return (String) get(key);
+ Object val = get(key);
+ return val == null ? null : val.toString();
+ }
+
+ public String getAsString(String key, String defaultValue)
+ {
+ Object val = get(key);
+ return val == null ? defaultValue : val.toString();
+ }
+
+ @Nullable
+ public Boolean getAsBoolean(String parameter)
Review Comment:
Nit: best to not call query context values "parameters" since a query can have "parameters" which are not in the context. Suggestion: "key".
##########
processing/src/main/java/org/apache/druid/query/Query.java:
##########
@@ -124,22 +124,130 @@ default QueryContext getQueryContext()
return null;
}
- <ContextType> ContextType getContextValue(String key);
+ /**
+ * Get context value. It's recommended to use following methods instead
+ * {@link #getContextBoolean(String, boolean)}
+ * {@link #getContextAsString(String)}
+ * {@link #getContextAsInt(String)}
+ * {@link #getContextAsLong(String)}
+ * {@link #getContextAsFloat(String, float)}
+ * {@link #getContextAsEnum(String, Class, Enum)}
+ * {@link #getContextAsHumanReadableBytes(String, HumanReadableBytes)}
+ */
+ @Nullable
+ default Object getContextValue(String key)
+ {
+ if (getQueryContext() != null) {
+ return getQueryContext().get(key);
+ } else {
+ return null;
+ }
+ }
- <ContextType> ContextType getContextValue(String key, ContextType defaultValue);
+ @Nullable
+ default String getContextAsString(String key)
+ {
+ if (getQueryContext() != null) {
+ return getQueryContext().getAsString(key);
+ } else {
+ return null;
+ }
+ }
- boolean getContextBoolean(String key, boolean defaultValue);
+ default String getContextAsString(String key, String defaultValue)
+ {
+ if (getQueryContext() != null) {
+ return getQueryContext().getAsString(key, defaultValue);
+ } else {
+ return defaultValue;
+ }
+ }
+
+ @Nullable
+ default Integer getContextAsInt(String key)
+ {
+ if (getQueryContext() != null) {
+ return getQueryContext().getAsInt(key);
+ } else {
+ return null;
+ }
+ }
+
+ default int getContextAsInt(String key, int defaultValue)
+ {
+ if (getQueryContext() != null) {
Review Comment:
Nit: use `==` and revers the then and `else` clauses. Makes the code just a bit easier to read.
##########
processing/src/main/java/org/apache/druid/query/QueryContexts.java:
##########
@@ -399,12 +398,12 @@ public static <T> Query<T> verifyMaxQueryTimeout(Query<T> query, long maxQueryTi
public static <T> long getMaxQueuedBytes(Query<T> query, long defaultValue)
{
- return parseLong(query, MAX_QUEUED_BYTES_KEY, defaultValue);
+ return query.getContextAsLong(MAX_QUEUED_BYTES_KEY, defaultValue);
}
public static <T> long getMaxScatterGatherBytes(Query<T> query)
{
- return parseLong(query, MAX_SCATTER_GATHER_BYTES_KEY, Long.MAX_VALUE);
+ return query.getContextAsLong(MAX_SCATTER_GATHER_BYTES_KEY, Long.MAX_VALUE);
Review Comment:
Thanks for cleaning this up! I'd wondered why we have two different ways to get values.
##########
processing/src/main/java/org/apache/druid/query/QueryContexts.java:
##########
@@ -503,7 +487,24 @@ public static String getAsString(
} else if (value instanceof String) {
return (String) value;
} else {
- throw new IAE("Expected parameter [%s] to be String", parameter);
+ throw new IAE("Expected parameter [%s] to be a String, but got [%s]", parameter, value.getClass().getName());
+ }
+ }
+
+ @Nullable
+ public static Boolean getAsBoolean(
+ final String parameter,
+ final Object value
Review Comment:
This set of methods has gotten a bit muddled.
* This version takes the name of a context key (not "parameter") just so it can throw an error. But, there are places where we (or at least I) want to use this method where the map is not a context.
* There are versions that take a map and a default, and versions, like this, that take a value (not parameter) and convert it.
Maybe have the following set:
```java
// Check if a key exists for code that wants to do something special for this case
public boolean hasValue(Map<..> context, String key) ...
// Generic method to convert objects to boolean
public boolean parseBoolean(Object value)
{
// if can't parse, throw an exception
}
// Specific method for boolean context values
public boolean getBoolean(Map<...> context, String key, boolean defaultValue)
{
value = context.get(key);
if (value == null) {
return defaultValue;
}
try {
return parseBoolean(value);
} catch (Exception e) {
// Throw the "Expected context value [%s] to be a ... exception
}
}
```
```
##########
processing/src/main/java/org/apache/druid/query/QueryContexts.java:
##########
@@ -604,18 +644,31 @@ private QueryContexts()
{
}
- static <T, E extends Enum<E>> E parseEnum(Query<T> query, String key, Class<E> clazz, E defaultValue)
+ public static <E extends Enum<E>> E getAsEnum(String parameter, Object val, Class<E> clazz, E defaultValue)
{
- Object val = query.getContextValue(key);
if (val == null) {
return defaultValue;
}
- if (val instanceof String) {
- return Enum.valueOf(clazz, StringUtils.toUpperCase((String) val));
- } else if (val instanceof Boolean) {
- return Enum.valueOf(clazz, StringUtils.toUpperCase(String.valueOf(val)));
- } else {
- throw new ISE("Unknown type [%s]. Cannot parse!", val.getClass());
+
+ try {
+ if (val instanceof String) {
+ return Enum.valueOf(clazz, StringUtils.toUpperCase((String) val));
+ } else if (val instanceof Boolean) {
+ return Enum.valueOf(clazz, StringUtils.toUpperCase(String.valueOf(val)));
+ }
+ }
+ catch (IllegalArgumentException e) {
+ throw new IAE("Expected parameter [%s] must be value of enum [%s], but got [%s].",
+ parameter,
+ clazz.getName(),
+ val.toString());
}
+
+ throw new ISE(
+ "Expected parameter [%s] must be type of [%s], actual type is [%s].",
Review Comment:
Again, context values are not "parameters".
##########
processing/src/main/java/org/apache/druid/query/QueryContext.java:
##########
@@ -179,6 +192,11 @@ public boolean getAsBoolean(
return QueryContexts.getAsBoolean(parameter, get(parameter), defaultValue);
}
+ public Integer getAsInt(final String parameter)
Review Comment:
I wonder, since we have methods for all of these on `QueryContexts`, and we use `QueryContexts` to get each specific value type, should we just omit these methods and standard on always using `QueryContexts` to get context values? 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 #13022: Avoid ClassCastException when getting values from `QueryContext`
Posted by GitBox <gi...@apache.org>.
FrankChen021 commented on code in PR #13022:
URL: https://github.com/apache/druid/pull/13022#discussion_r965583376
##########
processing/src/main/java/org/apache/druid/query/Query.java:
##########
@@ -124,22 +124,130 @@ default QueryContext getQueryContext()
return null;
}
- <ContextType> ContextType getContextValue(String key);
+ /**
+ * Get context value. It's recommended to use following methods instead
+ * {@link #getContextBoolean(String, boolean)}
+ * {@link #getContextAsString(String)}
+ * {@link #getContextAsInt(String)}
+ * {@link #getContextAsLong(String)}
+ * {@link #getContextAsFloat(String, float)}
+ * {@link #getContextAsEnum(String, Class, Enum)}
+ * {@link #getContextAsHumanReadableBytes(String, HumanReadableBytes)}
+ */
+ @Nullable
+ default Object getContextValue(String key)
+ {
+ if (getQueryContext() != null) {
+ return getQueryContext().get(key);
+ } else {
+ return null;
+ }
+ }
- <ContextType> ContextType getContextValue(String key, ContextType defaultValue);
+ @Nullable
+ default String getContextAsString(String key)
+ {
+ if (getQueryContext() != null) {
+ return getQueryContext().getAsString(key);
+ } else {
+ return null;
+ }
+ }
- boolean getContextBoolean(String key, boolean defaultValue);
+ default String getContextAsString(String key, String defaultValue)
+ {
+ if (getQueryContext() != null) {
+ return getQueryContext().getAsString(key, defaultValue);
+ } else {
+ return defaultValue;
+ }
+ }
+
+ @Nullable
+ default Integer getContextAsInt(String key)
+ {
+ if (getQueryContext() != null) {
+ return getQueryContext().getAsInt(key);
+ } else {
+ return null;
+ }
+ }
+
+ default int getContextAsInt(String key, int defaultValue)
+ {
+ if (getQueryContext() != null) {
+ return getQueryContext().getAsInt(key, defaultValue);
+ } else {
+ return defaultValue;
+ }
+ }
+
+ @Nullable
+ default Long getContextAsLong(String key)
+ {
+ if (getQueryContext() != null) {
+ return getQueryContext().getAsLong(key);
+ } else {
+ return null;
+ }
+ }
+
+ default long getContextAsLong(String key, long defaultValue)
+ {
+ if (getQueryContext() != null) {
+ return getQueryContext().getAsLong(key, defaultValue);
+ } else {
+ return defaultValue;
+ }
+ }
+
+ default float getContextAsFloat(String key, float defaultValue)
+ {
+ if (getQueryContext() != null) {
+ return getQueryContext().getAsFloat(key, defaultValue);
+ } else {
+ return defaultValue;
+ }
+ }
+
+ default <E extends Enum<E>> E getContextAsEnum(String key, Class<E> clazz, E defaultValue)
+ {
+ if (getQueryContext() != null) {
+ return getQueryContext().getAsEnum(key, clazz, defaultValue);
+ } else {
+ return defaultValue;
+ }
+ }
+
+ @Nullable
+ default Boolean getContextAsBoolean(String key)
Review Comment:
I think you mean this method `getContextBoolean`?
I didn't change this method to use the same name pattern as 'getContextAs...' because there're lots reference to this method which might cause this PR so large. But indeed, it should be the same name pattern.
--
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 #13022: Avoid ClassCastException when getting values from `QueryContext`
Posted by GitBox <gi...@apache.org>.
FrankChen021 commented on PR #13022:
URL: https://github.com/apache/druid/pull/13022#issuecomment-1244974694
@paul-rogers Is there anything that I need to do in this PR? If it looks good to you, I think we can merge it to continue #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] FrankChen021 commented on pull request #13022: Avoid ClassCastException when getting values from `QueryContext`
Posted by GitBox <gi...@apache.org>.
FrankChen021 commented on PR #13022:
URL: https://github.com/apache/druid/pull/13022#issuecomment-1243192852
@paul-rogers I've replace all reference from calling `getContextAsXXXX` to `getQueryContext().getAsXXXX`, which is more similar to the final APIs that you propose.
old one:
```
final String timestampStringFromContext = query.getContextAsString(CTX_KEY_FUDGE_TIMESTAMP, "");
```
new one:
```
final String timestampStringFromContext = query.getQueryContext().getAsString(CTX_KEY_FUDGE_TIMESTAMP, "");
```
--
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 #13022: Avoid ClassCastException when getting values from `QueryContext`
Posted by GitBox <gi...@apache.org>.
FrankChen021 commented on PR #13022:
URL: https://github.com/apache/druid/pull/13022#issuecomment-1245179500
Thank you @paul-rogers @clintropolis
I will merge this.
--
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 #13022: Avoid ClassCastException when getting values from `QueryContext`
Posted by GitBox <gi...@apache.org>.
FrankChen021 commented on PR #13022:
URL: https://github.com/apache/druid/pull/13022#issuecomment-1239409967
@clintropolis Could you review this?
--
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] clintropolis commented on a diff in pull request #13022: Avoid ClassCastException when getting values from `QueryContext`
Posted by GitBox <gi...@apache.org>.
clintropolis commented on code in PR #13022:
URL: https://github.com/apache/druid/pull/13022#discussion_r965172651
##########
processing/src/main/java/org/apache/druid/query/Query.java:
##########
@@ -124,22 +124,130 @@ default QueryContext getQueryContext()
return null;
}
- <ContextType> ContextType getContextValue(String key);
Review Comment:
hmm, actually I guess this is an `@ExtensionPoint`, we should probably leave the old methods here to not be backwards incompatible
##########
processing/src/main/java/org/apache/druid/query/QueryContexts.java:
##########
@@ -604,18 +644,31 @@ private QueryContexts()
{
}
- static <T, E extends Enum<E>> E parseEnum(Query<T> query, String key, Class<E> clazz, E defaultValue)
Review Comment:
this class is also marked `@PublicApi` and so we should ideally leave the signatures in place
--
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 merged pull request #13022: Avoid ClassCastException when getting values from `QueryContext`
Posted by GitBox <gi...@apache.org>.
FrankChen021 merged PR #13022:
URL: https://github.com/apache/druid/pull/13022
--
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] clintropolis commented on pull request #13022: Avoid ClassCastException when getting values from `QueryContext`
Posted by GitBox <gi...@apache.org>.
clintropolis commented on PR #13022:
URL: https://github.com/apache/druid/pull/13022#issuecomment-1245942270
the last commit of this PR didn't actually run through travis and seems to be failing https://github.com/apache/druid/runs/8325253980, looking into the issue
--
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 #13022: Avoid ClassCastException when getting values from `QueryContext`
Posted by GitBox <gi...@apache.org>.
paul-rogers commented on PR #13022:
URL: https://github.com/apache/druid/pull/13022#issuecomment-1242733933
Here's a follow-on idea. Modifying a context map can perhaps be left to working directly with the map, since it is mostly just calling `put()` along with a few other items. So, perhaps we can just focus on reading the context. Maybe we define:
```java
interface Query<...> {
default QueryContext getQueryContext() { return new QueryContext(getContext()); }
```
Then, `QueryContext` has all the many accessor methods mentioned above. That is, `QueryContext` is a transient facade on top of the map.
With this approach, all the myriad `getFoo` and `parseFoo` method in `QueryContexts` can move to `QueryContext`.
What do you think? If this will work, I can perhaps modify the PR I mentioned to include this approach.
--
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 #13022: Avoid ClassCastException when getting values from `QueryContext`
Posted by GitBox <gi...@apache.org>.
paul-rogers commented on PR #13022:
URL: https://github.com/apache/druid/pull/13022#issuecomment-1242936186
@FrankChen021, your goal of type-safe conversion is a good one. The question is how to achieve that goal so that you can get this PR done, while also allowing us to solve the other issues in the other PR.
Perhaps the most expedient solution is the one you suggest: go ahead and commit this PR. The other one will undo the new `Query` methods; but that should be OK because they won't have been around long enough for extensions to use them. Replacing calls to your new methods with the other approach should just be a simple text replace operation, which I can do in the other PR.
The question we're asking is: should we add more methods to the `Query` interface, or should we move those responsibilities to a new class? In this PR, we've added more `getContextX` methods to `Query`. In the other PR, we've actually deprecated the existing ones in favor of the new `QueryContext` facade.
If we want to consider alternatives, let's note that if we commit this one first, then the other PR (assuming it is approved), would immediately remove the new `getContextX` methods: they'd live only for the time between the two commits. So, perhaps an alternative solution is to use the existing `QueryContexts` methods in this PR, then switch to the facade in the next PR. For example, consider this line in this PR:
```java
final String timestampStringFromContext = query.getContextAsString(CTX_KEY_FUDGE_TIMESTAMP, "");
```
With the suggested "temporary" solution, perhaps write this as:
```java
final String timestampStringFromContext = QueryContexts.getAsString(query, CTX_KEY_FUDGE_TIMESTAMP, "");
```
In the later PR, this would become:
```java
final String timestampStringFromContext = query.queryContext().getString(CTX_KEY_FUDGE_TIMESTAMP, "");
```
The alternative approach avoids adding new methods to the `Query` interface.
Otherwise, the two approaches are basically the same. There is one other difference, however: all the `getFoo()` methods in `QueryContexts`. With the approach in this PR, we'd have two ways to get values:
```java
final String timestampStringFromContext = query.getContextAsString(CTX_KEY_FUDGE_TIMESTAMP, "");
final boolean flag = QueryContexts.getEnableJoinFilterRewrite(query);
```
In the other PR, we can move all the getters to a single class:
```java
final QueryContext queryContext = query.queryContext();
final String timestampStringFromContext = queryContext.getString(CTX_KEY_FUDGE_TIMESTAMP, "");
final boolean flag = queryContext.enableJoinFilterRewrite();
```
The result is simpler code, while providing type safety, concurrency safety, and the ability to authorize context values.
So, the only question is how we help you get this one done now, and how we move onto the broader fix a bit later.
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 pull request #13022: Avoid ClassCastException when getting values from `QueryContext`
Posted by GitBox <gi...@apache.org>.
FrankChen021 commented on PR #13022:
URL: https://github.com/apache/druid/pull/13022#issuecomment-1246197865
@clintropolis Thanks for the fix. I didn't notice that travis not started but just saw the CI status of the last commit is OK. So why did the travis not start?
--
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] clintropolis commented on pull request #13022: Avoid ClassCastException when getting values from `QueryContext`
Posted by GitBox <gi...@apache.org>.
clintropolis commented on PR #13022:
URL: https://github.com/apache/druid/pull/13022#issuecomment-1245947764
opened #13083 to fix the test
--
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 #13022: Avoid ClassCastException when getting values from `QueryContext`
Posted by GitBox <gi...@apache.org>.
FrankChen021 commented on PR #13022:
URL: https://github.com/apache/druid/pull/13022#issuecomment-1243144052
@paul-rogers I think the new APIs you proposed is what I want -- that is there's only one place for these `getXXX` methods.
```java
final QueryContext queryContext = query.queryContext();
final String timestampStringFromContext = queryContext.getString(CTX_KEY_FUDGE_TIMESTAMP, "");
final boolean flag = queryContext.enableJoinFilterRewrite();
```
Let me remove all newly addde `getContextXXXX` methods in this PR from `Query` interface, and replace all reference to these methods to what you suggest as
```java
final String timestampStringFromContext = QueryContexts.getAsString(query, CTX_KEY_FUDGE_TIMESTAMP, "");
```
I will do this ASAP so that it won't block other PRs for long time.
--
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] lgtm-com[bot] commented on pull request #13022: Avoid ClassCastException when getting values from `QueryContext`
Posted by GitBox <gi...@apache.org>.
lgtm-com[bot] commented on PR #13022:
URL: https://github.com/apache/druid/pull/13022#issuecomment-1236509671
This pull request **introduces 1 alert** when merging 93db08253d83ea093c82527f3829a19a18c2c757 into 6805a7f9c243445b95090e9aa777512f6411b506 - [view on LGTM.com](https://lgtm.com/projects/g/apache/druid/rev/pr-a0c7d50033748f902fbaa8bfcf00f149ad42b421)
**new alerts:**
* 1 for Missing format argument
--
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 #13022: Avoid ClassCastException when getting values from `QueryContext`
Posted by GitBox <gi...@apache.org>.
FrankChen021 commented on code in PR #13022:
URL: https://github.com/apache/druid/pull/13022#discussion_r965537816
##########
processing/src/main/java/org/apache/druid/query/QueryContexts.java:
##########
@@ -604,18 +644,31 @@ private QueryContexts()
{
}
- static <T, E extends Enum<E>> E parseEnum(Query<T> query, String key, Class<E> clazz, E defaultValue)
Review Comment:
But this method is not declared as public. Do we need to keep it?
--
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 #13022: Avoid ClassCastException when getting values from `QueryContext`
Posted by GitBox <gi...@apache.org>.
FrankChen021 commented on code in PR #13022:
URL: https://github.com/apache/druid/pull/13022#discussion_r965539155
##########
processing/src/main/java/org/apache/druid/query/Query.java:
##########
@@ -124,22 +124,130 @@ default QueryContext getQueryContext()
return null;
}
- <ContextType> ContextType getContextValue(String key);
Review Comment:
Let me revert this.
--
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 #13022: Avoid ClassCastException when getting values from `QueryContext`
Posted by GitBox <gi...@apache.org>.
FrankChen021 commented on code in PR #13022:
URL: https://github.com/apache/druid/pull/13022#discussion_r965779945
##########
processing/src/main/java/org/apache/druid/query/QueryContexts.java:
##########
@@ -503,7 +487,24 @@ public static String getAsString(
} else if (value instanceof String) {
return (String) value;
} else {
- throw new IAE("Expected parameter [%s] to be String", parameter);
+ throw new IAE("Expected parameter [%s] to be a String, but got [%s]", parameter, value.getClass().getName());
+ }
+ }
+
+ @Nullable
+ public static Boolean getAsBoolean(
+ final String parameter,
+ final Object value
Review Comment:
A `parseBoolean` is still kept at line 474 in this class to allow to parse a value from a map object.
--
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