More methods in QueryMetrics and TopNQueryMetrics (the last part of #3798)#4284
More methods in QueryMetrics and TopNQueryMetrics (the last part of #3798)#4284drcrallen merged 3 commits intoapache:masterfrom
Conversation
| Granularity gran, | ||
| boolean descending | ||
| boolean descending, | ||
| @Nullable QueryMetrics<?> queryMetrics |
There was a problem hiding this comment.
instead of putting null everywhere, can you make a backwards compatible method signature and just @Deprecated it?
There was a problem hiding this comment.
@drcrallen null queryMetrics is allowed not for backward compatibility, it is valid, means that the caller doesn't want or couldn't record metrics. OTOH updated signatures are not in the interfaces/classes which are extension points, so adding "backwards compatible" deprecated methods is pointless.
| /** | ||
| * {@link #getBitmapIndex} and {@link #getBitmapResult} methods both have default implementations, delegating to each | ||
| * other. Every implementation of {@link Filter} should override {@link #getBitmapResult}, currently it has a default | ||
| * implementation for compatibility with Filters in extensions. In Druid 0.11 {@link #getBitmapResult} is going to |
There was a problem hiding this comment.
hmmm... I'm not sure if we have a good way to "not forget about" this intention, can you file an issue related to this with a 0.11 tag?
| return getBitmapResult(selector, new DefaultBitmapResultFactory(selector.getBitmapFactory())); | ||
| } | ||
|
|
||
| default <T> T getBitmapResult(BitmapIndexSelector selector, BitmapResultFactory<T> bitmapResultFactory) |
There was a problem hiding this comment.
Is it appropriate to mark this one @Deprecated so that any usage gets the notice about the intended 0.11 change?
There was a problem hiding this comment.
@drcrallen probably you meant this comment on getBitmapIndex(), but even getBitmapIndex() is not deprecated for usage, for those who don't need BitmapResult thing. getBitmapIndex() is deprecated for overriding.
|
so if I understand this correctly, the key change here is the change from using different creation methods for bitmap results to using a unified |
Yes.
Just add methods, let custom |
|
Design/code looks good to me, 👍 @drcrallen did you have any other feedback on this? I also noticed a couple of conflicts with #4288 (in DefaultQueryMetrics and DefaultTopNQueryMetrics) when applying this patch locally, not sure why they don't show up here... |
|
Thanks @jon-wei. The master branch merged cleanly. |
3 participants
QueryMetricsandTopNQueryMetricsand call them fromTopNQueryEngine. When somebody needs the hooks added toQueryMetricsfor other query types, corresponding query engine(s) could be updated to call the hooks, in another PR.BitmapResultFactory, an abstraction that wrapsBitmapFactoryand provides ability to record the bitmap construction "form" (hierarchical tree of bitmap unions, intersections, and complements) and report it via a query metric dimension. It was mentioned here: Don't union with empty bitmap in the end in Filters.matchPredicate() #3754 (comment)This PR is the last part of #3798.