Skip to content

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