Remove IncrementalIndex template modifier#11160
Conversation
| } | ||
|
|
||
| @Override | ||
| public Iterable<Row> iterableWithPostAggregations( |
There was a problem hiding this comment.
Can you explain the reason for moving this method from IncrementalInde to here and OffheapIncrementalIndex?
There was a problem hiding this comment.
When this method was in the generic IncrementalIndex, it has to use the generic AggregatorType by calling the following abstract generic functions:
AggregatorType[] getAggsForRow(int rowOffset);
Object getAggVal(AggregatorType agg, int rowOffset, int aggPosition);These functions were forced and didn't really fit each of the implementations.
getAggsForRow() didn't fit OffheapIncrementalIndex since it only have one aggregator instance (not one for each row).
getAggVal() didn't fit OnheapIncrementalIndex since rowOffset and aggPosition is only relevant to BufferAggregator.
That said, other than that, the implementation is identical in both incremental-index implementations.
It might be possible to find a more generic implementation. But since we are removing OffheapIncrementalIndex, I don't see the point of making this effort here.
Once this PR and PR #11124 will be merged, I will try to generify this function for both OnheapIncrementalIndex and OakIncrementalIndex (#10001).
There was a problem hiding this comment.
I see. Thank you for the explanation.
# Conflicts: # processing/src/main/java/org/apache/druid/segment/incremental/IncrementalIndex.java # processing/src/main/java/org/apache/druid/segment/incremental/OffheapIncrementalIndex.java
|
@jihoonson I updated this PR after the merger of #11124. |
jihoonson
left a comment
There was a problem hiding this comment.
LGTM. Thanks @liran-funaro.
| } | ||
|
|
||
| @Override | ||
| public Iterable<Row> iterableWithPostAggregations( |
There was a problem hiding this comment.
I see. Thank you for the explanation.
This patch removes the template modifier from
IncrementalIndex<AggregatorType>. It is no longer required:private final AggregatorType[] aggsmoved toOffheapIncrementalIndex(it is only used there).iterableWithPostAggregations()defined asabstractand implemented inOnheapIncrementalIndex/OffheapIncrementalIndex.initAggs()returnsvoid.Rationale
It is not required. Especially since
OffheapIncrementalIndexwill be removed by #11124.