kgyrtkirk commented on code in PR #16562:
URL: https://github.com/apache/druid/pull/16562#discussion_r1628970678
##########
extensions-contrib/spectator-histogram/src/test/java/org/apache/druid/spectator/histogram/SpectatorHistogramAggregatorTest.java:
##########
@@ -365,6 +383,60 @@ public void testBuildingAndCountingHistograms() throws
Exception
Assert.assertEquals(9.0, (Double) results.get(0).get(1), 0.001);
}
+ @Test
+ public void testBuildingAndCountingHistogramsIncrementalIndex() throws
Exception
+ {
+ NullHandling.initializeForTestsWithValues(true, true);
+ List<String> dimensions = Collections.singletonList("d");
+ int n = 10;
+ DateTime startOfDay =
DateTime.now(DateTimeZone.UTC).withTimeAtStartOfDay();
+ List<InputRow> inputRows = new ArrayList<>(n);
+ for (int i = 1; i <= n; i++) {
+ String val = String.valueOf(i * 1.0d);
+
+ inputRows.add(new MapBasedInputRow(
+ startOfDay.plusMinutes(i),
+ dimensions,
+ ImmutableMap.of("x", i, "d", val)
+ ));
+ }
+
+ IncrementalIndex index = AggregationTestHelper.createIncrementalIndex(
+ inputRows.iterator(),
+ new NoopInputRowParser(null),
+ new AggregatorFactory[]{
+ new CountAggregatorFactory("count"),
+ new SpectatorHistogramAggregatorFactory("histogram", "x")
+ },
+ 0,
+ Granularities.NONE,
+ 100,
+ false
+ );
+
+ ImmutableList<Segment> segments = ImmutableList.of(
+ new IncrementalIndexSegment(index, SegmentId.dummy("test")),
+ helper.persistIncrementalIndex(index, null)
+ );
+
+ GroupByQuery query = new GroupByQuery.Builder()
+ .setDataSource("test")
+ .setGranularity(Granularities.HOUR)
+ .setInterval("1970/2050")
+ .setAggregatorSpecs(
+ new DoubleSumAggregatorFactory("doubleSum", "histogram")
Review Comment:
I don't really see why this is beneficial at all - why can't the user invoke
a method which will translate the histogram to a literal and the aggregate
those values with sum?
I think an aggregate which is repesented by a sketch or something could
normally not be represented by a single literal number; if we want to introduce
something which translates the sketch to some other type automatically - I
think that should be registered properly and not hidden in the aggregator as
some bonus functionality.
am I alone with this mindset?
--
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]
