Big arrays sliced from netty buffers (byte)#92706
Big arrays sliced from netty buffers (byte)#92706elasticsearchmachine merged 8 commits intoelastic:mainfrom
Conversation
Based on elastic#90745 but for longs. This should allow aggregations down the road to read long values directly from netty buffer, rather than copying it from the netty buffer. Relates to elastic#89437
|
Pinging @elastic/es-analytics-geo (Team:Analytics) |
not-napoleon
left a comment
There was a problem hiding this comment.
This looks right to me. As with all of these, I think we should let the distributed team know we're adding it in.
original-brownbear
left a comment
There was a problem hiding this comment.
This looks just fine except for the rather serious issue with the array() method that we have to resolve by either not supporting it or enhancing the interface IMO.
|
|
||
| @Override | ||
| public byte[] array() { | ||
| return ref.array(); |
There was a problem hiding this comment.
This seems dangerous. There's a weird assumption in the arrays returned by this method in so far that we assume that valid data starts at index 0.
This is not guaranteed for arrays resulting from in.readReleasableBytesReference(); (as a matter of fact, it's rather unlikely.
If we want to support this, which seems nice, we'd have to enhance the interface to return an arrayOffset just like we do with org.elasticsearch.common.bytes.BytesReference#arrayOffset.
There was a problem hiding this comment.
Thanks for spotting this! Maybe for now shouldn't support this? I can add a comment here and throw unsupported operation exception. Then if/when this really gets used the interface can be improved and supported in a safe manner.
There was a problem hiding this comment.
Sounds good, lets make it not supported here for now then :)
This was used as an optimization for the path ByteArray -> BytesReference which is quite nice. I can open a suggested fix PR once this is in :)
There was a problem hiding this comment.
This was used as an optimization for the path ByteArray -> BytesReference which is quite nice. I can open a suggested fix PR once this is in :)
👍
|
|
||
| @Override | ||
| public byte[] array() { | ||
| // The assumption of this method is that the returned array has valid entries starting from sloy 0 and |
There was a problem hiding this comment.
I guess you mean, "slot" here? :)
|
@elasticmachine update branch |
Based on #90745 but for longs.
This should allow aggregations down the road to read long values directly from netty buffer, rather than copying it from the netty buffer.
Relates to #89437