Improper topN caching when sorting by postaggregator

[gianm]

When a topN is sorting by a postaggregator, the postaggregator name is part of the query cache key but the definition is not. But, the postaggregator should be part of the cache key too, since it's used to determine which result rows to keep for a particular segment in the cache. If the postaggregator is redefined but keeps the same name, the query improperly uses previously cached results.

I think fixing this for real will require adding a getCacheKey method to PostAggregators.

If we want to hold off on that until 0.10.0 then we could work around it in the meantime by disabling caching for any topN query that sorts on a postaggregator. But this has a performance cost that could hit more people than the bug does.

[drcrallen]

Is this a "forever" kind of bug, or a regression?

[fjy]

@drcrallen forever

[drcrallen]

Disabling caching seems a little heavy handed unless there's a way to enable the old behavior where postagg sorted topN's can be cached.

I suggest making this a 0.10.0 ticket with the solution of adding getCacheKey to post aggregators, and having any "temporary" solution be a different ticket.

[fjy]

+1 on disabling caching on topNs sorted by post agg and fixing as part of 0.10.0

[nishantmonu51]

One more way to fix this in 0.9 can be to add another Interface CacheablePostAggregator that defines getCacheKey and have all the internal post aggs impls use it. Now in the query If the post agg is cacheable we cache the query otherwise its not cached. I think this will solve the issue without impacting performance for most of the use cases.
Only performance impact will be for users who have defined custom post aggs via extensions, which they can work around by modifying their impls to make it cacheable.

[gianm]

That solution sounds workable @nishantmonu51. What do you think about a Cacheable interface instead, so people can do implements PostAggregator, Cacheable? This way, in 0.10.0 if we make the PostAggregator interface extend Cacheable, old source files will continue to work, and we won't have an unused "CacheablePostAggregator" interface floating around.

[nishantmonu51]

@gianm: sounds good.

[gianm]

Tagging incompatible since odds are we will fix this by changing PostAggregator.

[jihoonson]

Thanks @gianm and @nishantmonu51 for the guide. I'm working on this.

[gianm]

@jihoonson since 0.10.0 allows incompatible changes, imo the best way to address this is just adding a getCacheKey method to the PostAggregator interface.

[jihoonson]

@gianm, thanks. I'll focus on that point.