kfaraz commented on code in PR #14475:
URL: https://github.com/apache/druid/pull/14475#discussion_r1240030795
##########
extensions-core/multi-stage-query/src/main/java/org/apache/druid/msq/util/IntervalUtils.java:
##########
@@ -61,4 +64,20 @@ public static List<Interval> difference(final List<Interval>
list1, final List<I
return retVal;
}
+
+ /**
+ * This method checks if the provided interval is aligned by the granularity
provided and if it occupies exactly
+ * one "unit" of the granularity
+ * Note: ALL granularity isn't aligned to any interval, however this method
is defines that ALL granularity matches
+ * an interval with boundary ({@code DateTimes.MIN}, {@code DateTimes.MAX})
+ */
+ public static boolean doesIntervalMatchesGranularity(final Interval
interval, final Granularity granularity)
Review Comment:
Nit: `align` probably suits better.
```suggestion
public static boolean isAlignedWithGranularity(final Interval interval,
final Granularity granularity)
```
##########
extensions-core/multi-stage-query/src/main/java/org/apache/druid/msq/util/IntervalUtils.java:
##########
@@ -61,4 +64,20 @@ public static List<Interval> difference(final List<Interval>
list1, final List<I
return retVal;
}
+
+ /**
+ * This method checks if the provided interval is aligned by the granularity
provided and if it occupies exactly
+ * one "unit" of the granularity
+ * Note: ALL granularity isn't aligned to any interval, however this method
is defines that ALL granularity matches
+ * an interval with boundary ({@code DateTimes.MIN}, {@code DateTimes.MAX})
+ */
+ public static boolean doesIntervalMatchesGranularity(final Interval
interval, final Granularity granularity)
+ {
+ // AllGranularity needs special handling since AllGranularity#bucketStart
always returns false
+ if (granularity instanceof AllGranularity) {
+ return (interval.getStartMillis() == DateTimes.MIN.getMillis())
+ && (interval.getEndMillis() == DateTimes.MAX.getMillis());
Review Comment:
```suggestion
return Intervals.isEternity(interval);
```
##########
extensions-core/multi-stage-query/src/main/java/org/apache/druid/msq/util/IntervalUtils.java:
##########
@@ -61,4 +64,20 @@ public static List<Interval> difference(final List<Interval>
list1, final List<I
return retVal;
}
+
+ /**
Review Comment:
Maybe add this method to `Intervals` class instead, as it could have uses
outside the MSQ extension too.
##########
extensions-core/multi-stage-query/src/test/java/org/apache/druid/msq/exec/MSQFaultsTest.java:
##########
@@ -74,6 +76,36 @@ public void testInsertCannotAllocateSegmentFault()
.verifyResults();
}
+ @Test
+ public void testInsertCannotAllocateSegmentFaultWhenInvalidAllocation()
+ {
+ RowSignature rowSignature = RowSignature.builder()
+ .add("__time", ColumnType.LONG)
+ .add("dim1", ColumnType.STRING)
+ .add("cnt",
ColumnType.LONG).build();
+
+ // If there is some problem allocating the segment,task action client will
return a null value.
+ Mockito.doReturn(new SegmentIdWithShardSpec(
+ "foo1",
+ Intervals.of("2000-01-01/2000-02-01"),
+ "test",
+ new LinearShardSpec(2)
+ )).when(testTaskActionClient).submit(isA(SegmentAllocateAction.class));
+
+ testIngestQuery().setSql(
+ "insert into foo1 select __time, dim1 , count(*) as
cnt from foo where dim1 is not null and __time >= TIMESTAMP '2000-01-02
00:00:00' and __time < TIMESTAMP '2000-01-03 00:00:00' group by 1, 2
PARTITIONED by day clustered by dim1")
Review Comment:
Nit: Break up the sql into multiple lines for readability
```suggestion
String sql = "insert into foo1"
+ " select __time, dim1 , count(*) as cnt from foo"
+ " where dim1 is not null"
+ " and __time >= TIMESTAMP '2000-01-02 00:00:00'"
+ " and __time < TIMESTAMP '2000-01-03 00:00:00'"
+ " group by 1, 2 PARTITIONED by day clustered by
dim1";
testIngestQuery().setSql(sql)
```
##########
extensions-core/multi-stage-query/src/main/java/org/apache/druid/msq/exec/ControllerImpl.java:
##########
@@ -936,7 +936,12 @@ private List<SegmentIdWithShardSpec>
generateSegmentIdsWithShardSpecsForAppend(
}
}
- if (allocation == null) {
+ // Even if allocation isn't null, the overlord makes the best effort job
of allocating a segment with the given
+ // segmentGranularity. This is commonly seen in case when there is
already a coarser segment in the interval where
+ // the requested segment is present and that segment completely overlaps
the request. Throw an error if the interval
+ // doesn't match the granularity requested
+ if (allocation == null
+ ||
!IntervalUtils.doesIntervalMatchesGranularity(allocation.getInterval(),
segmentGranularity)) {
Review Comment:
I think the error message should be different in the null case and the
unexpected interval case.
##########
extensions-core/multi-stage-query/src/main/java/org/apache/druid/msq/util/IntervalUtils.java:
##########
@@ -61,4 +64,20 @@ public static List<Interval> difference(final List<Interval>
list1, final List<I
return retVal;
}
+
+ /**
+ * This method checks if the provided interval is aligned by the granularity
provided and if it occupies exactly
+ * one "unit" of the granularity
+ * Note: ALL granularity isn't aligned to any interval, however this method
is defines that ALL granularity matches
+ * an interval with boundary ({@code DateTimes.MIN}, {@code DateTimes.MAX})
+ */
+ public static boolean doesIntervalMatchesGranularity(final Interval
interval, final Granularity granularity)
+ {
+ // AllGranularity needs special handling since AllGranularity#bucketStart
always returns false
+ if (granularity instanceof AllGranularity) {
+ return (interval.getStartMillis() == DateTimes.MIN.getMillis())
+ && (interval.getEndMillis() == DateTimes.MAX.getMillis());
+ }
+ return granularity.isAligned(interval) &&
granularity.bucketEnd(interval.getStart()).equals(interval.getEnd());
Review Comment:
I think just `isAligned` should be enough. This is the code for
`PeriodGranularity.isAligned`:
```
@Override
public boolean isAligned(Interval interval)
{
return bucket(interval.getStart()).equals(interval);
}
```
--
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]
