feat!: add typed query context parameters by FrankChen021 · Pull Request #20198 · apache/druid

FrankChen021 · 2026-08-29T07:35:14Z

Description

This PR introduces typed, centrally declared query context parameters and migrates two representative parameters end to end:

useResultLevelCache, a Boolean parameter with a declared default.
maxRowsQueuedForOrdering, an Integer parameter with a runtime fallback and a range constraint.

The goal is to establish and demonstrate the parameter model with a small, reviewable change before migrating the remaining query context parameters.

Roadmap

After this PR is accepted, we will move other existing query context paramters into the centralized class in serveral PRs and finally we will provide a sys.query_context_parameters upon this centralized configuration.

Background

Issue #17769 proposes improving query context discoverability and validation. The closed PR #18087 explored a centralized registry, a system table, and broad migration of existing parameters, but touched more than 100 files.

This PR extracts a smaller end-to-end foundation from that work. It deliberately catalogs only two parameters with different types and migrates their call sites completely.

Complete validation of SQL SET parameters is not implemented in this PR. The SET processing path calls the catalog validation API to demonstrate the intended integration, but only the two migrated parameters are recognized. Unknown and unmigrated parameter names continue to be accepted until the catalog migration is complete.

Changes

Parameter model and catalog

Adds QueryContextParameter<T> and the centralized QueryContextParameters catalog. A descriptor contains its name, Java type, parser, nullability, optional default, constraints, deprecation message, and documentation metadata. The immutable BY_NAME catalog is derived from the declared public parameter fields.

Typed context APIs and call-site migration

Migrated these two parameters to demonstrate the APIs of above models:

useResultLevelCache, a Boolean parameter with a declared default.
maxRowsQueuedForOrdering, an Integer parameter with a runtime fallback and a range constraint.

Documentation generation

Adds a compile-time generator for descriptor-backed rows in the query context and Scan query documentation.

Normal Maven builds use verify mode. The generator renders complete document copies under processing/target/generated-docs, compares them with the checked-in Markdown, and fails the build if a generated row is stale. It does not modify source documentation in this mode.
Running Maven with -Dquery.context.docs.mode=generate updates the checked-in Markdown. The generator replaces only lines ending in an exact GENERATED QUERY CONTEXT PARAMETER marker. This currently covers the useResultLevelCache row in query-context-reference.md and the maxRowsQueuedForOrdering row in scan-query.md.
All other document lines are retained. Because generate mode rewrites each affected source file, it also normalizes line endings to \n.

SQL `SET` integration hook

Connects SqlQueryPlus to catalog validation. This validates recognized, migrated parameters and intentionally accepts all others. It demonstrates the future validation flow without claiming complete SET validation.

System Table Integration

Copilot

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

FrankChen021 · 2026-08-29T07:40:02Z

@clintropolis @gianm @kfaraz please review this change

FrankChen021

Review complete: no high-confidence correctness, security, or reliability issues found in the current changes.

Reviewed 30 of 30 changed files.

Validation: focused git diff --no-ext-diff --check passed. Builds and tests were not run.

This is an automated review by Codex GPT-5.6-Luna(max)

FrankChen021

Severity	Findings
P0	0
P1	1
P2	0
P3	0
Total	1

The review found one high-confidence user-facing error-handling issue; see the inline finding above.

Reviewed 30 of 30 changed files.

This is an automated review by Codex GPT-5.6-Luna(max)

FrankChen021

Severity	Findings
P0	0
P1	1
P2	0
P3	0
Total	1

Severity	Findings
P0	0
P1	1
P2	0
P3	0
Total	1

The updated validation path still lets malformed recognized SET values become HTTP 500; see the inline finding.

Reviewed 31 of 31 changed files.

This is an automated review by Codex GPT-5.6-Luna(max)

FrankChen021

I have reviewed the code for correctness, edge cases, concurrency, security, compatibility, serialization, integration, and test coverage; no high-confidence issues found in the current changes.

Reviewed 31 of 31 changed files.

This is an automated review by Codex GPT-5.6-Luna(max)

GWphua

PR seems to have a lot of unrelated changes. Lots of them can be re-used by methods currently in the codebase. Can do a re-check of them.

Unnecessary public APIs

QueryContext.of(...) and ofMap(...): eight overloads covering one through four parameter/value pairs. Replace all with the builder.
Query.withOverriddenContext(parameter, value): no production caller; only new tests use it.
Druids.TimeseriesQueryBuilder.context(parameter, value): no production caller and arbitrarily added only to the timeseries builder.
QueryContexts.override(context, parameter, value): completely unused and conflicts with the other APIs’ null semantics.
QueryContextParameter.set: production usage is only an edited embedded test.
QueryContextParameter.parseOrDefault: used only by its own unit tests.
QueryContext.has(parameter): only needed internally by get; it does not need to be public.
QueryContextParameter.toString: used only by its test.
longParameter and stringParameter: no descriptors use them.

