[GitHub] [druid] paul-rogers commented on a diff in pull request #13893: Fix for OOM in the Tombstone generating logic in MSQ

["paul-rogers (via GitHub)" <gi...@apache.org>]

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: commits-unsubscribe@druid.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org