You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@druid.apache.org by "kfaraz (via GitHub)" <gi...@apache.org> on 2023/06/23 17:56:00 UTC
[GitHub] [druid] kfaraz commented on a diff in pull request #14475: [MSQ] Confirm that the allocated segment's granularity matches the requested granularity
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: 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