-
Notifications
You must be signed in to change notification settings - Fork 24.7k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add nio Buffers to Painless #79870
Add nio Buffers to Painless #79870
Conversation
Pinging @elastic/es-core-infra (Team:Core/Infra) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, two suggestions
int limit() | ||
} | ||
|
||
class java.nio.ByteBuffer { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we also expose the wrap
methods? Otherwise null
would be the only value available to pass to getValue on a binary field?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will add this.
IntBuffer asIntBuffer() | ||
LongBuffer asLongBuffer() | ||
ShortBuffer asShortBuffer() | ||
byte get(int) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we also need order
, otherwise the order is always big endian. I think is ok to have a such a setter because it modifies the byte buffer object, which is transient, not the underlying data.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll add this too.
@rjernst I've added the requested methods along with tests. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
@rjernst Thanks for the review! |
This change adds the basic java.nio.Buffer to Painless along with the typed direct subclasses. We have only allow listed an absolute get method and some conversion methods to ensure that we do not limit our design space moving forward without more discussion. This includes a test for each method added.
This supports (#79760).
I have also added an issue to allow list the rest of the absolute getter methods (#79867) once Java 13+ is available.