adds immutable implementation of byte sequence #4748
Conversation
With this new class there will two implementations of ByteSequence in the Accumulo API, one being mutable and other immutable. The ByteSequence abstract class easily supports an immutable implementation with its current public methods and requires no changes to its public methods. There is a single example of where this new class is used in VisibilityFilter where an iterator caches some data that it does not expect to change for the lifetime of the class. Using the new immutable byte sequence this expectation can be enforced. In a follow on PR could analyze the codebase for other places where this new class could be used. New test were written that achieve 100% coverage of both ByteSequence implementations. Tests were written for some existing code that this PR did not change.
keith-turner
force-pushed
the
immutable_byte_sequence
branch
from
8ac524f
to
ea6a044
Compare
| int len1 = bs1.length(); | ||
| int len2 = bs2.length(); | ||
|
|
||
| int minLen = Math.min(len1, len2); |
There was a problem hiding this comment.
Should probably make these final.
There was a problem hiding this comment.
Updated in a7bf73f
|
|
||
| @Override | ||
| public byte byteAt(int i) { | ||
| throw new IndexOutOfBoundsException(); |
There was a problem hiding this comment.
Curious if this should be UnsupportedOperationException instead?
There was a problem hiding this comment.
Was trying to make all of the different impls of this method throw the same exception. Made some improvements related to the exception in a7bf73f
| * @since 3.1.0 | ||
| */ | ||
| public static ByteSequence of() { | ||
| return EmptyByteSequence.INSTANCE; |
There was a problem hiding this comment.
I'm wondering if EmptyByteSequence should extend ImmutableByteSequence. This would allow you to change the return type of these of methods to ImmutableByteSequence for stronger typing.
There was a problem hiding this comment.
Currently ImmutableByteSequence is package private. Was loosely following the pattern of the java of methods on collections where the concrete type they might return is not public. Do you think ImmutableByteSequence should be made public?
| * @param data byte data | ||
| */ | ||
| ImmutableByteSequence(byte[] data) { | ||
| this.data = Arrays.copyOf(data, data.length); |
There was a problem hiding this comment.
Are you calling Arrays.copyOf here instead of System.arraycopy for the possible intrinsic? It seems like you could just call this(data, 0, data.length).
There was a problem hiding this comment.
updated in 1320c9b
| if (data.isBackedByArray()) { | ||
| this.data = | ||
| Arrays.copyOfRange(data.getBackingArray(), data.offset(), data.offset() + data.length()); | ||
| } else { | ||
| var dataArray = data.toArray(); | ||
| this.data = Arrays.copyOf(dataArray, dataArray.length); |
There was a problem hiding this comment.
There is a private static method , ArrayByteSequence.copy, that has very simliar logic. I wonder if that method should be made public and moved into ByteSequence. Then this becomes:
this.data = ByteSequence.copy(data);
There was a problem hiding this comment.
The else of this method and the other method have a difference in that one cares if the byte array ref escapes and the other does not. Made some changes in b52c685 related to this to avoid two allocations.
As for adding a copy method to public API there are already two way in public API to copy. For immutable copy the new of method. For mutable copy the existing ArrayByteSequence constructor.
This is a WIP to add an immutable implementation of ByteSequence. With this new class there will two implementations of ByteSequence in the Accumulo API, one being mutable and other immutable. The ByteSequence abstract class easily supports an immutable implementation with its current public methods and requires no changes to its public methods.
This commit is a WIP because it lacks unit test, posting the concept for review before implementing unit test. Would need to implement the unit test before taking out of draft. There are TODOs about optimizing also, those could be done in a follow on PR if this is committed.
There is a single example of where this new class is used in VisibilityFilter where an iterator caches some data that it does not expect to change for the lifetime of the class. Using the new immutable byte sequence this expectation can be enforced. In a follow on PR could analyze the codebase for other places where this new class could be used.
This PR was created based on discussions on #4735