FrankChen021 commented on code in PR #20198:
URL: https://github.com/apache/druid/pull/20198#discussion_r3955364179
##########
processing/src/main/java/org/apache/druid/query/QueryContext.java:
##########
@@ -81,11 +84,152 @@ public static QueryContext empty()
return EMPTY;
}
+ /**
+ * Creates a builder for a query context map.
+ */
+ public static QueryContextBuilder builder()
+ {
+ return new QueryContextBuilder();
+ }
+
public static QueryContext of(Map<String, Object> context)
{
return new QueryContext(context);
}
+ /**
+ * Creates a query context from one declared query context parameter.
+ */
+ public static <T> QueryContext of(
+ final QueryContextParameter<T> parameter,
+ @Nullable final T value
+ )
+ {
+ return new QueryContext(ofMap(parameter, value));
+ }
+
+ /**
+ * Creates a query context map from one declared query context parameter.
+ */
+ public static <T> Map<String, Object> ofMap(
+ final QueryContextParameter<T> parameter,
+ @Nullable final T value
+ )
+ {
+ return builder().put(parameter, value).toMap();
+ }
+
+ /**
+ * Creates a query context from two declared query context parameters.
+ */
+ public static <T1, T2> QueryContext of(
+ final QueryContextParameter<T1> parameter1,
+ @Nullable final T1 value1,
+ final QueryContextParameter<T2> parameter2,
+ @Nullable final T2 value2
+ )
+ {
+ return new QueryContext(ofMap(parameter1, value1, parameter2, value2));
+ }
+
+ /**
+ * Creates a query context map from two declared query context parameters.
+ */
+ public static <T1, T2> Map<String, Object> ofMap(
+ final QueryContextParameter<T1> parameter1,
+ @Nullable final T1 value1,
+ final QueryContextParameter<T2> parameter2,
+ @Nullable final T2 value2
+ )
+ {
+ return builder()
+ .put(parameter1, value1)
+ .put(parameter2, value2)
+ .toMap();
+ }
+
+ /**
+ * Creates a query context from three declared query context parameters.
+ */
+ public static <T1, T2, T3> QueryContext of(
+ final QueryContextParameter<T1> parameter1,
+ @Nullable final T1 value1,
+ final QueryContextParameter<T2> parameter2,
+ @Nullable final T2 value2,
+ final QueryContextParameter<T3> parameter3,
+ @Nullable final T3 value3
+ )
+ {
+ return new QueryContext(ofMap(parameter1, value1, parameter2, value2,
parameter3, value3));
+ }
+
+ /**
+ * Creates a query context map from three declared query context parameters.
+ */
+ public static <T1, T2, T3> Map<String, Object> ofMap(
+ final QueryContextParameter<T1> parameter1,
+ @Nullable final T1 value1,
+ final QueryContextParameter<T2> parameter2,
+ @Nullable final T2 value2,
+ final QueryContextParameter<T3> parameter3,
+ @Nullable final T3 value3
+ )
+ {
+ return builder()
+ .put(parameter1, value1)
+ .put(parameter2, value2)
+ .put(parameter3, value3)
+ .toMap();
+ }
+
+ /**
+ * Creates a query context from four declared query context parameters.
+ */
+ public static <T1, T2, T3, T4> QueryContext of(
+ final QueryContextParameter<T1> parameter1,
+ @Nullable final T1 value1,
+ final QueryContextParameter<T2> parameter2,
+ @Nullable final T2 value2,
+ final QueryContextParameter<T3> parameter3,
+ @Nullable final T3 value3,
+ final QueryContextParameter<T4> parameter4,
+ @Nullable final T4 value4
+ )
+ {
+ return new QueryContext(ofMap(
+ parameter1,
+ value1,
+ parameter2,
+ value2,
+ parameter3,
+ value3,
+ parameter4,
+ value4
+ ));
+ }
+
+ /**
+ * Creates a query context map from four declared query context parameters.
+ */
+ public static <T1, T2, T3, T4> Map<String, Object> ofMap(
+ final QueryContextParameter<T1> parameter1,
+ @Nullable final T1 value1,
+ final QueryContextParameter<T2> parameter2,
+ @Nullable final T2 value2,
+ final QueryContextParameter<T3> parameter3,
+ @Nullable final T3 value3,
+ final QueryContextParameter<T4> parameter4,
+ @Nullable final T4 value4
+ )
+ {
+ return builder()
+ .put(parameter1, value1)
+ .put(parameter2, value2)
+ .put(parameter3, value3)
+ .put(parameter4, value4)
+ .toMap();
+ }
+
Review Comment:
These overloads are intentional rather than accidental duplication. They are
the typed replacement for `ImmutableMap.of`/`Map.of` at migrated call sites:
`ofMap` keeps the existing `Map<String, Object>` shape, while `of` returns
`QueryContext`. The follow-up query-context migrations will use these APIs, so
removing them now would force those changes back to raw keys or repetitive
builder code. The builder remains available for larger or mixed contexts.
<!-- mergelens:review -->
##########
processing/src/main/java/org/apache/druid/query/QueryContext.java:
##########
@@ -199,18 +389,6 @@ public long getLong(final String key, final long
defaultValue)
return QueryContexts.parseLong(context, key, defaultValue);
}
- /**
- * 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)
- {
- return QueryContexts.getAsFloat(key, get(key));
- }
-
Review Comment:
This removal is intentional. `getFloat(String)` has no in-repository
callers, and the typed descriptor accessors are the API we want to expose going
forward. Removing this unused overload keeps `QueryContext`'s public API
focused; it is not required by the follow-up parameter migrations.
<!-- mergelens:review -->
##########
processing/src/main/java/org/apache/druid/query/QueryContext.java:
##########
@@ -514,15 +692,6 @@ public long getTimeout(long defaultTimeout)
);
}
- @Nullable
- public Duration getTimeoutDuration()
- {
- if (hasTimeout()) {
- return Duration.ofMillis(getTimeout());
- }
- return null;
- }
-
Review Comment:
This is intentional API cleanup: `getTimeoutDuration()` has no in-repository
callers, and callers can derive a duration from `getTimeout()` when needed. We
want to remove unused legacy convenience methods from `QueryContext` rather
than carry them into the typed API.
<!-- mergelens:review -->
##########
processing/src/main/java/org/apache/druid/query/QueryContext.java:
##########
@@ -818,12 +987,4 @@ public RealtimeSegmentsMode getRealtimeSegmentsMode()
return QueryContexts.DEFAULT_REALTIME_SEGMENTS_MODE;
}
- /**
- * @deprecated Use {@link #getRealtimeSegmentsMode()} instead.
- */
- @Deprecated
- public boolean isRealtimeSegmentsOnly()
- {
- return getRealtimeSegmentsMode() == RealtimeSegmentsMode.EXCLUSIVE;
- }
Review Comment:
`isRealtimeSegmentsOnly()` is deprecated and has no in-repository callers.
`getRealtimeSegmentsMode()` is the replacement, so this removal is intentional
to keep the cleaned-up API focused. It is not needed by the follow-up
migrations.
<!-- mergelens:review -->
##########
processing/src/main/java/org/apache/druid/query/Query.java:
##########
@@ -132,7 +134,7 @@ default QueryContext context()
* {@link QueryContext#getString(String)} <br/>
* {@link QueryContext#getInt(String)} <br/>
* {@link QueryContext#getLong(String)} <br/>
- * {@link QueryContext#getFloat(String)} <br/>
+ * {@link QueryContext#getFloat(String, float)} <br/>
Review Comment:
Fixed the Javadoc indentation in commit
[`350ce20635`](https://github.com/FrankChen021/druid/commit/350ce20635498de5f08acbccff9a8843c18f6ed9).
The `getFloat(String)` removal is intentional for the unused-API cleanup
described above.
<!-- mergelens:review -->
##########
server/src/main/java/org/apache/druid/client/CachingClusteredClient.java:
##########
@@ -297,24 +298,24 @@ private class SpecificQueryRunnable<T>
);
}
- private ImmutableMap<String, Object> makeDownstreamQueryContext()
+ private Map<String, Object> makeDownstreamQueryContext()
{
- final ImmutableMap.Builder<String, Object> contextBuilder = new
ImmutableMap.Builder<>();
+ final QueryContextBuilder contextBuilder = QueryContext.builder();
final QueryContext queryContext = query.context();
final int priority = queryContext.getPriority();
- contextBuilder.put(QueryContexts.PRIORITY_KEY, priority);
+ contextBuilder.putRaw(QueryContexts.PRIORITY_KEY, priority);
final String lane = queryContext.getLane();
if (lane != null) {
- contextBuilder.put(QueryContexts.LANE_KEY, lane);
+ contextBuilder.putRaw(QueryContexts.LANE_KEY, lane);
}
if (populateCache) {
// prevent down-stream nodes from caching results as well if we are
populating the cache
- contextBuilder.put(CacheConfig.POPULATE_CACHE, false);
- contextBuilder.put(QueryContexts.BY_SEGMENT_KEY, true);
+ contextBuilder.putRaw(CacheConfig.POPULATE_CACHE, false);
+ contextBuilder.putRaw(QueryContexts.BY_SEGMENT_KEY, true);
}
- return contextBuilder.build();
+ return contextBuilder.toMap();
Review Comment:
Yes, this change is intentional. Context construction should migrate from
`ImmutableMap`/`Map` builders to the typed
`QueryContextBuilder`/`QueryContext.of*` APIs. For parameters that are not
descriptors yet, `putRaw` is the compatibility path; continuing to use raw maps
would require repetitive `QueryContextParameters.X.getName()` calls and would
not demonstrate the new API. Once the remaining descriptors are migrated, those
raw entries can move to typed `put` calls.
<!-- mergelens:review -->
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
