kgyrtkirk commented on PR #15371: URL: https://github.com/apache/druid/pull/15371#issuecomment-2145013508
I think it would be better to not provide a sketch and literal values at the same time from the same [aggregator](https://github.com/apache/druid/blob/1974a38bc90783e7ff2d38362509377001177047/extensions-contrib/spectator-histogram/src/main/java/org/apache/druid/spectator/histogram/SpectatorHistogramAggregator.java#L75-L85). I'm a little bit unsure if that is a recommended way to do things - but @gianm might know better. I also suspect that these aggregators will not work well with the windowed codepaths - and in case of a windowed `sum` a `ClassCastException` would be thrown. I was not able to uncover why the need for `spectatorHistogramTimer` and `spectatorHistogramDistribution` aliases - as they seem to be just pure aliases with no difference in behaviour (or I missed it?) - but if they do change anything: I wonder if they could be implmeneted as a postagg method which gets a sketch as input like `SpectatorHistogramPercentilePostAggregator`? I think if there is a real need to have pure aliases to an existing aggregator - we should find an alternate way which doesn't duplicate class/types/etc. I feel like its very unfortunate that this was documented ; but not ensured to work with a test... I don't know if this could be considered a regression as #15340 was merged a month before #15371 - so this was never worked like the documentation suggested. I've spent some time trying to uncover what your expectations/options might be in this case - If you could write a testcase to get a better understanding - I think that will most likely speed up our communication speed to fix this. -- 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]
