jihoonson commented on a change in pull request #9905:
URL: https://github.com/apache/druid/pull/9905#discussion_r436075797
##########
File path:
indexing-service/src/main/java/org/apache/druid/indexing/common/task/CompactionTask.java
##########
@@ -710,20 +728,8 @@ private static DimensionsSpec
createDimensionsSpec(List<Pair<QueryableIndex, Dat
// Dimensions are extracted from the recent segments to olders because
recent segments are likely to be queried more
// frequently, and thus the performance should be optimized for recent
ones rather than old ones.
- // timelineSegments are sorted in order of interval, but we do a sanity
check here.
- final Comparator<Interval> intervalComparator =
Comparators.intervalsByStartThenEnd();
- for (int i = 0; i < queryableIndices.size() - 1; i++) {
- final Interval shouldBeSmaller =
queryableIndices.get(i).lhs.getDataInterval();
- final Interval shouldBeLarger = queryableIndices.get(i +
1).lhs.getDataInterval();
- Preconditions.checkState(
- intervalComparator.compare(shouldBeSmaller, shouldBeLarger) <= 0,
- "QueryableIndexes are not sorted! Interval[%s] of segment[%s] is
laster than interval[%s] of segment[%s]",
- shouldBeSmaller,
- queryableIndices.get(i).rhs.getId(),
- shouldBeLarger,
- queryableIndices.get(i + 1).rhs.getId()
- );
- }
+ // sort timelineSegments in order of interval.
+ queryableIndices.sort((o1, o2) ->
Comparators.intervalsByStartThenEnd().compare(o1.rhs.getInterval(),
o2.rhs.getInterval()));
Review comment:
@yuanlihan thanks for the explanation! What this method does is finding
the most reasonable order of dimensions when input segments have mixed orders.
The current algorithm is assuming that more recent segments will likely have
the order what you want more (there would be some reason if you have changed
the dimension order at some point). Maybe a potential improvement can be
considering the segment version as well if some segment intervals are
overlapped, but I think this could be done in a separate PR.
> Also I found that now timeline already has unit tests about sorting
timelineObjectHolders and the sanity check will pass in most case.
BTW, out of curiosity, can you point me out where this test is?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
