[GitHub] [druid] gianm commented on a diff in pull request #14136: Improved handling for zero-length intervals.

["gianm (via GitHub)" <gi...@apache.org>]

gianm commented on code in PR #14136:
URL: https://github.com/apache/druid/pull/14136#discussion_r1174236443


##########
processing/src/main/java/org/apache/druid/java/util/common/JodaUtils.java:
##########
@@ -69,30 +71,39 @@ public static List<Interval> condenseIntervals(Iterable<Interval> intervals)
   }
 
   /**
-   * This method does not materialize the intervals represented by the
-   * sortedIntervals iterator. However, caller needs to insure that sortedIntervals
-   * is already sorted in ascending order (use the Comparators.intervalsByStartThenEnd()).
-   * It avoids materialization by incrementally condensing the intervals by
+   * Condense the provided intervals such that the returned list has no abutting intervals, no overlapping intervals,
+   * and no zero-length intervals.
+   * <p>
+   * The provided intervals must be sorted in ascending order using {@link Comparators#intervalsByStartThenEnd()}.
+   * Otherwise, results are undefined.
+   * <p>
+   * The method avoids materialization by incrementally condensing the intervals by
    * starting from the first and looking for "adjacent" intervals. This is
    * possible since intervals in the Iterator are in ascending order (as
    * guaranteed by the caller).
-   * <p>
-   * *
    *
    * @param sortedIntervals The iterator object containing the intervals to condense
+   *
    * @return An iterator for the condensed intervals. By construction the condensed intervals are sorted
    * in ascending order and contain no repeated elements. The iterator can contain nulls,
    * they will be skipped if it does.
+   *
    * @throws IAE if an element is null or if sortedIntervals is not sorted in ascending order
    */
   public static Iterator<Interval> condensedIntervalsIterator(Iterator<Interval> sortedIntervals)
   {
-
     if (sortedIntervals == null || !sortedIntervals.hasNext()) {
       return Collections.emptyIterator();
     }
 
-    final PeekingIterator<Interval> peekingIterator = Iterators.peekingIterator(sortedIntervals);
+    final PeekingIterator<Interval> peekingIterator =
+        Iterators.peekingIterator(
+            Iterators.filter(
+                sortedIntervals,
+                interval -> interval == null || interval.toDurationMillis() > 0

Review Comment:
   Good question 🙂
   
   I'm not sure, but, there is a test case that validates that we do retain them. I figured someone added the test case for a reason.



-- 
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