GWphua · 2026-09-07T16:12:54Z

 |`brokerService`    | `null`                                 | Broker service to which this query should be routed. This parameter is honored only by a broker selector strategy of type *manual*. See [Router strategies](../design/router.md#router-strategies) for more details.|
 |`useCache`         | `true`                                 | Flag indicating whether to leverage the query cache for this query. When set to false, it disables reading from the query cache for this query. When set to true, Apache Druid uses `druid.broker.cache.useCache` or `druid.historical.cache.useCache` to determine whether or not to read from the query cache |
 |`populateCache`    | `true`                                 | Flag indicating whether to save the results of the query to the query cache. Primarily used for debugging. When set to false, it disables saving the results of this query to the query cache. When set to true, Druid uses `druid.broker.cache.populateCache` or `druid.historical.cache.populateCache` to determine whether or not to save the results of this query to the query cache |
-|`useResultLevelCache`| `true`                      | Flag indicating whether to leverage the result level cache for this query. When set to false, it disables reading from the query cache for this query. When set to true, Druid uses `druid.broker.cache.useResultLevelCache` to determine whether or not to read from the result-level query cache |


Is this comment an intended addition?

GWphua · 2026-09-07T16:13:04Z

 |property|description|values|default|
 |--------|-----------|------|-------|
-|maxRowsQueuedForOrdering|The maximum number of rows returned when time ordering is used.  Overrides the identically named config.|An integer in [1, 2147483647]|`druid.query.scan.maxRowsQueuedForOrdering`|
+|maxRowsQueuedForOrdering|The maximum number of rows returned when time ordering is used. Overrides the identically named config.|An integer in [1, 2147483647]|`druid.query.scan.maxRowsQueuedForOrdering`| <!-- GENERATED QUERY CONTEXT PARAMETER: maxRowsQueuedForOrdering -->


Same as above

GWphua · 2026-09-07T17:09:59Z

+  /**
+   * 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();
+  }
+


These methods should be unnecessary. We can simply use the following instead:

QueryContext context = QueryContext.builder()
.put(...)
.put(...)
.build();

GWphua · 2026-09-07T17:14:01Z

-  /**
-   * 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));
-  }
-


This removal seem out of scope of this PR

GWphua · 2026-09-07T17:14:14Z

-  @Nullable
-  public Duration getTimeoutDuration()
-  {
-    if (hasTimeout()) {
-      return Duration.ofMillis(getTimeout());
-    }
-    return null;
-  }
-


This removal seem out of scope of this PR

GWphua · 2026-09-07T17:14:18Z

-  /**
-   * @deprecated Use {@link #getRealtimeSegmentsMode()} instead.
-   */
-  @Deprecated
-  public boolean isRealtimeSegmentsOnly()
-  {
-    return getRealtimeSegmentsMode() == RealtimeSegmentsMode.EXCLUSIVE;
-  }


This removal seem out of scope of this PR

GWphua · 2026-09-07T17:16:44Z

-   * {@link QueryContext#getFloat(String)} <br/>
+ * {@link QueryContext#getFloat(String, float)} <br/>


Indentation + Should revert if we choose not to delete QueryContext#getFloat(String)

GWphua · 2026-09-07T17:22:18Z

-    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();


Is this change necessary?

feat!: add typed query context parameters

2da5ba5

Copilot AI lite review requested due to automatic review settings August 29, 2026 07:35

Copilot AI reviewed Aug 29, 2026

github-actions Bot added Area - Documentation Area - Querying Area - Dependencies labels Aug 29, 2026

FrankChen021 mentioned this pull request Aug 29, 2026

feat!: add typed query context parameters FrankChen021/druid#184

Closed

FrankChen021 commented Aug 29, 2026

View reviewed changes

FrankChen021 added 2 commits September 3, 2026 23:36

refactor: refine typed query context APIs

21cd38e

fix: satisfy forbidden API checks

add4d5f

FrankChen021 commented Sep 4, 2026

View reviewed changes

Comment thread sql/src/main/java/org/apache/druid/sql/SqlQueryPlus.java Outdated

fix: preserve invalid SET error semantics

2e37ba8

FrankChen021 commented Sep 5, 2026

View reviewed changes

Comment thread sql/src/main/java/org/apache/druid/sql/SqlQueryPlus.java Outdated

fix: normalize query context validation errors

dca2603

FrankChen021 commented Sep 5, 2026

View reviewed changes

GWphua reviewed Sep 7, 2026

View reviewed changes

GWphua requested changes Sep 7, 2026

View reviewed changes

		* {@link QueryContext#getFloat(String)} <br/>
		* {@link QueryContext#getFloat(String, float)} <br/>

Uh oh!

Conversation

FrankChen021 commented Aug 29, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Roadmap

Background

Changes

Parameter model and catalog

Typed context APIs and call-site migration

Documentation generation

SQL SET integration hook

System Table Integration

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Uh oh!

FrankChen021 commented Aug 29, 2026

Uh oh!

FrankChen021 left a comment

Choose a reason for hiding this comment

Uh oh!

FrankChen021 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

FrankChen021 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

FrankChen021 left a comment

Choose a reason for hiding this comment

Uh oh!

GWphua left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

FrankChen021 commented Aug 29, 2026 •

edited

Loading

SQL `SET` integration hook