Skip to content
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

Merged
merged 2 commits into from
Oct 26, 2021
Merged

Add nio Buffers to Painless #79870

merged 2 commits into from
Oct 26, 2021

Conversation

jdconrad
Copy link
Contributor

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.

@jdconrad jdconrad added >enhancement :Core/Infra/Scripting Scripting abstractions, Painless, and Mustache v8.0.0 labels Oct 26, 2021
@jdconrad jdconrad requested a review from rjernst October 26, 2021 20:47
@elasticmachine elasticmachine added the Team:Core/Infra Meta label for core/infra team label Oct 26, 2021
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-core-infra (Team:Core/Infra)

Copy link
Member

@rjernst rjernst left a 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 {
Copy link
Member

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?

Copy link
Contributor Author

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)
Copy link
Member

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.

Copy link
Contributor Author

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.

@jdconrad
Copy link
Contributor Author

@rjernst I've added the requested methods along with tests.

Copy link
Member

@rjernst rjernst left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@jdconrad jdconrad merged commit 665245a into elastic:master Oct 26, 2021
@jdconrad
Copy link
Contributor Author

@rjernst Thanks for the review!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
:Core/Infra/Scripting Scripting abstractions, Painless, and Mustache >enhancement Team:Core/Infra Meta label for core/infra team v8.0.0-beta1
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants