[maytasm]

Description

The Broker's result-level cache key comes only from the query's shape (CacheStrategy#computeResultLevelCacheKey). It doesn't include the datasource or the query intervals. Those only feed the ETag stored at the start of the cached value (intervals were added there in #7325). So queries that differ only by interval or datasource map to one cache entry. Each reads the other's entry, rejects it because the ETag doesn't match, and overwrites it, so alternating queries (a, b, a, b) never hit.

As far as I know, there's no reason not to do this. In #9280 (comment), @gianm suggested changing ResultLevelCachingQueryRunner so the datasource and interval are part of the cache key rather than the cached value. That would let it cache results for queries that are the same except for their interval. The comment gives no reason against the change; it wasn't done only because he didn't plan to work on it himself at the time.

Added druid.broker.cache.includeDatasourceAndIntervalsInResultLevelCacheKey

The default is false, which leaves the key unchanged. When true, ResultLevelCachingQueryRunner builds the key from:

• a version byte,
• the strategy's result-level key,
• the datasource cache key (DataSource#getCacheKey), when it isn't null,
• each query interval's start and end millis, encoded the same way as CacheUtil#computeSegmentCacheKey.

The ETag in the cached value, and the check on read, are unchanged. The flag only decides which entry a query reads and writes.

Design notes:

• The flag is off by default for two reasons. Enabling it means existing result-level entries are no longer read, and the cache then stores more entries. The extra entries matter most for clusters that use the in-memory Caffeine cache, which is usually much smaller than a remote distributed cache such as Redis or memcached.
• When the datasource cache key is null, only the intervals are added. The ETag is also null for those datasources, so nothing is cached for them either way.
• Intervals come from the query's segment spec, which already condenses them, so equivalent interval lists produce the same key.
• Trade-offs, also listed in the docs:
  • The cache holds more entries.
  • A query whose interval changes on every run, such as a rolling time window, adds a new entry each run that is never read again.
  • Queries whose intervals cover the same data but differ elsewhere no longer reuse each other's entry.

Release note

Added druid.broker.cache.includeDatasourceAndIntervalsInResultLevelCacheKey (default false). When enabled, the Broker's result-level cache key includes the query's datasource and intervals, so queries that differ only by interval or datasource get their own cache entries instead of overwriting one shared entry.

---

Key changed/added classes in this PR

• CacheConfig
• ResultLevelCachingQueryRunner
• ResultLevelCachingQueryRunnerTest

---

This PR has:

• been self-reviewed.
  • using the concurrency checklist (Remove this item if the PR doesn't have any relation to concurrency.)
• added documentation for new or modified features or behaviors.
• a release note entry in the PR description.
• added Javadocs for most classes and all non-trivial methods. Linked related entities via Javadoc links.
• added or updated version, license, or notice information in licenses.yaml
• added comments explaining the "why" and the intent of the code wherever would not be obvious for an unfamiliar reader.
• added unit tests or modified existing tests to cover new code paths, ensuring the threshold for code coverage is met.
• added integration tests.
• been tested in a test Druid cluster.

[FrankChen021]

🟢 Approval recommended

The opt-in key path preserves the legacy result-level cache key when disabled and separates entries by datasource cache key and condensed interval bounds when enabled. I also inspected surrounding result-cache wiring, datasource cacheability contracts, interval normalization, and the added tests.

Reviewed 4 of 4 changed files.

---

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

[agali-09]

LGTM

[agali-09]

nit: Wouldn't there be a situation where appending could lead the key to overflow?

[maytasm]

This shouldn't be a problem:

• Integer overflow: CacheKeyBuilder tracks the key size as an int, so overflowing it would take a key over 2 GiB. This is unlikely.
• Backend key-length limits: Distributed cache like memcached store the SHA-1 hex of the key (i.e. MemcachedCache.computeKeyHash), so their 250-character limit never applies. Caffeine and the map cache have no limit.
• Added size from this change: the flag adds the datasource key (its name plus about 3 bytes) and 18 bytes per interval. The existing query key already holds the filters, dimensions and aggregators, which are usually much bigger.

[agali-09]

Do we need to length-prefix or add a seperator between queryCacheKey and dataSourceCacheKey byte arrays to avoid collisions?

Though appendByteArray() adds 0x01 as a type marker, that byte can also appear in the data. For example, [AA] + [BB, 01, CC] and [AA, 01, BB] + [CC] both encode as 01 AA 01 BB 01 CC.

  public CacheKeyBuilder appendByteArray(byte[] input)
  {
    appendItem(BYTE_ARRAY_KEY, input);
    return this;
  }
...
  private void appendItem(byte typeKey, byte[] input)
  {
    final Item item = new Item(typeKey, input);
    items.add(item);
    size += item.byteSize();
  }

[maytasm]

• A collision is very unlikely. One query's key would have to equal another query's key followed by bytes shaped exactly like a datasource cache key. Although is possible as you pointed out.
• Despite that, the ETag still catches it. Each cache value starts with an ETag, a SHA-1 of the segment IDs, the clipped query intervals and the query cache key (CachingClusteredClient.computeResultLevelCachingEtag, around line 821). Segment IDs include the datasource and interval, so a collision between different datasources or intervals fails the ETag check. It then behaves like today's default: a miss, and the entry gets overwritten.
• It's a simple fix so let me push a change

[maytasm]

Done