https://linen.dev logo
Join Slack
Powered by
Hmmm…..this might be a regression from <https://gi...
# dev
m
Hmmm…..this might be a regression from https://github.com/apache/druid/commit/c353ccfdeff39dae99e1ebe5ce34bbacc7dd083f#diff-1aa22bcd62ddd4457f128832de7[…]954d015d8b3be6a03e3b3471aad6e3R57 So….seems like if we have a COMPLEX column, the complex column can no longer be an input for numeric aggregator like doubleSum in the sql compatibility mode. In the sql compatibility mode, it will wrap the COMPLEX column selector (ObjectColumnSelector) in a NullableNumericBufferAggregator and then this call: https://github.com/apache/druid/blob/master/processing/src/main/java/org/apache/druid/query/aggregation/NullableNumericBufferAggregator.java#L70 would fails.
Any way we can plug our own ColumnValueSelector in makeMetricColumnValueSelector of the IncrementalIndex for COMPLEX?
i.e. ObjectMetricColumnSelector doesn’t allow getDouble, etc but some COMPLEX like SpectatorHistogram needs those functions
g
do you have a test case that shows the problem?
hmm. reading through the 
SpectatorHistogramAggregator
code i think i see what you are talking about. it's a complex type, but it has 
isNull
and 
getLong
methods defined that do kind of allow it to act as the basis for a numeric selector
also 
SpectatorHistogram
extends 
Number
, so the result of 
get
can also be interpreted numerically
i don't know of anything else that acts this way (i.e. a complex type that can act as a number), so we probably didn't have tests for it
probably the best place for a test would be in 
SpectatorHistogramAggregatorTest
, extending it to use both 
IncrementalIndexSegment
as well as 
QueryableIndexSegment
the old behavior is documented in 
spectator-histogram.md
so we should fix this so it works
i can think of three approaches 1) restore the old logic in 
ObjectColumnSelector
2) move the logic to a new subclass of 
ObjectColumnSelector
used by 
IncrementalIndex
for 
COMPLEX
(i.e., if the object is an instance of 
Number
, the selector has numeric methods that "work") 3) move the logic into the aggregators, so they check if their input is type 
COMPLEX
, and if so, they call 
getObject
and check if the returned thing is 
Number
@Maytas Monsereenusorn & @Zoltan Haindrich - wondering if you have any preferences?
personally i'm partial to (3) since it keeps the selectors cleaner
z
I feel like having an aggregator which could return different things for 
get
and 
getLong
will probably hit back later - so I think this should be change to some other approach there's also some comment about this [here](https://github.com/apache/druid/pull/15371#issuecomment-2145013508) I'm not sure - but I hope that for (2) possibly [ObjectBasedColumnSelector](https://github.com/apache/druid/blob/master/processing/src/main/java/org/apache/druid/segment/ObjectBasedColumnSelector.java) could be utilized I was thinking to move the non-object related stuff into some post-agg functions a testcase would really help me understand how this supposed to work...
b
There are [existing tests](https://github.com/apache/druid/blob/master/extensions-contrib/spectator-histogram[…]druid/spectator/histogram/SpectatorHistogramAggregatorTest.java) that test the longSum functionality, but not specifically for IncrementalIndex. We have a test locally that repros the issue by using IncrementalIndex. We can put that in a branch to share it.
g
a draft PR with a failing test would be a good way to share it (CI will run and show the failure)
i haven't added any new tests; @Ben Sykes or @Maytas Monsereenusorn if you have a test that fails on master, lmk and let's try it on this branch
m
Copy code
@Test
  public void testBuildingAndCountingHistogramsIncrementalIndex() throws Exception
  {
    List<String> dimensions = Collections.singletonList("d");
    int n = 10;
    List<InputRow> inputRows = new ArrayList<>(n);
    for (int i = 1; i <= n; i++) {
      String val = String.valueOf(i * 1.0d);

      inputRows.add(new MapBasedInputRow(
          DateTime.now(DateTimeZone.UTC),
          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.ALL)
        .setInterval("1970/2050")
        .setAggregatorSpecs(
            new DoubleSumAggregatorFactory("doubleSum", "histogram")
        ).build();

    Sequence<ResultRow> seq = helper.runQueryOnSegmentsObjs(segments, query);

    List<ResultRow> results = seq.toList();
    Assert.assertEquals(1, results.size());
    // Check doubleSum
    Assert.assertEquals((double) n * segments.size(), results.get(0).get(0));
  }
That should reproduce the issue….
b
g
PR with your test & approach (3) for a fix: https://github.com/apache/druid/pull/16564
z
I still don't understand why can't this be done with simple function which translates the sketch to the literal number....why the need to do this behind the scenes
b
I don’t see this as “behind the scenes”. The data is representing a Histogram. The “sum of a histogram” typically refers to the total count of all the data points represented in the histogram. In other words, it’s the sum of the frequencies (or counts) of all the bins in the histogram. Which is what we’re producing here when queried as a sum. To introduce a separate aggregator would add unnecessary indirection in the queries and duplicate the logic that the NullableNumericBufferAggregator is using.
z
yes - but there are only just a handfull of aggregations which could be meaningfully flattened to a number
I don't say a new aggregator - a new function for the Histogram -> literal conversion; if the sketch would be a say complex-numbers: any of the following complex-number -> literal functions are valid: length, real part, imaginary part
g
my 2¢— design-wise i would have liked having a dedicated function to extract the count from a histogram, rather than it being implicit in feeding a histogram to 
longSum
or 
doubleSum
. this is mostly for alignment with how it'd need to be exposed in sql (most of our users use sql so that's the main way i think about querying). in sql, you can't do 
SUM(histogram)
since it wouldn't validate: 
SUM
is a standard function that takes standard types. it would instead be something like 
SUM(SPECTATOR_HISTOGRAM_COUNT(histogram))
. the native query version of that would be an expression function like 
spectator_histogram_count
that could go in the 
expression
parameter of 
longSum
or 
doubleSum
. but since it's already released with docs that say you can use 
longSum
or 
doubleSum
directly (without an expression), i lean towards fixing things such that the docs are true (rather than changing the design and changing the docs)
z
ok - I was playing a bit to show what I was thinking about ; I think a postagg could possibly work - however I've seen double the count values ; I was not able to identify where that comes from ; but 2 SpectatorHistograms were merged which were both represented the full dataset of the test -> so the result became 20 https://github.com/kgyrtkirk/druid/pull/144/files#diff-320ece53028a25c01e11f200f6066ed2b8811b99db4271723456f6a686c2d8eeR418-R428
b
Thanks for the context both. I had hoped that “being a number” would mean things like this would “just work”. But it seems not. 
Cannot apply 'SUM' to arguments of type 'SUM(<COMPLEX<SPECTATORHISTOGRAMDISTRIBUTION>>)'. Supported form(s): 'SUM(<NUMERIC>)'
I still like that in native queries we can use 
longSum
directly without having to care to add a specific aggregator. Would it ever be possible to use a plain 
SUM
with columns like this?
The postagg looks like it would be less efficient. It’s first going to have to combine all histograms then take the sum. We can get the sum of each and combine that much for efficiently.
m
The postagg would also requires our users to change their query right (to add the postagg)
g
to be of equal efficiency we'd need a function call (i.e. an implementation of 
ExprMacro
). actually to expose in SQL, it's more important to have an 
ExprMacro
than a 
PostAggregator
anyway. post-aggs can only be used in a specific spot (post aggregation 😉) but exprs can be used anywhere (filtering, pre-agg, post-agg, join conditions, etc)
anyone available to review my more stop-gap patch here? https://github.com/apache/druid/pull/16564 it should preserve behavior from the existing docs. if anyone's inspired to add additional functions later on, that would be great, but IMO that would be a separate effort.
m
Will do! Thanks
2 Views
Open in Slack
PreviousNext
Powered by