-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
AVRO-4060: Use JDK to Hash Byte Array in UTF8 #3175
base: main
Are you sure you want to change the base?
Conversation
I feel there should be a test to verify that the result of Utf8.hashCode does not depend on whether the array has unused bytes in it. TestUtf8.hashCodeReused seems to largely cover that by hardcoding specific hash codes but it does not compute hash codes for equal sequences of bytes in two ways. I'm thinking about test code like Utf8 u1 = new Utf8("abcdefghi"); // length=9, bytes.length=9
u1.setByteLength(8); // length=8, bytes.length=9
Utf8 u2 = new Utf8("abcdefgh"); // length=8, bytes.length=8
assertEquals(u1.hashCode(), u2.getHashCode()); or with a hardcoded hash code in the test. |
Hello, Thank you for the feedback. I agree a unit test is appropriate here. Ideally, the hashcode implementation should return the same value for the same contents regardless on the size of the array. However, that is not currently the case because the two implementations are different. I believe AVRO-4061 will fix the discrepancy between the two implementations and then this unit test becomes possible. I also need to look at the |
I have a unit test that confirms the behavior, but it does require AVRO-4061 to be merged first. |
eba2337
to
ba04247
Compare
Unit test will fail on this PR until AVRO-401 is addressed. |
0cca0ef
to
408af58
Compare
408af58
to
64765e9
Compare
AVRO-4061 is now fixed. Added unit test for coverage. |
What is the purpose of the change
Verifying this change
This change is a trivial rework / code cleanup without any test coverage.
Documentation