You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@geode.apache.org by GitBox <gi...@apache.org> on 2021/05/11 20:05:49 UTC
[GitHub] [geode] DonalEvans opened a new pull request #6464: GEODE-9221: Remove uses of ByteArrayWrapper from RedisString
DonalEvans opened a new pull request #6464:
URL: https://github.com/apache/geode/pull/6464
- Replace uses of ByteArrayWrapper in RedisString and NullRedisString
with byte[]
- Refactor to remove warnings
Authored-by: Donal Evans <do...@vmware.com>
Thank you for submitting a contribution to Apache Geode.
In order to streamline the review of the contribution we ask you
to ensure the following steps have been taken:
### For all changes:
- [x] Is there a JIRA ticket associated with this PR? Is it referenced in the commit message?
- [x] Has your PR been rebased against the latest commit within the target branch (typically `develop`)?
- [x] Is your initial contribution a single, squashed commit?
- [x] Does `gradlew build` run cleanly?
- [x] Have you written or updated unit tests to verify your changes?
- [N/A] If adding new dependencies to the code, are these dependencies licensed in a way that is compatible for inclusion under [ASF 2.0](http://www.apache.org/legal/resolved.html#category-a)?
### Note:
Please ensure that once the PR is submitted, check Concourse for build issues and
submit an update to your PR as soon as possible. If you need help, please send an
email to dev@geode.apache.org.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] sabbey37 commented on a change in pull request #6464: GEODE-9221: Remove uses of ByteArrayWrapper from RedisString
Posted by GitBox <gi...@apache.org>.
sabbey37 commented on a change in pull request #6464:
URL: https://github.com/apache/geode/pull/6464#discussion_r631865553
##########
File path: geode-apis-compatible-with-redis/src/test/java/org/apache/geode/redis/internal/data/RedisStringTest.java
##########
@@ -313,6 +317,148 @@ public void setExpirationTimestamp_stores_delta_that_is_stable() throws IOExcept
assertThat(stringTwo).isEqualTo(stringOne);
}
+ @Test
+ public void bitposReturnsBitPosition() {
+ // [0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 1]
+ byte[] bytes = {0, 1};
+ RedisString string = new RedisString(bytes);
+ assertThat(string.bitpos(1, 0, 1)).isEqualTo(15);
+ }
+
+ @Test
+ public void bitposReturnsBitPositionOfFirstOccurrenceOfBit() {
+ // [0, 0, 0, 0, 0, 0, 0, 0, 1, 1, 1, 1, 1, 1, 1, 1]
+ byte[] bytes1 = {0, -1};
+ RedisString string = new RedisString(bytes1);
+ assertThat(string.bitpos(1, 0, 1)).isEqualTo(8);
+
+ // [1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1]
+ byte[] bytes2 = {-1, -1};
+ string = new RedisString(bytes2);
+ assertThat(string.bitpos(1, 0, 1)).isEqualTo(0);
+ }
+
+ @Test
+ public void bitposReturnsBitPositionWhenStartIsGreaterThanZero() {
+ // [1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1]
+ byte[] bytes = {-1, -1};
+ RedisString string = new RedisString(bytes);
+ assertThat(string.bitpos(1, 1, 1)).isEqualTo(8);
+ }
+
+ @Test
+ public void bitposReturnsBitPositionWhenEndIsNotSet() {
+ // [0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 1]
+ byte[] bytes = {0, 1};
+ RedisString string = new RedisString(bytes);
+ assertThat(string.bitpos(1, 0, null)).isEqualTo(15);
+ }
+
+ @Test
+ public void bitposReturnsBitPositionWhenStartIsNotMoreNegativeThanByteArrayLength() {
+ // [0, 0, 0, 0, 0, 0, 0, 1, 0, 0, 0, 0, 0, 0, 0, 1]
+ byte[] bytes = {1, 1};
+ RedisString string = new RedisString(bytes);
+ // Index from the end of the byte array, a value of -1 is equivalent to bytes.length - 2
+ assertThat(string.bitpos(1, -1, null)).isEqualTo(15);
+ }
+
+ @Test
+ public void bitposReturnsBitPositionWhenStartIsMoreNegativeThanByteArrayLength() {
+ // [0, 0, 0, 0, 0, 0, 0, 1, 0, 0, 0, 0, 0, 0, 0, 1]
+ byte[] bytes = {1, 1};
+ RedisString string = new RedisString(bytes);
+ // Index from the end of the byte array, a value of -3 is equivalent to bytes.length - 4
+ assertThat(string.bitpos(1, -3, null)).isEqualTo(7);
+ }
+
+ @Test
+ public void bitposReturnsBitPositionWhenEndIsNotMoreNegativeThanByteArrayLength() {
+ // [0, 0, 0, 0, 0, 0, 0, 1, 0, 0, 0, 0, 0, 0, 0, 1]
+ byte[] bytes = {1, 1};
+ RedisString string = new RedisString(bytes);
+ // Index from the end of the byte array, a value of -1 is equivalent to bytes.length - 2
+ assertThat(string.bitpos(1, 0, -1)).isEqualTo(7);
+ }
+
+ @Test
+ public void bitposReturnsBitPositionWhenStartAndEndAreMoreNegativeThanByteArrayLengthAndFirstByteContainsBit() {
+ // [0, 0, 0, 0, 0, 0, 0, 1, 0, 0, 0, 0, 0, 0, 0, 1]
+ byte[] bytes = {1, 1};
+ RedisString string = new RedisString(bytes);
+ // Index from the end of the byte array, a value of -3 is equivalent to bytes.length - 4
+ assertThat(string.bitpos(1, -3, -3)).isEqualTo(7);
+ }
+
+ @Test
+ public void bitposReturnsNegativeOneWhenBitIsNotFound() {
+ // [0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0]
+ byte[] bytes = {0, 0};
+ RedisString string = new RedisString(bytes);
+ assertThat(string.bitpos(1, 0, null)).isEqualTo(-1);
+ }
+
+ @Test
+ public void bitposReturnsNegativeOneWhenBitIsNotFoundAndBitIsZeroAndEndIsSet() {
+ // [1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1]
+ byte[] bytes = {-1, -1};
+ RedisString string = new RedisString(bytes);
+ assertThat(string.bitpos(0, 0, bytes.length)).isEqualTo(-1);
+ }
+
+ @Test
+ public void bitposReturnsValueBitArrayLengthWhenBitIsNotFoundAndBitIsZeroAndEndIsNotSet() {
+ // [1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1]
+ byte[] bytes = {-1, -1};
+ RedisString string = new RedisString(bytes);
+ assertThat(string.bitpos(0, 0, null)).isEqualTo(bytes.length * 8);
+ }
+
+ @Test
+ public void bitposReturnsBitPositionWhenEndIsGreaterThanOrEqualToValueArrayLength() {
+ // [0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 1]
+ byte[] bytes = {0, 1};
+ RedisString string = new RedisString(bytes);
+ assertThat(string.bitpos(1, 0, bytes.length)).isEqualTo(15);
+ assertThat(string.bitpos(1, 0, bytes.length + 1)).isEqualTo(15);
+ }
+
+ @Test
+ public void bitposReturnsBitPositionWhenStartAndEndAreGreaterThanOrEqualToValueArrayLengthAndLastByteContainsBit() {
+ // [0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 1]
+ byte[] bytes = {0, 1};
+ RedisString string = new RedisString(bytes);
+ assertThat(string.bitpos(1, bytes.length, null)).isEqualTo(15);
+ assertThat(string.bitpos(1, bytes.length + 1, null)).isEqualTo(15);
+ }
+
+ @Test
+ public void bitposReturnsNegativeOneWhenStartAndEndAreGreaterThanOrEqualToValueArrayLengthAndLastByteIsDoesNotContainBit() {
+ // [0, 0, 0, 0, 0, 0, 0, 1, 0, 0, 0, 0, 0, 0, 0, 0]
+ byte[] bytes = {1, 0};
+ RedisString string = new RedisString(bytes);
+ assertThat(string.bitpos(1, bytes.length, bytes.length)).isEqualTo(-1);
+ assertThat(string.bitpos(1, bytes.length + 1, bytes.length + 1)).isEqualTo(-1);
+ }
+
+ @Test
+ public void bitposReturnsNegativeOneWhenValueIsEmptyArray() {
+ RedisString string = new RedisString(new byte[] {});
+ assertThat(string.bitpos(1, 0, 1)).isEqualTo(-1);
+ }
+
+ @Test
+ public void bitposReturnsNegativeOneWhenEndIsLessThanStart() {
+ RedisString string = new RedisString(new byte[] {0, 1});
+ assertThat(string.bitpos(1, 1, 0)).isEqualTo(-1);
+ }
+
+ @Test
+ public void bitposReturnsNegativeOneWhenBitIsNotZeroOrOne() {
+ RedisString string = new RedisString(new byte[] {0, 1});
+ assertThat(string.bitpos(2, 0, 1)).isEqualTo(-1);
+ }
+
Review comment:
The reason it can't be exercised via an integration test is because we catch it and throw an error in the `BitPosExecutor`:
```
if (bit != 0 && bit != 1) {
return RedisResponse.error(ERROR_BIT);
}
```
So I'm not sure we need a unit test for it... though unit tests are cheap and more tests can't hurt. I guess this goes back to our testing strategy, which we can discuss with the team.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] DonalEvans commented on a change in pull request #6464: GEODE-9221: Remove uses of ByteArrayWrapper from RedisString
Posted by GitBox <gi...@apache.org>.
DonalEvans commented on a change in pull request #6464:
URL: https://github.com/apache/geode/pull/6464#discussion_r631345407
##########
File path: geode-apis-compatible-with-redis/src/test/java/org/apache/geode/redis/internal/data/RedisStringTest.java
##########
@@ -313,6 +317,148 @@ public void setExpirationTimestamp_stores_delta_that_is_stable() throws IOExcept
assertThat(stringTwo).isEqualTo(stringOne);
}
+ @Test
+ public void bitposReturnsBitPosition() {
+ // [0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 1]
+ byte[] bytes = {0, 1};
+ RedisString string = new RedisString(bytes);
+ assertThat(string.bitpos(1, 0, 1)).isEqualTo(15);
+ }
+
+ @Test
+ public void bitposReturnsBitPositionOfFirstOccurrenceOfBit() {
+ // [0, 0, 0, 0, 0, 0, 0, 0, 1, 1, 1, 1, 1, 1, 1, 1]
+ byte[] bytes1 = {0, -1};
+ RedisString string = new RedisString(bytes1);
+ assertThat(string.bitpos(1, 0, 1)).isEqualTo(8);
+
+ // [1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1]
+ byte[] bytes2 = {-1, -1};
+ string = new RedisString(bytes2);
+ assertThat(string.bitpos(1, 0, 1)).isEqualTo(0);
+ }
+
+ @Test
+ public void bitposReturnsBitPositionWhenStartIsGreaterThanZero() {
+ // [1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1]
+ byte[] bytes = {-1, -1};
+ RedisString string = new RedisString(bytes);
+ assertThat(string.bitpos(1, 1, 1)).isEqualTo(8);
+ }
+
+ @Test
+ public void bitposReturnsBitPositionWhenEndIsNotSet() {
+ // [0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 1]
+ byte[] bytes = {0, 1};
+ RedisString string = new RedisString(bytes);
+ assertThat(string.bitpos(1, 0, null)).isEqualTo(15);
+ }
+
+ @Test
+ public void bitposReturnsBitPositionWhenStartIsNotMoreNegativeThanByteArrayLength() {
+ // [0, 0, 0, 0, 0, 0, 0, 1, 0, 0, 0, 0, 0, 0, 0, 1]
+ byte[] bytes = {1, 1};
+ RedisString string = new RedisString(bytes);
+ // Index from the end of the byte array, a value of -1 is equivalent to bytes.length - 2
+ assertThat(string.bitpos(1, -1, null)).isEqualTo(15);
+ }
+
+ @Test
+ public void bitposReturnsBitPositionWhenStartIsMoreNegativeThanByteArrayLength() {
+ // [0, 0, 0, 0, 0, 0, 0, 1, 0, 0, 0, 0, 0, 0, 0, 1]
+ byte[] bytes = {1, 1};
+ RedisString string = new RedisString(bytes);
+ // Index from the end of the byte array, a value of -3 is equivalent to bytes.length - 4
+ assertThat(string.bitpos(1, -3, null)).isEqualTo(7);
+ }
+
+ @Test
+ public void bitposReturnsBitPositionWhenEndIsNotMoreNegativeThanByteArrayLength() {
+ // [0, 0, 0, 0, 0, 0, 0, 1, 0, 0, 0, 0, 0, 0, 0, 1]
+ byte[] bytes = {1, 1};
+ RedisString string = new RedisString(bytes);
+ // Index from the end of the byte array, a value of -1 is equivalent to bytes.length - 2
+ assertThat(string.bitpos(1, 0, -1)).isEqualTo(7);
+ }
+
+ @Test
+ public void bitposReturnsBitPositionWhenStartAndEndAreMoreNegativeThanByteArrayLengthAndFirstByteContainsBit() {
+ // [0, 0, 0, 0, 0, 0, 0, 1, 0, 0, 0, 0, 0, 0, 0, 1]
+ byte[] bytes = {1, 1};
+ RedisString string = new RedisString(bytes);
+ // Index from the end of the byte array, a value of -3 is equivalent to bytes.length - 4
+ assertThat(string.bitpos(1, -3, -3)).isEqualTo(7);
+ }
+
+ @Test
+ public void bitposReturnsNegativeOneWhenBitIsNotFound() {
+ // [0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0]
+ byte[] bytes = {0, 0};
+ RedisString string = new RedisString(bytes);
+ assertThat(string.bitpos(1, 0, null)).isEqualTo(-1);
+ }
+
+ @Test
+ public void bitposReturnsNegativeOneWhenBitIsNotFoundAndBitIsZeroAndEndIsSet() {
+ // [1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1]
+ byte[] bytes = {-1, -1};
+ RedisString string = new RedisString(bytes);
+ assertThat(string.bitpos(0, 0, bytes.length)).isEqualTo(-1);
+ }
+
+ @Test
+ public void bitposReturnsValueBitArrayLengthWhenBitIsNotFoundAndBitIsZeroAndEndIsNotSet() {
+ // [1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1]
+ byte[] bytes = {-1, -1};
+ RedisString string = new RedisString(bytes);
+ assertThat(string.bitpos(0, 0, null)).isEqualTo(bytes.length * 8);
+ }
+
+ @Test
+ public void bitposReturnsBitPositionWhenEndIsGreaterThanOrEqualToValueArrayLength() {
+ // [0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 1]
+ byte[] bytes = {0, 1};
+ RedisString string = new RedisString(bytes);
+ assertThat(string.bitpos(1, 0, bytes.length)).isEqualTo(15);
+ assertThat(string.bitpos(1, 0, bytes.length + 1)).isEqualTo(15);
+ }
+
+ @Test
+ public void bitposReturnsBitPositionWhenStartAndEndAreGreaterThanOrEqualToValueArrayLengthAndLastByteContainsBit() {
+ // [0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 1]
+ byte[] bytes = {0, 1};
+ RedisString string = new RedisString(bytes);
+ assertThat(string.bitpos(1, bytes.length, null)).isEqualTo(15);
+ assertThat(string.bitpos(1, bytes.length + 1, null)).isEqualTo(15);
+ }
+
+ @Test
+ public void bitposReturnsNegativeOneWhenStartAndEndAreGreaterThanOrEqualToValueArrayLengthAndLastByteIsDoesNotContainBit() {
+ // [0, 0, 0, 0, 0, 0, 0, 1, 0, 0, 0, 0, 0, 0, 0, 0]
+ byte[] bytes = {1, 0};
+ RedisString string = new RedisString(bytes);
+ assertThat(string.bitpos(1, bytes.length, bytes.length)).isEqualTo(-1);
+ assertThat(string.bitpos(1, bytes.length + 1, bytes.length + 1)).isEqualTo(-1);
+ }
+
+ @Test
+ public void bitposReturnsNegativeOneWhenValueIsEmptyArray() {
+ RedisString string = new RedisString(new byte[] {});
+ assertThat(string.bitpos(1, 0, 1)).isEqualTo(-1);
+ }
+
+ @Test
+ public void bitposReturnsNegativeOneWhenEndIsLessThanStart() {
+ RedisString string = new RedisString(new byte[] {0, 1});
+ assertThat(string.bitpos(1, 1, 0)).isEqualTo(-1);
+ }
+
+ @Test
+ public void bitposReturnsNegativeOneWhenBitIsNotZeroOrOne() {
+ RedisString string = new RedisString(new byte[] {0, 1});
+ assertThat(string.bitpos(2, 0, 1)).isEqualTo(-1);
+ }
+
Review comment:
Ah, sorry for the mistake. I'll look over the contents of `AbstractBitPosIntegrationTest` and expand the coverage if anything's not there that I added here.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] sabbey37 commented on a change in pull request #6464: GEODE-9221: Remove uses of ByteArrayWrapper from RedisString
Posted by GitBox <gi...@apache.org>.
sabbey37 commented on a change in pull request #6464:
URL: https://github.com/apache/geode/pull/6464#discussion_r631865553
##########
File path: geode-apis-compatible-with-redis/src/test/java/org/apache/geode/redis/internal/data/RedisStringTest.java
##########
@@ -313,6 +317,148 @@ public void setExpirationTimestamp_stores_delta_that_is_stable() throws IOExcept
assertThat(stringTwo).isEqualTo(stringOne);
}
+ @Test
+ public void bitposReturnsBitPosition() {
+ // [0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 1]
+ byte[] bytes = {0, 1};
+ RedisString string = new RedisString(bytes);
+ assertThat(string.bitpos(1, 0, 1)).isEqualTo(15);
+ }
+
+ @Test
+ public void bitposReturnsBitPositionOfFirstOccurrenceOfBit() {
+ // [0, 0, 0, 0, 0, 0, 0, 0, 1, 1, 1, 1, 1, 1, 1, 1]
+ byte[] bytes1 = {0, -1};
+ RedisString string = new RedisString(bytes1);
+ assertThat(string.bitpos(1, 0, 1)).isEqualTo(8);
+
+ // [1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1]
+ byte[] bytes2 = {-1, -1};
+ string = new RedisString(bytes2);
+ assertThat(string.bitpos(1, 0, 1)).isEqualTo(0);
+ }
+
+ @Test
+ public void bitposReturnsBitPositionWhenStartIsGreaterThanZero() {
+ // [1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1]
+ byte[] bytes = {-1, -1};
+ RedisString string = new RedisString(bytes);
+ assertThat(string.bitpos(1, 1, 1)).isEqualTo(8);
+ }
+
+ @Test
+ public void bitposReturnsBitPositionWhenEndIsNotSet() {
+ // [0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 1]
+ byte[] bytes = {0, 1};
+ RedisString string = new RedisString(bytes);
+ assertThat(string.bitpos(1, 0, null)).isEqualTo(15);
+ }
+
+ @Test
+ public void bitposReturnsBitPositionWhenStartIsNotMoreNegativeThanByteArrayLength() {
+ // [0, 0, 0, 0, 0, 0, 0, 1, 0, 0, 0, 0, 0, 0, 0, 1]
+ byte[] bytes = {1, 1};
+ RedisString string = new RedisString(bytes);
+ // Index from the end of the byte array, a value of -1 is equivalent to bytes.length - 2
+ assertThat(string.bitpos(1, -1, null)).isEqualTo(15);
+ }
+
+ @Test
+ public void bitposReturnsBitPositionWhenStartIsMoreNegativeThanByteArrayLength() {
+ // [0, 0, 0, 0, 0, 0, 0, 1, 0, 0, 0, 0, 0, 0, 0, 1]
+ byte[] bytes = {1, 1};
+ RedisString string = new RedisString(bytes);
+ // Index from the end of the byte array, a value of -3 is equivalent to bytes.length - 4
+ assertThat(string.bitpos(1, -3, null)).isEqualTo(7);
+ }
+
+ @Test
+ public void bitposReturnsBitPositionWhenEndIsNotMoreNegativeThanByteArrayLength() {
+ // [0, 0, 0, 0, 0, 0, 0, 1, 0, 0, 0, 0, 0, 0, 0, 1]
+ byte[] bytes = {1, 1};
+ RedisString string = new RedisString(bytes);
+ // Index from the end of the byte array, a value of -1 is equivalent to bytes.length - 2
+ assertThat(string.bitpos(1, 0, -1)).isEqualTo(7);
+ }
+
+ @Test
+ public void bitposReturnsBitPositionWhenStartAndEndAreMoreNegativeThanByteArrayLengthAndFirstByteContainsBit() {
+ // [0, 0, 0, 0, 0, 0, 0, 1, 0, 0, 0, 0, 0, 0, 0, 1]
+ byte[] bytes = {1, 1};
+ RedisString string = new RedisString(bytes);
+ // Index from the end of the byte array, a value of -3 is equivalent to bytes.length - 4
+ assertThat(string.bitpos(1, -3, -3)).isEqualTo(7);
+ }
+
+ @Test
+ public void bitposReturnsNegativeOneWhenBitIsNotFound() {
+ // [0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0]
+ byte[] bytes = {0, 0};
+ RedisString string = new RedisString(bytes);
+ assertThat(string.bitpos(1, 0, null)).isEqualTo(-1);
+ }
+
+ @Test
+ public void bitposReturnsNegativeOneWhenBitIsNotFoundAndBitIsZeroAndEndIsSet() {
+ // [1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1]
+ byte[] bytes = {-1, -1};
+ RedisString string = new RedisString(bytes);
+ assertThat(string.bitpos(0, 0, bytes.length)).isEqualTo(-1);
+ }
+
+ @Test
+ public void bitposReturnsValueBitArrayLengthWhenBitIsNotFoundAndBitIsZeroAndEndIsNotSet() {
+ // [1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1]
+ byte[] bytes = {-1, -1};
+ RedisString string = new RedisString(bytes);
+ assertThat(string.bitpos(0, 0, null)).isEqualTo(bytes.length * 8);
+ }
+
+ @Test
+ public void bitposReturnsBitPositionWhenEndIsGreaterThanOrEqualToValueArrayLength() {
+ // [0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 1]
+ byte[] bytes = {0, 1};
+ RedisString string = new RedisString(bytes);
+ assertThat(string.bitpos(1, 0, bytes.length)).isEqualTo(15);
+ assertThat(string.bitpos(1, 0, bytes.length + 1)).isEqualTo(15);
+ }
+
+ @Test
+ public void bitposReturnsBitPositionWhenStartAndEndAreGreaterThanOrEqualToValueArrayLengthAndLastByteContainsBit() {
+ // [0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 1]
+ byte[] bytes = {0, 1};
+ RedisString string = new RedisString(bytes);
+ assertThat(string.bitpos(1, bytes.length, null)).isEqualTo(15);
+ assertThat(string.bitpos(1, bytes.length + 1, null)).isEqualTo(15);
+ }
+
+ @Test
+ public void bitposReturnsNegativeOneWhenStartAndEndAreGreaterThanOrEqualToValueArrayLengthAndLastByteIsDoesNotContainBit() {
+ // [0, 0, 0, 0, 0, 0, 0, 1, 0, 0, 0, 0, 0, 0, 0, 0]
+ byte[] bytes = {1, 0};
+ RedisString string = new RedisString(bytes);
+ assertThat(string.bitpos(1, bytes.length, bytes.length)).isEqualTo(-1);
+ assertThat(string.bitpos(1, bytes.length + 1, bytes.length + 1)).isEqualTo(-1);
+ }
+
+ @Test
+ public void bitposReturnsNegativeOneWhenValueIsEmptyArray() {
+ RedisString string = new RedisString(new byte[] {});
+ assertThat(string.bitpos(1, 0, 1)).isEqualTo(-1);
+ }
+
+ @Test
+ public void bitposReturnsNegativeOneWhenEndIsLessThanStart() {
+ RedisString string = new RedisString(new byte[] {0, 1});
+ assertThat(string.bitpos(1, 1, 0)).isEqualTo(-1);
+ }
+
+ @Test
+ public void bitposReturnsNegativeOneWhenBitIsNotZeroOrOne() {
+ RedisString string = new RedisString(new byte[] {0, 1});
+ assertThat(string.bitpos(2, 0, 1)).isEqualTo(-1);
+ }
+
Review comment:
The reason it can't be exercised via an integration test is because we catch it and throw an error in the `BitPosExecutor`:
```
if (bit != 0 && bit != 1) {
return RedisResponse.error(ERROR_BIT);
}
```
So I'm not sure we need a unit test for it (but I don't think we have an integration test for it yet)... though unit tests are cheap and more tests can't hurt. I guess this goes back to our testing strategy, which we can discuss with the team.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] DonalEvans merged pull request #6464: GEODE-9221: Remove uses of ByteArrayWrapper from RedisString
Posted by GitBox <gi...@apache.org>.
DonalEvans merged pull request #6464:
URL: https://github.com/apache/geode/pull/6464
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] sabbey37 commented on a change in pull request #6464: GEODE-9221: Remove uses of ByteArrayWrapper from RedisString
Posted by GitBox <gi...@apache.org>.
sabbey37 commented on a change in pull request #6464:
URL: https://github.com/apache/geode/pull/6464#discussion_r631852185
##########
File path: geode-apis-compatible-with-redis/src/integrationTest/java/org/apache/geode/redis/internal/executor/string/AbstractBitPosIntegrationTest.java
##########
@@ -115,11 +123,69 @@ public void bitposWithStart_givenOneInLastByte() {
assertThat(jedis.bitpos(key, true, new BitPosParams(-1))).isEqualTo(7 + 3 * 8);
}
+ @Test
+ public void bitposWithStart_givenStartGreaterThanOrEqualToByteArrayLength() {
+ byte[] key = {1, 2, 3};
+ byte[] bytes = {1, 1, 1, 1};
+ jedis.set(key, bytes);
+ assertThat(jedis.bitpos(key, true, new BitPosParams(bytes.length))).isEqualTo(7 + 3 * 8);
+ assertThat(jedis.bitpos(key, true, new BitPosParams(bytes.length + 1))).isEqualTo(7 + 3 * 8);
+ }
Review comment:
Looks like Native Redis doesn't check whether the start is greater than or equal to the length of the value beforehand and just returns -1 if start is greater than end:
```
/* For empty ranges (start > end) we return -1 as an empty range does
* not contain a 0 nor a 1. */
if (start > end) {
addReplyLongLong(c, -1);
```
I definitely feel like we've gotten beyond the scope of this Jira ticket/PR though.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] DonalEvans commented on a change in pull request #6464: GEODE-9221: Remove uses of ByteArrayWrapper from RedisString
Posted by GitBox <gi...@apache.org>.
DonalEvans commented on a change in pull request #6464:
URL: https://github.com/apache/geode/pull/6464#discussion_r631437994
##########
File path: geode-apis-compatible-with-redis/src/test/java/org/apache/geode/redis/internal/data/RedisStringTest.java
##########
@@ -313,6 +317,148 @@ public void setExpirationTimestamp_stores_delta_that_is_stable() throws IOExcept
assertThat(stringTwo).isEqualTo(stringOne);
}
+ @Test
+ public void bitposReturnsBitPosition() {
+ // [0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 1]
+ byte[] bytes = {0, 1};
+ RedisString string = new RedisString(bytes);
+ assertThat(string.bitpos(1, 0, 1)).isEqualTo(15);
+ }
+
+ @Test
+ public void bitposReturnsBitPositionOfFirstOccurrenceOfBit() {
+ // [0, 0, 0, 0, 0, 0, 0, 0, 1, 1, 1, 1, 1, 1, 1, 1]
+ byte[] bytes1 = {0, -1};
+ RedisString string = new RedisString(bytes1);
+ assertThat(string.bitpos(1, 0, 1)).isEqualTo(8);
+
+ // [1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1]
+ byte[] bytes2 = {-1, -1};
+ string = new RedisString(bytes2);
+ assertThat(string.bitpos(1, 0, 1)).isEqualTo(0);
+ }
+
+ @Test
+ public void bitposReturnsBitPositionWhenStartIsGreaterThanZero() {
+ // [1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1]
+ byte[] bytes = {-1, -1};
+ RedisString string = new RedisString(bytes);
+ assertThat(string.bitpos(1, 1, 1)).isEqualTo(8);
+ }
+
+ @Test
+ public void bitposReturnsBitPositionWhenEndIsNotSet() {
+ // [0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 1]
+ byte[] bytes = {0, 1};
+ RedisString string = new RedisString(bytes);
+ assertThat(string.bitpos(1, 0, null)).isEqualTo(15);
+ }
+
+ @Test
+ public void bitposReturnsBitPositionWhenStartIsNotMoreNegativeThanByteArrayLength() {
+ // [0, 0, 0, 0, 0, 0, 0, 1, 0, 0, 0, 0, 0, 0, 0, 1]
+ byte[] bytes = {1, 1};
+ RedisString string = new RedisString(bytes);
+ // Index from the end of the byte array, a value of -1 is equivalent to bytes.length - 2
+ assertThat(string.bitpos(1, -1, null)).isEqualTo(15);
+ }
+
+ @Test
+ public void bitposReturnsBitPositionWhenStartIsMoreNegativeThanByteArrayLength() {
+ // [0, 0, 0, 0, 0, 0, 0, 1, 0, 0, 0, 0, 0, 0, 0, 1]
+ byte[] bytes = {1, 1};
+ RedisString string = new RedisString(bytes);
+ // Index from the end of the byte array, a value of -3 is equivalent to bytes.length - 4
+ assertThat(string.bitpos(1, -3, null)).isEqualTo(7);
+ }
+
+ @Test
+ public void bitposReturnsBitPositionWhenEndIsNotMoreNegativeThanByteArrayLength() {
+ // [0, 0, 0, 0, 0, 0, 0, 1, 0, 0, 0, 0, 0, 0, 0, 1]
+ byte[] bytes = {1, 1};
+ RedisString string = new RedisString(bytes);
+ // Index from the end of the byte array, a value of -1 is equivalent to bytes.length - 2
+ assertThat(string.bitpos(1, 0, -1)).isEqualTo(7);
+ }
+
+ @Test
+ public void bitposReturnsBitPositionWhenStartAndEndAreMoreNegativeThanByteArrayLengthAndFirstByteContainsBit() {
+ // [0, 0, 0, 0, 0, 0, 0, 1, 0, 0, 0, 0, 0, 0, 0, 1]
+ byte[] bytes = {1, 1};
+ RedisString string = new RedisString(bytes);
+ // Index from the end of the byte array, a value of -3 is equivalent to bytes.length - 4
+ assertThat(string.bitpos(1, -3, -3)).isEqualTo(7);
+ }
+
+ @Test
+ public void bitposReturnsNegativeOneWhenBitIsNotFound() {
+ // [0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0]
+ byte[] bytes = {0, 0};
+ RedisString string = new RedisString(bytes);
+ assertThat(string.bitpos(1, 0, null)).isEqualTo(-1);
+ }
+
+ @Test
+ public void bitposReturnsNegativeOneWhenBitIsNotFoundAndBitIsZeroAndEndIsSet() {
+ // [1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1]
+ byte[] bytes = {-1, -1};
+ RedisString string = new RedisString(bytes);
+ assertThat(string.bitpos(0, 0, bytes.length)).isEqualTo(-1);
+ }
+
+ @Test
+ public void bitposReturnsValueBitArrayLengthWhenBitIsNotFoundAndBitIsZeroAndEndIsNotSet() {
+ // [1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1]
+ byte[] bytes = {-1, -1};
+ RedisString string = new RedisString(bytes);
+ assertThat(string.bitpos(0, 0, null)).isEqualTo(bytes.length * 8);
+ }
+
+ @Test
+ public void bitposReturnsBitPositionWhenEndIsGreaterThanOrEqualToValueArrayLength() {
+ // [0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 1]
+ byte[] bytes = {0, 1};
+ RedisString string = new RedisString(bytes);
+ assertThat(string.bitpos(1, 0, bytes.length)).isEqualTo(15);
+ assertThat(string.bitpos(1, 0, bytes.length + 1)).isEqualTo(15);
+ }
+
+ @Test
+ public void bitposReturnsBitPositionWhenStartAndEndAreGreaterThanOrEqualToValueArrayLengthAndLastByteContainsBit() {
+ // [0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 1]
+ byte[] bytes = {0, 1};
+ RedisString string = new RedisString(bytes);
+ assertThat(string.bitpos(1, bytes.length, null)).isEqualTo(15);
+ assertThat(string.bitpos(1, bytes.length + 1, null)).isEqualTo(15);
+ }
+
+ @Test
+ public void bitposReturnsNegativeOneWhenStartAndEndAreGreaterThanOrEqualToValueArrayLengthAndLastByteIsDoesNotContainBit() {
+ // [0, 0, 0, 0, 0, 0, 0, 1, 0, 0, 0, 0, 0, 0, 0, 0]
+ byte[] bytes = {1, 0};
+ RedisString string = new RedisString(bytes);
+ assertThat(string.bitpos(1, bytes.length, bytes.length)).isEqualTo(-1);
+ assertThat(string.bitpos(1, bytes.length + 1, bytes.length + 1)).isEqualTo(-1);
+ }
+
+ @Test
+ public void bitposReturnsNegativeOneWhenValueIsEmptyArray() {
+ RedisString string = new RedisString(new byte[] {});
+ assertThat(string.bitpos(1, 0, 1)).isEqualTo(-1);
+ }
+
+ @Test
+ public void bitposReturnsNegativeOneWhenEndIsLessThanStart() {
+ RedisString string = new RedisString(new byte[] {0, 1});
+ assertThat(string.bitpos(1, 1, 0)).isEqualTo(-1);
+ }
+
+ @Test
+ public void bitposReturnsNegativeOneWhenBitIsNotZeroOrOne() {
+ RedisString string = new RedisString(new byte[] {0, 1});
+ assertThat(string.bitpos(2, 0, 1)).isEqualTo(-1);
+ }
+
Review comment:
I left one tase case in the unit test, as it's testing behaviour specific to the implementation in RedisString that can't be exercised via an integration test.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] sabbey37 commented on a change in pull request #6464: GEODE-9221: Remove uses of ByteArrayWrapper from RedisString
Posted by GitBox <gi...@apache.org>.
sabbey37 commented on a change in pull request #6464:
URL: https://github.com/apache/geode/pull/6464#discussion_r631333123
##########
File path: geode-apis-compatible-with-redis/src/test/java/org/apache/geode/redis/internal/data/RedisStringTest.java
##########
@@ -313,6 +317,148 @@ public void setExpirationTimestamp_stores_delta_that_is_stable() throws IOExcept
assertThat(stringTwo).isEqualTo(stringOne);
}
+ @Test
+ public void bitposReturnsBitPosition() {
+ // [0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 1]
+ byte[] bytes = {0, 1};
+ RedisString string = new RedisString(bytes);
+ assertThat(string.bitpos(1, 0, 1)).isEqualTo(15);
+ }
+
+ @Test
+ public void bitposReturnsBitPositionOfFirstOccurrenceOfBit() {
+ // [0, 0, 0, 0, 0, 0, 0, 0, 1, 1, 1, 1, 1, 1, 1, 1]
+ byte[] bytes1 = {0, -1};
+ RedisString string = new RedisString(bytes1);
+ assertThat(string.bitpos(1, 0, 1)).isEqualTo(8);
+
+ // [1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1]
+ byte[] bytes2 = {-1, -1};
+ string = new RedisString(bytes2);
+ assertThat(string.bitpos(1, 0, 1)).isEqualTo(0);
+ }
+
+ @Test
+ public void bitposReturnsBitPositionWhenStartIsGreaterThanZero() {
+ // [1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1]
+ byte[] bytes = {-1, -1};
+ RedisString string = new RedisString(bytes);
+ assertThat(string.bitpos(1, 1, 1)).isEqualTo(8);
+ }
+
+ @Test
+ public void bitposReturnsBitPositionWhenEndIsNotSet() {
+ // [0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 1]
+ byte[] bytes = {0, 1};
+ RedisString string = new RedisString(bytes);
+ assertThat(string.bitpos(1, 0, null)).isEqualTo(15);
+ }
+
+ @Test
+ public void bitposReturnsBitPositionWhenStartIsNotMoreNegativeThanByteArrayLength() {
+ // [0, 0, 0, 0, 0, 0, 0, 1, 0, 0, 0, 0, 0, 0, 0, 1]
+ byte[] bytes = {1, 1};
+ RedisString string = new RedisString(bytes);
+ // Index from the end of the byte array, a value of -1 is equivalent to bytes.length - 2
+ assertThat(string.bitpos(1, -1, null)).isEqualTo(15);
+ }
+
+ @Test
+ public void bitposReturnsBitPositionWhenStartIsMoreNegativeThanByteArrayLength() {
+ // [0, 0, 0, 0, 0, 0, 0, 1, 0, 0, 0, 0, 0, 0, 0, 1]
+ byte[] bytes = {1, 1};
+ RedisString string = new RedisString(bytes);
+ // Index from the end of the byte array, a value of -3 is equivalent to bytes.length - 4
+ assertThat(string.bitpos(1, -3, null)).isEqualTo(7);
+ }
+
+ @Test
+ public void bitposReturnsBitPositionWhenEndIsNotMoreNegativeThanByteArrayLength() {
+ // [0, 0, 0, 0, 0, 0, 0, 1, 0, 0, 0, 0, 0, 0, 0, 1]
+ byte[] bytes = {1, 1};
+ RedisString string = new RedisString(bytes);
+ // Index from the end of the byte array, a value of -1 is equivalent to bytes.length - 2
+ assertThat(string.bitpos(1, 0, -1)).isEqualTo(7);
+ }
+
+ @Test
+ public void bitposReturnsBitPositionWhenStartAndEndAreMoreNegativeThanByteArrayLengthAndFirstByteContainsBit() {
+ // [0, 0, 0, 0, 0, 0, 0, 1, 0, 0, 0, 0, 0, 0, 0, 1]
+ byte[] bytes = {1, 1};
+ RedisString string = new RedisString(bytes);
+ // Index from the end of the byte array, a value of -3 is equivalent to bytes.length - 4
+ assertThat(string.bitpos(1, -3, -3)).isEqualTo(7);
+ }
+
+ @Test
+ public void bitposReturnsNegativeOneWhenBitIsNotFound() {
+ // [0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0]
+ byte[] bytes = {0, 0};
+ RedisString string = new RedisString(bytes);
+ assertThat(string.bitpos(1, 0, null)).isEqualTo(-1);
+ }
+
+ @Test
+ public void bitposReturnsNegativeOneWhenBitIsNotFoundAndBitIsZeroAndEndIsSet() {
+ // [1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1]
+ byte[] bytes = {-1, -1};
+ RedisString string = new RedisString(bytes);
+ assertThat(string.bitpos(0, 0, bytes.length)).isEqualTo(-1);
+ }
+
+ @Test
+ public void bitposReturnsValueBitArrayLengthWhenBitIsNotFoundAndBitIsZeroAndEndIsNotSet() {
+ // [1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1]
+ byte[] bytes = {-1, -1};
+ RedisString string = new RedisString(bytes);
+ assertThat(string.bitpos(0, 0, null)).isEqualTo(bytes.length * 8);
+ }
+
+ @Test
+ public void bitposReturnsBitPositionWhenEndIsGreaterThanOrEqualToValueArrayLength() {
+ // [0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 1]
+ byte[] bytes = {0, 1};
+ RedisString string = new RedisString(bytes);
+ assertThat(string.bitpos(1, 0, bytes.length)).isEqualTo(15);
+ assertThat(string.bitpos(1, 0, bytes.length + 1)).isEqualTo(15);
+ }
+
+ @Test
+ public void bitposReturnsBitPositionWhenStartAndEndAreGreaterThanOrEqualToValueArrayLengthAndLastByteContainsBit() {
+ // [0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 1]
+ byte[] bytes = {0, 1};
+ RedisString string = new RedisString(bytes);
+ assertThat(string.bitpos(1, bytes.length, null)).isEqualTo(15);
+ assertThat(string.bitpos(1, bytes.length + 1, null)).isEqualTo(15);
+ }
+
+ @Test
+ public void bitposReturnsNegativeOneWhenStartAndEndAreGreaterThanOrEqualToValueArrayLengthAndLastByteIsDoesNotContainBit() {
+ // [0, 0, 0, 0, 0, 0, 0, 1, 0, 0, 0, 0, 0, 0, 0, 0]
+ byte[] bytes = {1, 0};
+ RedisString string = new RedisString(bytes);
+ assertThat(string.bitpos(1, bytes.length, bytes.length)).isEqualTo(-1);
+ assertThat(string.bitpos(1, bytes.length + 1, bytes.length + 1)).isEqualTo(-1);
+ }
+
+ @Test
+ public void bitposReturnsNegativeOneWhenValueIsEmptyArray() {
+ RedisString string = new RedisString(new byte[] {});
+ assertThat(string.bitpos(1, 0, 1)).isEqualTo(-1);
+ }
+
+ @Test
+ public void bitposReturnsNegativeOneWhenEndIsLessThanStart() {
+ RedisString string = new RedisString(new byte[] {0, 1});
+ assertThat(string.bitpos(1, 1, 0)).isEqualTo(-1);
+ }
+
+ @Test
+ public void bitposReturnsNegativeOneWhenBitIsNotZeroOrOne() {
+ RedisString string = new RedisString(new byte[] {0, 1});
+ assertThat(string.bitpos(2, 0, 1)).isEqualTo(-1);
+ }
+
Review comment:
Where possible, we favor adding these types of tests at the integration level (see `AbstractBitPosIntegrationTest`) so we can make sure our behavior matches Native Redis. Could we add these there instead?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] jdeppe-pivotal commented on a change in pull request #6464: GEODE-9221: Remove uses of ByteArrayWrapper from RedisString
Posted by GitBox <gi...@apache.org>.
jdeppe-pivotal commented on a change in pull request #6464:
URL: https://github.com/apache/geode/pull/6464#discussion_r631037662
##########
File path: geode-apis-compatible-with-redis/src/main/java/org/apache/geode/redis/internal/data/RedisString.java
##########
@@ -684,23 +675,23 @@ public boolean equals(Object o) {
return false;
}
RedisString that = (RedisString) o;
- return Objects.equals(value, that.value);
+ return Arrays.equals(value, that.value);
}
@Override
public int hashCode() {
return Objects.hash(super.hashCode(), value);
}
- ByteArrayWrapper getValue() {
+ byte[] getValue() {
return value;
}
@Override
public String toString() {
return "RedisString{" +
super.toString() + ", " +
- "value=" + value +
+ "value=" + new String(value, StandardCharsets.UTF_8) +
Review comment:
This should not do any explicit encoding; since we wouldn't know what the original encoding was.
##########
File path: geode-apis-compatible-with-redis/src/test/java/org/apache/geode/redis/internal/data/RedisStringTest.java
##########
@@ -43,85 +45,81 @@
@BeforeClass
public static void beforeClass() {
- InternalDataSerializer
- .getDSFIDSerializer()
- .registerDSFID(DataSerializableFixedID.REDIS_BYTE_ARRAY_WRAPPER, ByteArrayWrapper.class);
InternalDataSerializer.getDSFIDSerializer().registerDSFID(
DataSerializableFixedID.REDIS_STRING_ID,
RedisString.class);
}
@Test
public void constructorSetsValue() {
- ByteArrayWrapper byteArrayWrapper = new ByteArrayWrapper(new byte[] {0, 1, 2});
- RedisString string = new RedisString(byteArrayWrapper);
- ByteArrayWrapper returnedByteArrayWrapper = string.get();
- assertThat(returnedByteArrayWrapper).isNotNull();
- assertThat(returnedByteArrayWrapper.value).isEqualTo(byteArrayWrapper.value);
+ byte[] bytes = {0, 1, 2};
+ RedisString string = new RedisString(bytes);
+ byte[] returnedBytes = string.get();
+ assertThat(returnedBytes).isNotNull();
+ assertThat(returnedBytes).isEqualTo(bytes);
}
@Test
public void setSetsValue() {
RedisString string = new RedisString();
- string.set(new ByteArrayWrapper(new byte[] {0, 1, 2}));
- ByteArrayWrapper returnedByteArrayWrapper = string.get();
- assertThat(returnedByteArrayWrapper).isNotNull();
- assertThat(returnedByteArrayWrapper.value)
- .isEqualTo(new ByteArrayWrapper(new byte[] {0, 1, 2}).value);
+ byte[] bytes = {0, 1, 2};
+ string.set(bytes);
+ byte[] returnedBytes = string.get();
+ assertThat(returnedBytes).isNotNull();
+ assertThat(returnedBytes).isEqualTo(bytes);
}
@Test
public void getReturnsSetValue() {
- RedisString string = new RedisString(new ByteArrayWrapper(new byte[] {0, 1}));
- ByteArrayWrapper returnedByteArrayWrapper = string.get();
- assertThat(returnedByteArrayWrapper).isNotNull();
- assertThat(returnedByteArrayWrapper.value)
- .isEqualTo(new ByteArrayWrapper(new byte[] {0, 1}).value);
+ byte[] bytes = {0, 1};
+ RedisString string = new RedisString(bytes);
+ byte[] returnedBytes = string.get();
+ assertThat(returnedBytes).isNotNull();
+ assertThat(returnedBytes).isEqualTo(bytes);
}
@Test
public void appendResizesByteArray() {
- // allows unchecked cast of mock to Region<RedisKey, RedisData>
- @SuppressWarnings("unchecked")
- Region<RedisKey, RedisData> region = mock(Region.class);
- RedisString redisString = new RedisString(new ByteArrayWrapper(new byte[] {0, 1}));
- ByteArrayWrapper part2 = new ByteArrayWrapper(new byte[] {2, 3, 4, 5});
+ Region<RedisKey, RedisData> region = uncheckedCast(mock(Region.class));
+ RedisString redisString = new RedisString(new byte[] {0, 1});
int redisStringSize = redisString.strlen();
- int part2Size = part2.length();
- int appendedStringSize = redisString.append(part2, region, null);
- assertThat(appendedStringSize).isEqualTo(redisStringSize + part2Size);
+ byte[] bytesToAppend = {2, 3, 4, 5};
+ int appendedSize = bytesToAppend.length;
+ int appendedStringSize = redisString.append(region, null, bytesToAppend);
+ assertThat(appendedStringSize).isEqualTo(redisStringSize + appendedSize);
}
@Test
public void appendStoresStableDelta() throws IOException {
- // allows unchecked cast of mock to Region<RedisKey, RedisData>
- @SuppressWarnings("unchecked")
- Region<RedisKey, RedisData> region = mock(Region.class);
- RedisString o1 = new RedisString(new ByteArrayWrapper(new byte[] {0, 1}));
- ByteArrayWrapper part2 = new ByteArrayWrapper(new byte[] {2, 3});
- o1.append(part2, region, null);
- assertThat(o1.hasDelta()).isTrue();
- assertThat(o1.get()).isEqualTo(new ByteArrayWrapper(new byte[] {0, 1, 2, 3}));
+ Region<RedisKey, RedisData> region = uncheckedCast(mock(Region.class));
+ byte[] baseBytes = {0, 1};
+ byte[] bytesToAppend = {2, 3};
+ byte[] baseAndAppendedBytes = {0, 1, 2, 3};
+
+ RedisString stringOne = new RedisString(baseBytes);
+ stringOne.append(region, null, bytesToAppend);
+ assertThat(stringOne.hasDelta()).isTrue();
+ assertThat(stringOne.get()).isEqualTo(baseAndAppendedBytes);
HeapDataOutputStream out = new HeapDataOutputStream(100);
- o1.toDelta(out);
- assertThat(o1.hasDelta()).isFalse();
+ stringOne.toDelta(out);
+ assertThat(stringOne.hasDelta()).isFalse();
ByteArrayDataInput in = new ByteArrayDataInput(out.toByteArray());
- RedisString o2 = new RedisString(new ByteArrayWrapper(new byte[] {0, 1}));
- assertThat(o2).isNotEqualTo(o1);
- o2.fromDelta(in);
- assertThat(o2.get()).isEqualTo(new ByteArrayWrapper(new byte[] {0, 1, 2, 3}));
- assertThat(o2).isEqualTo(o1);
+ RedisString stringTwo = new RedisString(baseBytes);
+ assertThat(stringTwo).isNotEqualTo(stringOne);
+ stringTwo.fromDelta(in);
+ assertThat(stringTwo.get()).isEqualTo(baseAndAppendedBytes);
+ assertThat(stringTwo).isEqualTo(stringOne);
}
@Test
public void confirmSerializationIsStable() throws IOException, ClassNotFoundException {
- RedisString o1 = new RedisString(new ByteArrayWrapper(new byte[] {0, 1, 2, 3}));
- o1.setExpirationTimestampNoDelta(1000);
+ RedisString stringOne = new RedisString(new byte[] {0, 1, 2, 3});
+ stringOne.setExpirationTimestampNoDelta(1000);
HeapDataOutputStream outputStream = new HeapDataOutputStream(100);
- DataSerializer.writeObject(o1, outputStream);
+ DataSerializer.writeObject(stringOne, outputStream);
ByteArrayDataInput dataInput = new ByteArrayDataInput(outputStream.toByteArray());
- RedisString o2 = DataSerializer.readObject(dataInput);
- assertThat(o2).isEqualTo(o1);
+ RedisString stringTwo = DataSerializer.readObject(dataInput);
+ assertThat(stringTwo).isEqualTo(stringOne);
Review comment:
Since the expiration is being set on `stringOne`, it would probably make sense to also assert it on `stringTwo`.
##########
File path: geode-apis-compatible-with-redis/src/main/java/org/apache/geode/redis/internal/data/RedisString.java
##########
@@ -233,18 +227,17 @@ public int bitpos(Region<RedisKey, RedisData> region, RedisKey key, int bit,
if (start > length) {
start = length - 1;
}
- if (end > length) {
+ if (end >= length) {
Review comment:
Well spotted! Please would you also add a test that validates this change.
##########
File path: geode-apis-compatible-with-redis/src/main/java/org/apache/geode/redis/internal/data/RedisString.java
##########
@@ -718,15 +709,16 @@ protected void handleSetExpiration(SetOptions options) {
////// methods that modify the "value" field ////////////
protected void valueAppend(byte[] bytes) {
- value.append(bytes);
- }
-
- protected void valueSet(ByteArrayWrapper newValue) {
- value = newValue;
+ int initialLength = value.length;
Review comment:
Since this is being pulled from `ByteArrayWrapper`, I think it can be deleted from that class now.
##########
File path: geode-apis-compatible-with-redis/src/main/java/org/apache/geode/redis/internal/data/RedisString.java
##########
@@ -718,15 +709,16 @@ protected void handleSetExpiration(SetOptions options) {
////// methods that modify the "value" field ////////////
protected void valueAppend(byte[] bytes) {
- value.append(bytes);
- }
-
- protected void valueSet(ByteArrayWrapper newValue) {
- value = newValue;
+ int initialLength = value.length;
+ int additionalLength = bytes.length;
+ byte[] combined = new byte[initialLength + additionalLength];
+ System.arraycopy(value, 0, combined, 0, initialLength);
+ System.arraycopy(bytes, 0, combined, initialLength, additionalLength);
+ value = combined;
}
- protected void valueSetBytes(byte[] bytes) {
- value.setBytes(bytes);
+ protected void valueSet(byte[] bytes) {
Review comment:
This class uses a mix of this method and direct assignment to `value`. I'd prefer just one approach. Don't mind which, but would lean towards just direct `value = ...`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] sabbey37 commented on a change in pull request #6464: GEODE-9221: Remove uses of ByteArrayWrapper from RedisString
Posted by GitBox <gi...@apache.org>.
sabbey37 commented on a change in pull request #6464:
URL: https://github.com/apache/geode/pull/6464#discussion_r631175507
##########
File path: geode-apis-compatible-with-redis/src/main/java/org/apache/geode/redis/internal/data/RedisString.java
##########
@@ -233,18 +227,17 @@ public int bitpos(Region<RedisKey, RedisData> region, RedisKey key, int bit,
if (start > length) {
start = length - 1;
}
- if (end > length) {
+ if (end >= length) {
Review comment:
Definitely appreciate the find, but I'm worried about scope creep here. `BITPOS` is still an unsupported command, and there hasn't been a story to fully implement it and test it yet. I am not sure where it is on our list of priorities.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] DonalEvans commented on a change in pull request #6464: GEODE-9221: Remove uses of ByteArrayWrapper from RedisString
Posted by GitBox <gi...@apache.org>.
DonalEvans commented on a change in pull request #6464:
URL: https://github.com/apache/geode/pull/6464#discussion_r632083175
##########
File path: geode-apis-compatible-with-redis/src/integrationTest/java/org/apache/geode/redis/internal/executor/string/AbstractBitPosIntegrationTest.java
##########
@@ -115,11 +123,69 @@ public void bitposWithStart_givenOneInLastByte() {
assertThat(jedis.bitpos(key, true, new BitPosParams(-1))).isEqualTo(7 + 3 * 8);
}
+ @Test
+ public void bitposWithStart_givenStartGreaterThanOrEqualToByteArrayLength() {
+ byte[] key = {1, 2, 3};
+ byte[] bytes = {1, 1, 1, 1};
+ jedis.set(key, bytes);
+ assertThat(jedis.bitpos(key, true, new BitPosParams(bytes.length))).isEqualTo(7 + 3 * 8);
+ assertThat(jedis.bitpos(key, true, new BitPosParams(bytes.length + 1))).isEqualTo(7 + 3 * 8);
+ }
Review comment:
Agreed. I'll remove this test case. If/when bitpos becomes a supported command, proper coverage can be established.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org