Repository navigation
AVRO-4354: [java] Fix generated equals() and hashCode() semantics - #4045
sharanggupta wants to merge 2 commits into
Conversation
Since AVRO-3527 the generated equals() compares array, map and union fields with java.util.Objects.equals(), and the generated hashCode() hashes CharSequence fields with their own hashCode(). Decoded records hold GenericData.Array and Utf8 values while built records hold java.util.List and String values, so: - equals() throws "Unknown datum type" for a decoded array whose elements contain a union with a logical type, because GenericData.Array.equals() uses the global GenericData, which has no logical type conversions, and List.equals(Array) is not symmetric with Array.equals(List) for string elements; - hashCode() differs for a String and a Utf8 with the same content, although equals() treats them as equal. Array, map and non-nullable union fields are now compared and hashed through the SpecificData model of the generated class, which knows the conversions and the Avro equality rules, and CharSequence fields are hashed with the new Utf8.hashCodeFor(), the hash code of the UTF-8 encoding of the sequence, so that the generated hashCode() agrees with equals() and with SpecificData.hashCode(). The four-argument GenericData.compare() is public, so that generated code can call it.
|
Thanks @sharanggupta for tackling this. Delegating arrays/maps/unions to the model is the right direction, TL;DR
What I verified
Matrix summary (built = builder/Java types, decoded = after encode/decode)
1. Needs fixing:
|
| Record generated by 1.12.0 | hashCode() on main runtime |
on PR runtime |
|---|---|---|
{map<string>} with stringType=String |
89 | 93 |
{map<enum>} holding a Java enum value |
1270144544 (Enum.hashCode() is an identity hash, varies per JVM) |
170 (ordinal-based) |
And for a GenericRecord {map<array<string>>} with Utf8 keys/values and a 12-element array: 1850569164 -> 2061980516 (Map.hashCode() used to delegate to List.hashCode(); the bounded HashCodeCalculator now only visits the first nine elements). The cases listed in the description (Utf8-keyed maps with Utf8, numeric or record values; arrays of up to nine elements) are indeed unchanged, and decoded records keep their 1.12.1/1.12.2 hash, which is good.
The new values are arguably more correct (consistent with equality, deterministic for enums), but this is a behaviour change for every user of GenericData.hashCode, not just generated code, in an area where silent hash changes have already been reported (AVRO-4334). I'd either split it into its own PR/JIRA with its own release note, or list it explicitly in the description and release notes.
The description also says that "records generated by this compiler, records generated before 1.12.1 and GenericRecords with the same content keep the same hash code". That is not accurate in a few cases:
HashCodeCalculatoronly visits the first nine fields/elements (counter = 10,GenericData.java:1146-1205), whereas the generatedhashCode()hashes every field. For a record with 12stringfields the generatedhashCode()is 1135094951 andSpecificData.get().hashCode(record, schema)is 327880286 (already the case onmain/1.12.1+, buthashCodeOfGeneratedRecordMatchesSpecificDataonly covers a two-field record, which suggests an equivalence that does not hold in general).- "Before 1.12.1":
Utf8.hashCode()itself changed in 1.12.1 (AVRO-4334; e.g."a": 97 -> 128), so the reference point for "unchanged" is 1.12.1+, not earlier. - The map cases above.
Please reword and, if you keep that test, cover a record with more than nine fields.
5. Non-blocking: cost of Utf8.hashCodeFor(String)
For a CharSequence field holding a String, the generated hashCode() now re-scans the string on every call, where it used to hit the cached String.hashCode(). Rough numbers (plain timing loop, not JMH; JDK 21, same machine; main vs PR):
main |
PR | |
|---|---|---|
record with one string field holding a 200-char String: hashCode() |
~7-8 ns | ~240-310 ns |
same record decoded (Utf8, hash cached): hashCode() |
~7 ns | ~7 ns |
map<string> with 100 String entries: hashCode() |
~0.22 µs | ~1.9-2.3 µs |
array<string> of 1000: equals() built / decoded |
4.7-4.9 µs / 9.3-11 µs | 6.8-7.4 µs / 13-15 µs |
array<string> of 1000: hashCode() built / decoded |
~0.9-1.0 µs / ~1.1-1.2 µs | ~0.16 µs / ~0.05 µs (only nine elements are hashed) |
This is probably acceptable, but it deserves a sentence in the description: code that uses records built from Strings as hash keys will notice it (records decoded from Avro hold Utf8, whose hash is cached, and are unchanged). If you want to mitigate it: for ASCII-only content the UTF-8 hash equals 31^n + s.hashCode() (n = length), so a fast path on top of the cached String hash is possible, although it still needs a scan to prove the content is ASCII.
6. Non-blocking: BigDecimal scale vs hash
For arrays/maps/unions of decimal, equality now goes through compare(), which uses BigDecimal.compareTo (scale-insensitive), while the hash uses BigDecimal.hashCode() (scale-sensitive). So List.of(new BigDecimal("1.5")) and List.of(new BigDecimal("1.50")) in an array<decimal(9,2)> field are equals but have different hash codes (on main they were simply not equal). It only affects values that were never encoded (the conversion rejects a scale different from the schema's on write) and 1.12.0 behaved the same, so low priority; but since the JIRA is precisely about the equals/hashCode contract, a note or a test would be nice.
7. Tests and follow-ups
Coverage that would have caught the points above:
- the three map shapes (item 2) and null vs empty (item 1);
- a non-nullable union field (e.g.
["null","string","int"]):requiresModelComparisonhas a dedicated!nullablebranch that no field ingenerated_equals.avscexercises (it works, I checked, but it should be covered); stringType=String(theCharSequencebranch ofhashCodeForis not used there, and arrays/maps hold plainStrings on both sides);- a record with more than nine fields (item 4).
Out of scope here per the description, but worth tracking so that AVRO-4354 isn't considered fully done:
GenericData.AbstractArray#equalsstill usesGenericData.get()(GenericData.java:361, AVRO-4036).record.equalsnow works, but comparing the extracted list directly still throws:order.getDates().equals(expected)->Unknown datum type java.time.LocalDate(expected.equals(order.getDates())istrue). That is typical in assertions likeassertThat(order.getDates()).isEqualTo(expected).- An opt-out for the generated
equals/hashCode(item 4 of the JIRA). Given the behaviour changes above I'd like that escape hatch to be tracked. - A backport to
branch-1.12(the JIRA lists 1.12.1/1.12.2 as affected and has no fix version), see 3c. - AVRO-4198 is fixed for generated records by this PR, so it's worth referencing/closing.
Thanks again, happy to help review follow-ups.
…code The generated equals() delegated array, map and non-nullable union fields to SpecificData.compare(), which dereferences arrays and maps, so a record whose array or map field is null (possible through the constructors and setters) threw a NullPointerException where Objects.equals() returned false. Check for null before delegating. GenericData.compare() compared map values with Object.equals(), so a map whose values are arrays or maps had the same problems as array fields: "Unknown datum type" for logical types in unions, and String vs Utf8 asymmetries. Compare the values with the value schema through a new compareMaps(Map, Map, Schema); the two-argument compareMaps() keeps its behaviour. Also regenerate the expected classes of TestSpecificCompilerTool, whose equals() and hashCode() follow the new template.
|
Thank you for this review, it's far more thorough than I could have hoped for. You were right on both items that need fixing, and you also found something you couldn't have seen without building Fixed
Description corrected I rewrote the hash-stability paragraph: the reference point is 1.12.1+, the nine-element/field bound of Open for the maintainers
Locally (JDK 21, build cache disabled): |
2 participants
What is the purpose of the change
Fixes AVRO-4354: since 1.12.1 (AVRO-3527) the generated
equals()compares array, map and union fields withjava.util.Objects.equals(), and the generatedhashCode()hashesCharSequencefields with their ownhashCode(). A decoded record holdsGenericData.ArrayandUtf8values while a built record holdsjava.util.ListandStringvalues, so:equals()throwsAvroRuntimeException: Unknown datum type java.util.UUID(orLocalDate,Instant, ...) for a decoded array whose elements contain a union with a logical type, becauseGenericData.Array.equals()compares through the globalGenericData, which has no logical type conversions. It is also asymmetric:List.equals(Array)compares element by element whileArray.equals(List)goes through the model, so for string elements one direction seesStringvsUtf8and returnsfalse(AVRO-4198).hashCode()of a record holding aStringdiffers from the same record after a round trip, which holds aUtf8, althoughequals()treats them as equal, soHashSet/HashMaplookups fail.The fix keeps the fast paths of AVRO-3527 for primitive,
CharSequence, record, enum, fixed, bytes and converted logical-type fields, and changes the generated code where the runtime type of the value is not under the control of the generated class:MODEL$.compare(this.x, other.x, fieldSchema, true)and hashed withMODEL$.hashCode(this.x, fieldSchema): theSpecificDataof the generated class knows the logical type conversions and applies the Avro equality rules (String/Utf8,List/GenericData.Array, map keys) symmetrically, asSpecificRecordBase.equals()/hashCode()did before 1.12.1 for the whole record. Anullfield (possible through the constructors and setters, not the builder) is equal only tonulland is checked before delegating, soequals()never throws for it;CharSequencefields are hashed with the newUtf8.hashCodeFor(CharSequence), which computes the hash code of the UTF-8 encoding of the sequence without encoding it, so that it matchesUtf8.hashCode()for the same content andUtf8.compareSequences(), used by the generatedequals(). The result is the hash thatSpecificData.hashCode()computes for a string (used byGenericData.HashCodeCalculatornow too, which previously allocated aUtf8perString).GenericData.compare(Object, Object, Schema, boolean)(and theSpecificData/ReflectDataoverrides) is now public so that generated code can call it; the three-argumentcompare()was already public.GenericData.compare()compares the values of a map with the value schema, through a newcompareMaps(m1, m2, valueSchema), instead ofObject.equals(), so that map values that are arrays or maps get the same conversions andString/Utf8equality as array elements. Record values are therefore compared field-wise by the model, as record elements of arrays already were. The two-argumentcompareMaps()keeps its behaviour.GenericData.hashCode(Object, Schema)gains aMAPcase: it used to fall back toMap.hashCode(), whose result depends on whether the keys and values areStrings orUtf8s, whilecompareMaps()treats them as equal. Map entries are now hashed order-independently with the UTF-8 hash of the key and the schema hash of the value.Behaviour and compatibility changes
Utf8.hashCode()itself changed in 1.12.1, AVRO-4334): records generated by this compiler that holdStrings get the hash of the decoded record, which they must to honourequals(). TheMAPcase ofGenericData.hashCode()also changes the hash ofGenericRecords and of classes generated by older compilers that inheritSpecificRecordBase.hashCode(). Examples: amap<string>holdingStrings, amap<enum>holding Java enums (identity hash before, ordinal-based now), and a map whose values are arrays longer than nine elements.Utf8-keyed maps withUtf8, numeric or record values keep their hash.GenericData.HashCodeCalculatoronly visits the first nine fields or elements, so the generatedhashCode()equalsSpecificData.hashCode()only for records and arrays within that bound, as onmain.compare()ofGenericData,SpecificDataandReflectDatabecomes public. That is binary compatible, but not source compatible for a subclass that overrides it asprotected. Also new:Utf8.hashCodeFor(CharSequence),GenericData.compareMaps(Map, Map, Schema),SpecificCompiler#requiresModelComparisonandSpecificCompiler#hashCodeFor(Field, String). An alternative that widens nothing is discussed in the review: protected helpers onSpecificRecordBase/SpecificExceptionBasecalled by the template.Utf8.compareSequences()in 1.12.1, code generated by this compiler needs this runtime: on an older runtime itshashCode()fails withNoSuchMethodError(Utf8.hashCodeFor) and itsequals()withIllegalAccessError(protectedcompare). Code generated by older compilers runs on this runtime. This is worth a release note, and a reason to be careful with a backport tobranch-1.12.hashCode()of aCharSequencefield holding aStringnow scans the string on each call (about 250-300 ns for 200 characters in the measurements from the review, instead of the cachedString.hashCode()), while records decoded from Avro holdUtf8s, whose hash is cached.decimal, equality goes throughcompare()(BigDecimal.compareTo, scale-insensitive) while the hash usesBigDecimal.hashCode()(scale-sensitive), as in 1.12.0. This only affects values with a scale different from the schema's, which can't be encoded.Not addressed here, as they look like separate decisions, and worth tracking so AVRO-4354 isn't considered complete without them:
GenericData.Array.equals()called directly with logical type elements, for exampleorder.getDates().equals(expected)(AVRO-4036).equals()/hashCode()(item 4 of the ticket).branch-1.12, see the runtime coupling above.This also fixes AVRO-4198 for generated records.
Verifying this change
This change added tests and can be verified as follows:
TestGeneratedEqualsAndHashCodein the compiler module, on a new test schemagenerated_equals.avsccompiled at build time like the otherSchemaTaskschemas: array of records with a nullableuuid, array of nullabledate, array of strings, nullable array of strings, map of strings, map of arrays of a nullableuuid, map of arrays of strings, map of maps of strings, a non-nullable union ofstringandint,nullarray/map fields against empty ones,CharSequencefields with aHashSetlookup, and generatedhashCode()equal toSpecificData.hashCode()for a small record. The map tests and thenulltest were added after review: they fail on the first commit, with the sameUnknown datum typeexception, the two asymmetries and aNullPointerException. Onmain,arrayOfRecordsWithNullableLogicalTypeandarrayOfNullableLogicalTypeerror withAvroRuntimeException: Unknown datum type java.util.UUID/java.time.LocalDate,arrayOfStrings,nullableArrayOfStringsandmapOfStringsfail onafter.equals(before), and the two hash tests fail with different hash codes (4066 vs 5058 forEqualsNamed("a", "b")).TestUtf8.hashCodeForMatchesUtf8HashCodeand...ForRandomStrings:Utf8.hashCodeFor()equalsnew Utf8(s).hashCode()for ASCII, 2-, 3- and 4-byte characters, unpaired surrogates and 1000 random strings.TestGenericData.testMapHashCodeIsConsistentWithEquals: aString-keyed and aUtf8-keyed map with the same content compare equal and hash the same, and theUtf8map keeps its previous hash code.TestSpecificCompilerToolintools/src/test/compiler/output*are regenerated. Only theequals()/hashCode()lines change.avro,compiler,tools,ipc,idl,maven-pluginandintegration-test/codegen-testpass withinstall, andspotless:checkis clean.Documentation
Utf8.hashCodeFor()and the template helpers have JavaDoc)