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/07 20:22:47 UTC
[GitHub] [druid] paul-rogers commented on a diff in pull request #13022: Avoid ClassCastException when getting values from `QueryContext`
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