Correct comparator of item sorter in Dynamo DB adapter #327
Conversation
|
@thongdk8 Sorry, I forgot one thing. Can you please add a unit test for this? We basically need a unit test to make sure the bug is really fixed in this kind of case. |
| @@ -264,10 +266,27 @@ public void sort_ItemsGivenWithDescOrderingText_ShouldSortProperly() { | |||
| @Test | |||
| public void sort_ItemsGivenWithDescOrderingBlob_ShouldSortProperly() { | |||
| // Arrange | |||
| Random random = new Random(777); | |||
There was a problem hiding this comment.
I'm just wondering if there is any benefit of using Random with a fixed seed here.
I feel it's better to use fixed hard-coded byte arrays so that we (people) can see what is the expected result.
What do you think?
There was a problem hiding this comment.
There was a problem hiding this comment.
It makes more sense if we test many cases using loop and random but the test was only comparing two byte-arrays, that's why I asked.
There was a problem hiding this comment.
That's right. So should we use random and for loop here or keep it like the new commit I made?
There was a problem hiding this comment.
I think the current code is fine and consistent with other tests.
If you feel it's not tested well, we can add more tests with loop and random. (please add instead of update in such a case)
There was a problem hiding this comment.
@thongdk8 I will merge this. If you want add more tests with loop and random, please create a new PR for that. Thanks.
Current implementation using byte buffer comparator so it makes the result when scanning with clustering key of blob values not correctly. This PR made for correct the comparator. Please take a look. Thank you.