fix: UI cache toggle should disable all cache layers - #19774
Conversation
The "Use cache" toggle in the run panel only set useCache/populateCache, leaving result-level and forwarded result-level cache context keys untouched. Extend the toggle to also set useResultLevelCache, populateResultLevelCache, useForwardedResultLevelCache, and populateForwardedResultLevelCache so disabling cache from the UI actually disables all cache layers.
49bec51 to
d71c1a8
Compare
FrankChen021
left a comment
There was a problem hiding this comment.
| Severity | Findings |
|---|---|
| P0 | 0 |
| P1 | 1 |
| P2 | 1 |
| P3 | 0 |
| Total | 2 |
Legacy queries saved with only the pre-existing cache flags remain result-cache-enabled, and the newly emitted forwarded-cache keys have no server-side consumer.
Reviewed 2 of 2 changed files.
This is an automated review by Codex GPT-5.6-Sol
| ...queryContext, | ||
| useCache, | ||
| populateCache: useCache, | ||
| useResultLevelCache: useCache, |
There was a problem hiding this comment.
[P1] Normalize legacy disabled cache contexts
The new flags are written only after an onValueChange event. Queries saved by the previous UI already contain useCache=false/populateCache=false but lack these result-level keys; value={useCache} still displays Disabled, while execution leaves both result-level flags absent. Druid defaults absent useResultLevelCache/populateResultLevelCache values to true, so an enabled broker result cache remains active. Normalize legacy disabled contexts or submit false result-level flags whenever useCache is false, without requiring users to toggle on and off again.
| populateCache: useCache, | ||
| useResultLevelCache: useCache, | ||
| populateResultLevelCache: useCache, | ||
| useForwardedResultLevelCache: useCache, |
There was a problem hiding this comment.
[P2] Do not emit unsupported forwarded-cache keys
These two forwarded result-level keys have no server-side consumer: their only repository occurrences are the newly added UI/type fields, while Druid reads only useResultLevelCache and populateResultLevelCache. They therefore disable nothing. Additionally, when query-context authorization is enabled, SQL treats each user-supplied key as a QUERY_CONTEXT resource, so toggling this control can require permissions for meaningless keys and reject otherwise valid queries. Remove them, or add actual server-side support if a separate forwarded cache layer is intended.
Summary
useCache/populateCachein the query context, leaving result-level and forwarded result-level cache settings unaffected.useResultLevelCache,populateResultLevelCache,useForwardedResultLevelCache, andpopulateForwardedResultLevelCache, so disabling cache from the UI actually disables all cache layers.QueryContexttype definition.Test plan
tsc --noEmitpasses for web-console