Default implementation for getDouble().

[akashdw]

Default implementation for getDouble() in Aggregator/BufferAggregator.
@b-slim

[himanshug]

👍

[b-slim]

@akashdw and @himanshug why do we need a default implementation ?

[himanshug]

@b-slim because all aggregators are failing to compile.
what is the downside of having default impl ? that method would not have a meaningful implementation for the sketch based complex aggs.

[b-slim]

Which aggregators are failing to compile ? this default implementation is not a meaningful one and it can lead the dev into forgetting about implementing it.

[himanshug]

@b-slim e.g. https://github.com/druid-io/druid/blob/master/extensions-core/datasketches/src/main/java/io/druid/query/aggregation/datasketches/theta/SketchBufferAggregator.java#L123 has same and all the other sketch based aggregators (qualtiles, tuples,.. ) would have same impl

[b-slim]

yeah that is the way to go every aggregator will implement what ever needed, but that's not a default implementation anyway same for other methods like getLong. Again not sure which code you said is not compiling ?

[himanshug]

we have custom aggregators which are failing to compile and getDouble() can't have a meaningful implementation for any, hence the default.

[himanshug]

also, about dev forgetting to implement it: wouldn't that specific aggregator end up throwing exception and then the dev would know that it needs to be implemented?

[b-slim]

From a design perspective, I don't think throwing an exception is suppose to be the default implementation of such method it makes no sense IMO, it sends wrong message/question to any new developer.

also, about dev forgetting to implement it: wouldn't that specific aggregator end up throwing exception and then the dev would know that it needs to be implemented?

not necessarily, it can throw the exception on run time and not seeing it during development phase.

Not sure what is wrong with implementing those within the in-house custom aggregator and avoid polluting the interface ?

[himanshug]

I see your point of enforcing that aggregators compilation fail so that owning dev takes a look at it and implement it appropriately. ( I wonder if documenting it in the release notes wouldn't be enough. )

On the other hand, there are many custom aggregators that different teams have (not necessarily one set of devs) and a general user complain about extra work needed to do upgrades. Intention of this PR is to reduce the extra work needed if possible. getDouble() would have the default impl proposed in this PR in most cases.

let us discuss it in the dev sync to arrive at a conclusion.

[leventov]

I support @b-slim here, default implementation makes more harm than good here IMO. If adding default impl to getDouble(), why not to add it to getFloat() and getLong() also? But if we do so, it looks like we give preference to Object aggregators, that is actually false, numeric aggregators are more widely used overall.

[cheddar]

The introduction of the getDouble is a backwards incompatible API change on a public API. Forcing all implementations of all aggregators to add the same default implementation that is added here will only introduce friction for upgrading for anybody who has taken the time to implement their own aggregator.

Backwards incompatible API changes should only be made when absolutely necessary in order to facilitate new functionality. Philosophical concerns are the least important element when discussing backwards incompatible API changes. If there are philosophical concerns with the APIs, then a new set of APIs that won't have the same philosophical issues (both at inception and during evolution) should be introduced and we should migrate to them.

And yes, a default on getLong would've been called for if we had been on Java 8 at the time. getFloat was part of the original interface, so defaulting that wouldn't have happened, but there's no reason for it to not happen. If the request is to add defaults on getFloat and getLong as well as documentation for why we have the individual primitive getters to make it more clear to developers, that sounds reasonable to me.

Fwiw, if we really want to make it super simple to generate different types of columns, it might actually make sense to use getObject and just verify that it is a Double before converting it to the primitive double. I.e. just figure out a way to completely circumvent the primitive getters altogether.

[himanshug]

it was concluded that having a default implementation makes sense to have easy upgrade, however default implementation should return the result of getFloat() instead of throwing exception.
interface change would be noted in the release notes so that extension owners could change their implementations if needed.

[himanshug]

@akashdw can you change the default impl to getFloat() ? it should be mergeable afterwards.

[akashdw]

@b-slim @himanshug Thanks for reviewing the PR. I changed the default implementation to use getFloat().

[b-slim]

can we add a comment like this default method is add to enable smooth backward compatibility, please re-implement it if you aggregators work with numeric double columns or something similar.

[akashdw]

Done.

[b-slim]

👍 left a minor comment.