Fix result-level cache for queries #7325
Conversation
Make HavingSpec cacheable and implement getCacheKey for subclasses Add unit tests for computeResultLevelCacheKey
surekhasaharan
changed the title
| public byte[] getCacheKey() | ||
| { | ||
| try { | ||
| final ByteArrayOutputStream outputStream = new ByteArrayOutputStream(); |
There was a problem hiding this comment.
Would CacheKeyBuilder work for this (and similar method)?
There was a problem hiding this comment.
Yeah, I can use CacheKeyBuilder, wonder why I didn't earlier.
| .appendString(aggregationName) | ||
| .appendByteArray(StringUtils.toUtf8(String.valueOf(value))) | ||
| .appendStrings(aggregators.keySet()) | ||
| .appendCacheables(aggregators.values()) |
There was a problem hiding this comment.
not sure the best way to add the aggregators map to CacheKeyBuilder, I am adding the keys and values separately, is that okay ?
There was a problem hiding this comment.
You don't need to add them to the cache key -- only the aggregation and value matter. The idea is that anything that changes the meaning or effect of the operator should be part of the cache key, but not other stuff. aggregators is just some extra information that the HavingSpec uses to do its job.
|
Labeled "Incompatible" due to adding Cacheable interface to HavingSpec. |
| @Override | ||
| public byte[] computeResultLevelCacheKey(SegmentMetadataQuery query) | ||
| { | ||
| return computeCacheKey(query); |
There was a problem hiding this comment.
This also needs to include "merge" and "lenientAggregatorMerge". They're not part of the segment-level cache key, but they should be part of the result-level key.
There was a problem hiding this comment.
ok, added those to result level cache key.
| .appendCacheable(query.getVirtualColumns()) | ||
| .appendCacheables(query.getPostAggregatorSpecs()) | ||
| .appendInt(query.getLimit()); | ||
| if (query.isGrandTotal()) { |
There was a problem hiding this comment.
This might as well just be .appendBoolean(query.isGrandTotal()), and is probably better, since there's less of a need to make sure that things we add after it will mesh well.
| .appendCacheable(query.getHavingSpec()) | ||
| .appendCacheable(query.getLimitSpec()); | ||
|
|
||
| if (!query.getPostAggregatorSpecs().isEmpty()) { |
There was a problem hiding this comment.
Might as well add the post-aggregator specs unconditionally.
| .appendCacheable(query.getVirtualColumns()); | ||
|
|
||
| final List<PostAggregator> postAggregators = query.getPostAggregatorSpecs(); | ||
| if (!postAggregators.isEmpty()) { |
There was a problem hiding this comment.
Might as well add the post-aggregator specs unconditionally here, too.
| @@ -385,6 +385,7 @@ private String computeCurrentEtag(final Set<ServerToSegment> segments, @Nullable | |||
| break; | |||
| } | |||
| hasher.putString(p.getServer().getSegment().getId().toString(), StandardCharsets.UTF_8); | |||
| hasher.putString(p.rhs.getInterval().toString(), StandardCharsets.UTF_8); | |||
There was a problem hiding this comment.
Comment here would be nice, explaining that this interval is the query interval, not the segment interval, and it's important for it to be part of the cache key.
* Add SegmentDescriptor interval in the hash while calculating Etag * Add computeResultLevelCacheKey to CacheStrategy Make HavingSpec cacheable and implement getCacheKey for subclasses Add unit tests for computeResultLevelCacheKey * Add more tests * Use CacheKeyBuilder for HavingSpec's getCacheKey * Initialize aggregators map to avoid NPE * adjust cachekey builder for HavingSpec to ignore aggregators * unused import * PR comments
* Add SegmentDescriptor interval in the hash while calculating Etag * Add computeResultLevelCacheKey to CacheStrategy Make HavingSpec cacheable and implement getCacheKey for subclasses Add unit tests for computeResultLevelCacheKey * Add more tests * Use CacheKeyBuilder for HavingSpec's getCacheKey * Initialize aggregators map to avoid NPE * adjust cachekey builder for HavingSpec to ignore aggregators * unused import * PR comments
Addresses #7302
Added
SegmentDescriptor'sIntervalto the Etag encoding so that same query with different intervals will have non-equal Etags, and query would not return incorrectly cached results.Added a new method
computeResultLevelCacheKeyin theCacheStrategyfor computing result-level cache keys.Made HavingSpec cacheable and implemented
getCacheKeyfor subclassesAdded unit tests for
computeResultLevelCacheKey