clintropolis commented on a change in pull request #10208:
URL: https://github.com/apache/druid/pull/10208#discussion_r461874885
##########
File path: server/src/main/java/org/apache/druid/guice/QueryToolChestModule.java
##########
@@ -97,7 +97,7 @@ public void configure(Binder binder)
binder.bind(QueryToolChestWarehouse.class).to(MapQueryToolChestWarehouse.class);
- JsonConfigProvider.bind(binder, "druid.query", QueryConfig.class);
+ JsonConfigProvider.bind(binder, "druid.query.override.default.context",
OverrideDefaultQueryContext.class);
Review comment:
I think `.override.default.` is sort of redundant and overly nested,
please either make this `druid.query.default.context` or maybe
`druid.query.override.context`, but I think the `default` works a little better.
##########
File path:
processing/src/main/java/org/apache/druid/query/OverrideDefaultQueryContext.java
##########
@@ -19,43 +19,48 @@
package org.apache.druid.query;
-import com.fasterxml.jackson.annotation.JsonProperty;
-import org.apache.druid.query.QueryContexts.Vectorize;
-import org.apache.druid.segment.QueryableIndexStorageAdapter;
+import com.fasterxml.jackson.annotation.JsonAnyGetter;
+import com.fasterxml.jackson.annotation.JsonAnySetter;
+
+import java.util.HashMap;
+import java.util.Map;
/**
* A user configuration holder for all query types.
* Any query-specific configurations should go to their own configuration.
- *
* @see org.apache.druid.query.groupby.GroupByQueryConfig
* @see org.apache.druid.query.search.SearchQueryConfig
* @see org.apache.druid.query.topn.TopNQueryConfig
* @see org.apache.druid.query.metadata.SegmentMetadataQueryConfig
* @see org.apache.druid.query.scan.ScanQueryConfig
+ *
+ * This class config map is populated by any runtime property prefixed with
druid.query.override.default.context
+ * Note that config values should not be directly retrieved from this class
but instead should
+ * be read through {@link QueryContexts}. This class contains configs from
runtime property which is then merged with
+ * configs passed in query context. The result of the merge is subsequently
stored in the query context.
+ * The order of precedence in merging of the configs is as follow:
+ * runtime property values (store in this class) override by query context
parameter passed in with the query
+ *
+
*/
-public class QueryConfig
+public class OverrideDefaultQueryContext
{
- @JsonProperty
- private Vectorize vectorize = QueryContexts.DEFAULT_VECTORIZE;
-
- @JsonProperty
- private int vectorSize = QueryableIndexStorageAdapter.DEFAULT_VECTOR_SIZE;
+ private Map<String, Object> configs = new HashMap<>();
- public Vectorize getVectorize()
+ @JsonAnyGetter
Review comment:
This doesn't seem very flexible or future proof, what if we ever want to
add any defaults that are not context parameters? The way this currently is we
would need to add another config class that either has this as a property and
remove the binding to this, or we ensure that doesn't itself have a `context`
property and just have bindings for both. Why not just making this
`@JsonProperty("context")` and binding the class itself to the parent path? Is
there some benefit to doing it this way that I'm missing?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
