Split regular histograms from date histograms.

[jpountz]

Currently both aggregations really share the same implementation. This commit
splits the implementations so that regular histograms can support decimal
intervals/offsets and compute correct buckets for negative decimal values.

However the response API is still the same. So for intance both regular
histograms and date histograms will produce an
org.elasticsearch.search.aggregations.bucket.histogram.Histogram
aggregation.

The optimization to compute an identifier of the rounded value and the
rounded value itself has been removed since it was only used by regular
histograms, which now do the rounding themselves instead of relying on the
Rounding abstraction.

Closes #8082
Closes #4847

[nik9000]

I think it is a bit strange to throw an AssertionError when assertions might not be enabled? Maybe just throw and IllegalArgumentException or a RuntimeException or ElasticsearchException or something?

[jpountz]

We seem to be using this exception in a number of places already. I like that this is an error and not an exception as it means there is an error in the code rather than on the user end, and should not be caught?

[bleskes]

Feel free to tell me I'm totally wrong - but do we feel confident enough that a mistake here (in code) is a good enough reason to cause the node to restart ? I'm worried about endless restarts, where a single mistake on a specific API can cause problems on an entire node.

[jpountz]

This is a good point, I am not sure. On the one hand, this is indeed a specific API and restarting is unlikely to fix anything, but on the other hand this is the same for stack overflows are memory leaks?

[bleskes]

for stack overflows are memory leaks?

I'm not sure what you mean? can you unpack those examples?

[nik9000]

I think if you throw a subclass of RuntimeException here it'll get bubbled back to the user and we'll get a bug report if it shows up. AssertionError's problem is that catching it and forwarding it around properly gets us dangerously close to catching OOMs. We probably could make an exception for catching AssertionError but I'm not sure it is worth it when we can just throw a RuntimeException of some sort instead.

Also it just feels weird to throw an assertion error from outside of assertions! Its icky 😨

[jpountz]

For the record, the JVM code itself seems to be using AssertionError for impossible branches.

for stack overflows are memory leaks?

I'm not sure what you mean? can you unpack those examples?

I mean that a specific API could trigger eg. a StackOverflowError, which will cause the node to shut down even though it does not prevent normal operations of the node otherwise.

[nik9000]

For the record, the JVM code itself seems to be using AssertionError for impossible branches.

Weird. I can live with it but it is weird.

[nik9000]

I left a bunch of minor stuff. I love that InternalHistogram.Factory is going. And the new factory is well documented.

[jpountz]

Thanks @nik9000 for having a look to this huge PR! Why don't you like AssertionError? I actually like it for branches that should never get visited, as a way to mean that an invariant has been violated and that the code is buggy. Also the fact that it is an error and not an exception makes it less likely to be swallowed since errors are not supposed to be caught. Finally we seem to already have a number of places where we already use assertion errors.

[rmuir]

ok, but since its abstract can we still document what its doing? Does it really require the insane generic typing?

[jpountz]

I pushed more commits to address comments. The only one I did not address is the naming convention for setters since we use it in all aggregations and it would probably need to be a bigger discussion.

[nik9000]

LGTM.

[jpountz]

Thanks @nik9000 !