Add parsing method for binary range aggregation

[tlrx]

This pull request adds the parsing logic for InternalBinaryRange aggregation.

[javanna]

LGTM

[javanna]

it seems like the key can really be null here, in which case we just skip the key field?

[tlrx]

Yes, the key can be null. I didn't handle this case correctly so I changed InternalBinaryRangeTests so that it randomly generates a bucket with a null key. I also changed the bucket parsing so that it handles keyed buckets with null keys.

[cbuescher]

I left two minor nits, otherwise LGTM.

[cbuescher]

As far as I can see both getFrom/getFromAsString and getTo/getToAsString return String values. Should we store the converted String instead of the BytesRef in this case?

[tlrx]

Absolutly, good catch

[cbuescher]

Not so important, but as far as I can see InternalBinaryRange#toXContent() writes a String here, so we should be able to parse the value back with parser.text()?

[tlrx]

Sure, thanks

[tlrx]

Thanks for your reviews! I changed few things around null keys and keyed buckets, I'll be happy if one of you can have another look.

[javanna]

it is ok to return null here when calling getKey and getKeyAsString? Although with keyed response we actually do print out a key? Seems like this behaviour doesn't align to REST but does align with what transport client does today?

[tlrx]

I might be missing something but my latest changes should align the behavior... When the key is null, a keyed response will generate a string to name the object in the XContent response but the getKey/getKeyAsString will still return null.

This is what we do here: when we stop to parse the response, we check if the response was keyed and in this case we "try to detect" if the key has been generated on purpose using the from/to values. If so, we know that the original key is null et we set it explicitly.

The InternalMultiBucketAggregationTestCase.assertBucket() takes care of verifying that the getKey/getKeyAsString methods returns the same values after the parsing

[javanna]

I had missed that last line :) sorry!

[javanna]

the thing that worries me a bit here is that we've been testing fromXContent and toXContent everywhere, but we never test the returned values through getters, assuming that they will be the same printed out as part of toXContent, which is not the case here.

[tlrx]

we never test the returned values through getters, assuming that they will be the same printed out as part of toXContent, which is not the case here.

We should always test this. The InternalAggregationTestCase sub classes can override the assertFromXContent() method in order to test getters and other methods provided by the aggregation. Multi bucket ones can override assertBucket().

[javanna]

alright, sounds good!

[tlrx]

Thanks @javanna @cbuescher