Fix overflow bug in SortingNumericDocValues #70154
Conversation
iverase
added
>bug
:Analytics/Geo
elasticmachine
added
the
Team:Analytics
|
Pinging @elastic/es-analytics-geo (Team:Analytics) |
| @Override | ||
| public void accept(long value) { | ||
| total += value; | ||
| assertThat(total, Matchers.greaterThanOrEqualTo(0L)); |
There was a problem hiding this comment.
I think it'd be ever so slightly more readable to make the LongConsumer something like AtomicLong::getAndAdd and then assert the size is exactly what you expect at the bottom. I guess it doesn't assert that you don't have transient negative values though. Maybe do both? Not sure. This is fine too, really.
| @@ -70,15 +70,20 @@ protected final void resize(int newSize) { | |||
| // copying. | |||
| long oldValuesSizeInBytes = values.length * Long.BYTES; | |||
| int newValuesLength = ArrayUtil.oversize(newSize, Long.BYTES); | |||
There was a problem hiding this comment.
Actually the expression above:
long oldValuesSizeInBytes = values.length * Long.BYTES;
Can overflow as well?
There was a problem hiding this comment.
Because length and Long.BYTES are both int, right?
There was a problem hiding this comment.
booooo. And you can't test that one easily.
There was a problem hiding this comment.
yes, that is tricky to test now. we can make getLength a protected method that can be override?
There was a problem hiding this comment.
Yeah. I guess so. I think because this thing is designed for extension maybe we should do something else? These two new methods are only for testing which is fine if you are compositing this thing. But when you extend it its hard to know which to extend.
There was a problem hiding this comment.
Maybe we should be just add asserts in the code. I think this is what they are design for?
There was a problem hiding this comment.
Without the overrides you can't put the thing into a state where we'd see the overflows. I'd just go with the protected method and a comment, I think. I just kind of got confused.
|
@nik9000 another iteration, it leaks a bit the internal implementation but we are testing that anyway. |
This was found when performing some abusing geogrid aggregations. During resizing of the array, we are computing the size of the array as an integer, which may overflow if the size of the new array is big. This PR just make sure that computing the new size is done using long.