paul-rogers commented on code in PR #13893:
URL: https://github.com/apache/druid/pull/13893#discussion_r1128459766
##########
indexing-service/src/main/java/org/apache/druid/indexing/common/task/batch/parallel/TombstoneHelper.java:
##########
@@ -199,26 +195,34 @@ public Set<Interval> computeTombstoneIntervalsForReplace(
}
// Overlap might not be aligned with the granularity if the used
interval is not aligned with the granularity
- // However when fetching from the iterator, the first interval is
found using the bucketStart, which
- // ensures that the interval is "rounded down" to the first timestamp
that aligns with the granularity
- // Also, the interval would always be contained inside the
"intervalToDrop" because the original REPLACE
- // is aligned by the granularity, and by extension all the elements
inside the intervals to drop would
- // also be aligned by the same granularity (since intervalsToDrop =
replaceIntervals - publishIntervals, and
- // the right-hand side is always aligned)
- //
+ // However we align the boundaries manually, in the following code.
+
+ // If the start is aligned, then bucketStart is idempotent, else it
will return the latest timestamp less than
+ // overlap.getStart() which aligns with the replace granularity. That
extra interval that we are including
+ // before the overlap should be contained in intervalToDrop because
intervalToDrop is aligned by the
+ // replaceGranularity, and the overlap's beginning would always be
later than intervalToDrop (trivially,
+ // because its the overlap) and if bucketStart floors the overlap
beginning, it cannot floor it before
+ // the intervalToDrop's start
+ DateTime alignedIntervalStart =
replaceGranularity.bucketStart(overlap.getStart());
+
// For example, if the replace granularity is DAY, intervalsToReplace
are 20/02/2023 - 24/02/2023 (always
// aligned with the replaceGranularity), intervalsToDrop are
22/02/2023 - 24/02/2023 (they must also be aligned with the replaceGranularity)
// If the relevant usedIntervals for the datasource are from
22/02/2023 01:00:00 - 23/02/2023 02:00:00, then
// the overlap would be 22/02/2023 01:00:00 - 23/02/2023 02:00:00.
When iterating over the overlap we will get
// the intervals from 22/02/2023 - 23/02/2023, and 23/02/2023 -
24/02/2023
- IntervalsByGranularity intervalsToDropByGranularity = new
IntervalsByGranularity(
- ImmutableList.of(overlap),
- replaceGranularity
- );
-
- // Helps in deduplication if required. Since all the intervals are
uniformly granular, there should be no
- // no overlap post deduplication
-
retVal.addAll(Sets.newHashSet(intervalsToDropByGranularity.granularityIntervalsIterator()));
+
+ // If the end is aligned, then we donot alter it, else we align the
end by geting the earliest time later
Review Comment:
```suggestion
// If the end is aligned, then we do not alter it, else we align the
end by geting the earliest time later
```
##########
indexing-service/src/test/java/org/apache/druid/indexing/common/task/batch/parallel/TombstoneHelperTest.java:
##########
@@ -147,7 +147,7 @@ public void
tombstoneIntervalsCreatedForReplaceWhenReplaceIsContainedInUsedInter
replaceGranularity
);
Assert.assertEquals(
- ImmutableSet.of(Intervals.of("2020-03-05/2020-03-06"),
Intervals.of("2020-03-06/2020-03-07")),
+ ImmutableSet.of(Intervals.of("2020-03-05/2020-03-07")),
Review Comment:
Are the tests complete? That is, do we have, say replace a day into ALL?
Replace a day into YEAR with:
* Day as Jan 1. (I.e. one tombstone on the right)
* Day as Feb. 1. (Two tombstones)
* Day as Dec. 31. (One tombstone on the left)
Do we have a case for the non-aligned case? Original is month, target is
week. (Although, I know we don't want to support week. It is still a useful
non-aligned test case.)
--
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.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
