Repository navigation
feat!: add typed query context parameters - #20198
FrankChen021 merged 14 commits into
Conversation
github-actions
Bot
added
Area - Documentation
Area - Querying
Area - Dependencies
labels
|
@clintropolis @gianm @kfaraz please review this change |
FrankChen021
left a comment
FrankChen021
left a comment
There was a problem hiding this comment.
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
left a comment
FrankChen021
left a comment
There was a problem hiding this comment.
| 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
left a comment
FrankChen021
left a comment
There was a problem hiding this comment.
| 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
left a comment
FrankChen021
left a comment
There was a problem hiding this comment.
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
left a comment
GWphua
left a comment
There was a problem hiding this comment.
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.
| |`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 | |
There was a problem hiding this comment.
Is this comment an intended addition?
There was a problem hiding this comment.
the document is updated by the documentation generator which removes extra SPACEs
There was a problem hiding this comment.
Yes, the marker is intentional. <!-- GENERATED QUERY CONTEXT PARAMETER: useResultLevelCache --> is an invisible HTML comment used as a stable anchor: the compile-time generator finds the marker and replaces the complete table row from descriptor metadata, while preserving the surrounding hand-written documentation. It is not rendered as user-facing content. The same marker is used for generated rows in the scan and SQL context documents.
| /** | ||
| * 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(); | ||
| } | ||
|
|
There was a problem hiding this comment.
These methods should be unnecessary. We can simply use the following instead:
QueryContext context = QueryContext.builder()
.put(...)
.put(...)
.build();
There was a problem hiding this 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.
| /** | ||
| * 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)); | ||
| } | ||
|
|
There was a problem hiding this comment.
This removal seem out of scope of this PR
There was a problem hiding this 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.
| @Nullable | ||
| public Duration getTimeoutDuration() | ||
| { | ||
| if (hasTimeout()) { | ||
| return Duration.ofMillis(getTimeout()); | ||
| } | ||
| return null; | ||
| } | ||
|
|
There was a problem hiding this comment.
This removal seem out of scope of this PR
There was a problem hiding this 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.
| /** | ||
| * @deprecated Use {@link #getRealtimeSegmentsMode()} instead. | ||
| */ | ||
| @Deprecated | ||
| public boolean isRealtimeSegmentsOnly() | ||
| { | ||
| return getRealtimeSegmentsMode() == RealtimeSegmentsMode.EXCLUSIVE; | ||
| } |
There was a problem hiding this comment.
This removal seem out of scope of this PR
There was a problem hiding this 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.
…t-parameter-descriptors # Conflicts: # processing/pom.xml
These public APIs are supported in this PR for further migrations PRs so that we don't introduce new public API and those new PRs can be done in parallel without conflict with each. The usage are demonstrated in : FrankChen021#190 and FrankChen021#191 |
FrankChen021
left a comment
FrankChen021
left a comment
There was a problem hiding this comment.
Thanks for the review. The APIs called out as unnecessary are intentional parts of the migration surface: typed of/ofMap replace immutable-map construction, typed withOverriddenContext, the typed builder context overload, QueryContextParameter.set, and QueryContext.has support upcoming production migrations, and QueryContexts.override, parseOrDefault, and the parameter builder helpers are retained for the follow-up descriptor migrations. The unused legacy QueryContext convenience methods (getFloat(String), getTimeoutDuration(), and deprecated isRealtimeSegmentsOnly()) are deliberately removed to keep the public API focused. The CachingClusteredClient change demonstrates the same migration for context construction; putRaw is retained only where a descriptor is not available yet. Generated HTML markers are intentional invisible anchors used by the compile-time documentation generator.
Reviewed 31 of 31 changed files.
This is an automated review by Codex GPT-5.6-luna
…t-parameter-descriptors
FrankChen021
left a comment
FrankChen021
left a comment
There was a problem hiding this comment.
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)
FrankChen021
left a comment
FrankChen021
left a comment
There was a problem hiding this comment.
🟢 Approval recommended
No actionable issues found in the current head. The incremental commit only clarifies the inclusive range-validation message and updates the two exact-message assertions; the full merge-base review confirms the typed query-context descriptors, null/default behavior, SQL SET error classification, scan-query fallback, and documentation generation remain consistent. The prior invalid-SET findings were rechecked and are resolved by wrapping both parser and constraint failures as InvalidSqlInput.
Reviewed 31 of 31 changed files.
Validation: focused git diff --no-ext-diff --check cc8b58642d4e865906968c73332687917d13dd3b 57ff2af0c6ffcb28c3459232dae24f3ed5d5915d passed. Builds and tests were not run.
This is an automated review by Codex GPT-5.6-Luna(max)
0f7972f to
58a9d4c
Compare
FrankChen021
left a comment
FrankChen021
left a comment
There was a problem hiding this comment.
🟢 Approval recommended
No actionable issues found in the incremental changes. Extracting the validator preserves parser and constraint error handling for SQL SET, including the previously corrected InvalidSqlInput classification; the catalog remains immutable, and removing the explicit null check before Class.isInstance preserves null behavior.
Review mode: updated_since_review. Reviewed all 5 of 5 files changed since the previous review (counting the renamed test once), plus relevant portions of 8 supporting files: 13 files inspected in total. This is an incremental assessment; the other 19 files in the full 32-file PR were not re-reviewed this turn.
Incremental files reviewed:
processing/src/main/java/org/apache/druid/query/context/QueryContextParameter.javaprocessing/src/main/java/org/apache/druid/query/context/QueryContextParameterValidator.javaprocessing/src/main/java/org/apache/druid/query/context/QueryContextParameters.javaprocessing/src/test/java/org/apache/druid/query/context/QueryContextParameterValidatorTest.javasql/src/main/java/org/apache/druid/sql/SqlQueryPlus.java
Supporting files inspected:
processing/pom.xmlprocessing/src/main/java/org/apache/druid/query/QueryContexts.javaprocessing/src/main/java/org/apache/druid/query/context/constraint/Range.javaprocessing/src/main/java/org/apache/druid/query/context/docs/ParameterDocumentationGenerator.javaprocessing/src/test/java/org/apache/druid/query/context/QueryContextParameterTest.javasql/src/main/java/org/apache/druid/sql/http/SqlResource.javasql/src/test/java/org/apache/druid/sql/SqlQueryPlusTest.javasql/src/test/java/org/apache/druid/sql/http/SqlResourceTest.java
Validation: git diff --no-ext-diff --check 57ff2af0c6ffcb28c3459232dae24f3ed5d5915d 58a9d4c9f2b01acf85311e6a5169609c85345069 passed. A repository-wide Java reference search found no remaining calls to QueryContextParameters.validate or references to the old test class. Builds and tests were not run. No review blockers.
This is an automated review by Codex GPT-5.6-Luna(max)
- Treat null as unset consistently across typed readers and writers. - Parse integer/long parameters exactly; overflow reports the parameter's Range bounds instead of silently truncating. - Report all invalid values with BadQueryContextException and clearer "Query context parameter [x] ..." messages. - Build the parameter catalog lazily via a memoized Supplier. - Add typed context(param, value) to all Druids builders. - Remove QueryContextBuilder.putRaw and revert CachingClusteredClient; putAll rejects null keys/values like ImmutableMap.Builder. - Restore removed public QueryContext/QueryContexts APIs as deprecated.
|
Thank you @GWphua |
FrankChen021
left a comment
FrankChen021
left a comment
There was a problem hiding this comment.
🟢 Approval recommended
No actionable issues found in the current head. The incremental changes preserve the earlier fixes for SQL SET error classification, typed null/default behavior, exact integer bounds, lazy catalog access, and downstream context construction; rechecking the full merge-base diff and relevant callers found no regression.
Reviewed 29 of 29 changed files. This updated-since-review assessment started with all 21 files changed after 58a9d4c9f2b01acf85311e6a5169609c85345069, then covered the full current diff and relevant surrounding code.
Validation: git diff --no-ext-diff --check cc8b58642d4e865906968c73332687917d13dd3b f4c8f0bd19dd56a461bf025cca54d7411e5c0dfe and the corresponding incremental check both passed. Builds and tests were not run.
This is an automated review by Codex GPT-5.6-Luna(max)
- Drop tests that duplicate behavior covered elsewhere (later-wins, typed put validation, explicit null defaults, integer overflow and error messages repeated across layers, duplicate SQL and HTTP checks). - Consolidate integer parsing and set() tests, and move parser error messages to QueryContextParameterTest. - Make the ScanQuery test validate maxRowsQueuedForOrdering through ScanQuery itself rather than through typed overrides.
FrankChen021
left a comment
FrankChen021
left a comment
There was a problem hiding this comment.
🟢 Approval recommended
No actionable issues found in the current head. The incremental changes consolidate test coverage without changing production behavior and now exercise ScanQuery's range validation through a raw context map. Rechecking the full PR and its callers confirms the prior SQL SET parser/constraint error-classification fixes remain intact, typed null/default semantics are preserved, integer coercion rejects lossy and overflowing values, and Scan queries retain their runtime configuration fallback.
Reviewed 28 of 28 files in the current PR diff, plus the eight-file incremental diff and relevant surrounding SQL, cache, query-context, and scan execution code. Findings: 0. The incremental diff includes removal of a cache test from a file that is no longer changed in the current PR.
Validation: both the full diff check (git diff --no-ext-diff --check cc8b58642d4e865906968c73332687917d13dd3b c0d00e81e437d002dd2240fb0c4cef5e6fb8c240) and incremental diff check (git diff --no-ext-diff --check f4c8f0bd19dd56a461bf025cca54d7411e5c0dfe c0d00e81e437d002dd2240fb0c4cef5e6fb8c240) passed. This was a static review; tests and builds were not run.
This is an automated review by Codex GPT-5.6-Luna(max)
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_parametersupon 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
SETparameters is not implemented in this PR. TheSETprocessing 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 centralizedQueryContextParameterscatalog. A descriptor contains its name, Java type, parser, nullability, optional default, constraints, deprecation message, and documentation metadata. The immutableBY_NAMEcatalog 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.
verifymode. The generator renders complete document copies underprocessing/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.-Dquery.context.docs.mode=generateupdates the checked-in Markdown. The generator replaces only lines ending in an exactGENERATED QUERY CONTEXT PARAMETERmarker. This currently covers theuseResultLevelCacherow inquery-context-reference.mdand themaxRowsQueuedForOrderingrow inscan-query.md.\n.SQL
SETintegration hookConnects
SqlQueryPlusto catalog validation. This validates recognized, migrated parameters and intentionally accepts all others. It demonstrates the future validation flow without claiming completeSETvalidation.System Table Integration