abhishekrb19 commented on code in PR #16324:
URL: https://github.com/apache/druid/pull/16324#discussion_r1576913817
##########
server/src/test/java/org/apache/druid/segment/indexing/granularity/UniformGranularityTest.java:
##########
@@ -358,6 +363,54 @@ public void
testUniformGranularitySpecWithLargeNumberOfIntervalsDoesNotBlowUp()
Assert.assertEquals(3600 * 24 * 365 * 10 + 3 * 24 * 3600, count);
}
+
+ @Test
+ public void testSegmentGranularityEqualToQueryGranularity()
+ {
+ final Granularity secondGranularity = Granularities.SECOND;
+ final Granularity secondDurationGranularity = new
DurationGranularity(1000, 0);
+ try {
+ new UniformGranularitySpec(
+ secondGranularity,
+ secondGranularity,
+ Collections.emptyList()
+ );
+ new UniformGranularitySpec(
+ secondGranularity,
+ secondDurationGranularity,
+ Collections.emptyList()
+ );
+ }
+ catch (DruidException e) {
Review Comment:
We can simply validate that the instantiation was successful by asserting
that the query and segment granularities are equal in this case.
Or do a `catch (Exception e)`; otherwise, if someone changes the type of
exception thrown, this test will still pass.
I think the fist approach would be cleaner.
##########
server/src/main/java/org/apache/druid/segment/indexing/granularity/UniformGranularitySpec.java:
##########
@@ -45,6 +47,16 @@ public UniformGranularitySpec(
super(inputIntervals, rollup);
this.queryGranularity = queryGranularity == null ?
DEFAULT_QUERY_GRANULARITY : queryGranularity;
this.segmentGranularity = segmentGranularity == null ?
DEFAULT_SEGMENT_GRANULARITY : segmentGranularity;
+ if (Granularity.IS_FINER_THAN.compare(this.segmentGranularity,
this.queryGranularity) < 0) {
+ throw DruidException.forPersona(DruidException.Persona.USER)
+ .ofCategory(DruidException.Category.INVALID_INPUT)
+ .withErrorCode("invalidInput")
+ .build(
+ "SegmentGranularity[%s] must not be finer than
QueryGranularity[%s].",
+ this.segmentGranularity,
+ this.queryGranularity
+ );
Review Comment:
```suggestion
throw InvalidInput.exception(
"segmentGranularity[%s] must not be finer than
queryGranularity[%s].",
this.segmentGranularity, this.queryGranularity
);
```
##########
server/src/main/java/org/apache/druid/segment/indexing/granularity/UniformGranularitySpec.java:
##########
@@ -45,6 +47,15 @@ public UniformGranularitySpec(
super(inputIntervals, rollup);
this.queryGranularity = queryGranularity == null ?
DEFAULT_QUERY_GRANULARITY : queryGranularity;
this.segmentGranularity = segmentGranularity == null ?
DEFAULT_SEGMENT_GRANULARITY : segmentGranularity;
+ if (Granularity.IS_FINER_THAN.compare(this.segmentGranularity,
this.queryGranularity) < 0) {
+ throw DruidException.forPersona(DruidException.Persona.USER)
+ .ofCategory(DruidException.Category.UNSUPPORTED)
+ .build(
+ "SegmentGranularity[%s] must not be finer than
QueryGranularity[%s].",
+ this.segmentGranularity,
+ this.queryGranularity
+ );
Review Comment:
Ah, sorry, I meant capitalization of `segmentGranularity` vs
`SegmentGranularity`. Ditto for `queryGranularity`. Basically json annotations
for the properties as they would be specified in the spec. Please see related
suggestion
--
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]
