You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@avro.apache.org by "juliojgd (via GitHub)" <gi...@apache.org> on 2026/10/15 14:36:38 UTC
Re: [PR] AVRO-4354: [java] Fix generated equals() and hashCode() semantics [avro]
juliojgd commented on PR #4045:
URL: https://github.com/apache/avro/pull/4045#issuecomment-6194670669
Thanks @sharanggupta for tackling this. Delegating arrays/maps/unions to the model is the right direction, `Utf8.hashCodeFor` looks solid, and the description is upfront about what is out of scope. I built the PR head (`9b4b14a`) and its base (`28cb08c`) locally and went through it in some depth. TL;DR first, details and repros below (line numbers refer to the PR head).
**TL;DR**
- The three scenarios in the JIRA are fixed (plus many sibling shapes), the existing suites are green, and the new tests are discriminating (they fail on `main`, pass on the PR).
- **Needs fixing:**
1. `equals()` now throws `NullPointerException` when an array/map field is `null` (behaviour change vs `main`/1.12.1+).
2. The very same `Unknown datum type` exception (and String/Utf8 asymmetries) is still reachable through **map values**.
- **Needs an explicit decision / documentation:**
3. Widened visibility of `compare(...)` and other new public API, plus generated code that no longer runs on an older runtime.
4. `GenericData.hashCode` changes for maps (this affects `GenericRecord` and classes generated by older compilers), and the description is not accurate about hash stability in a few cases.
- **Non-blocking:** (5) cost of `Utf8.hashCodeFor(String)`; (6) `BigDecimal` scale vs hash; (7) test coverage and follow-ups (AVRO-4036, opt-out, backport).
## What I verified
- JDK 21 / Maven 3.9. The CI workflows have not run on this PR yet (only `triage`), so these are local results.
- Your 7 new tests in `TestGeneratedEqualsAndHashCode` **fail on `main`** (5 failures + 2 errors: `Unknown datum type java.util.UUID` / `java.time.LocalDate`, `4066` vs `5058`) and **pass on the PR**.
- `mvn verify` is green for `avro` (3463 tests), `idl` (39), `compiler` (99), `maven-plugin` (15), `ipc` (238) and `integration-test/codegen-test` (18). `spotless:check` is clean. I could not run the "Run all tests under Java 11" invoker IT, and I did not build `mapred`/`protobuf`/`thrift`/`tools`/`trevni`/`grpc`; nothing under `lang/java` overrides the 4-arg `compare` (see 3a).
- `Utf8.hashCodeFor` matches `new Utf8(s).hashCode()` (and `Arrays.hashCode(s.getBytes(UTF_8))`) on 3M random strings, including unpaired surrogates and 1-4 byte characters. No mismatches.
- I also ran a matrix of ~48 single-field shapes x `stringType` `CharSequence`/`String` against 1.12.0, 1.12.2, `main` and the PR. For each shape I checked `built.equals(decoded)`, `decoded.equals(built)`, `decoded.equals(decoded2)`, hash equality and `HashSet.contains`.
<details>
<summary>Matrix summary (built = builder/Java types, decoded = after encode/decode)</summary>
| Field type (single field of the record) | `main` | PR |
|---|---|---|
| JIRA 1: `array<record{["null", uuid]}>` | `decoded.equals(built)` throws | OK |
| JIRA 2: `array<["null", date]>` | throws | OK |
| JIRA 3: `string` hash, `String` vs `Utf8` | hashes differ (128 vs 159) | OK |
| `["null","string","int"]` holding a `String` | not equal, hashes differ | OK |
| `map<string>`, `array<string>`, `array<map<string>>`, `map<record>`, `map<enum>`, `map<["null",date]>`, nested arrays | not equal and/or hashes differ | OK |
| `map<array<["null", uuid]>>`, `map<array<record{["null", uuid]}>>` | throws / not equal | **throws (same exception)** |
| `map<array<string>>` | not equal both ways | **asymmetric** |
| `map<map<string>>` | not equal both ways | **not equal both ways** |
| `array<string>` / `map<string>` field `null` vs empty | `false` | **NPE** |
</details>
---
## 1. Needs fixing: `equals()` can now throw `NullPointerException`
The generated code calls `MODEL$.compare(this.f, other.f, ...)` with no null check (`record.vm:651-652`). `GenericData.compare` dereferences its arguments in the ARRAY case (`GenericData.java:1307`, `((Collection) o1).iterator()`) and in `compareMaps` (`m1.isEmpty()`). So comparing a record whose array/map field is `null` with one where it is not now throws, where `Objects.equals` used to return `false`.
The builder rejects nulls for non-nullable fields, but the public no-arg constructor, the all-args constructor and the setters do not, so `null` array/map fields are easy to hit (mapping frameworks, test fixtures, `new X()` plus a few setters).
<details>
<summary>Repro (using your EqualsOrder)</summary>
```java
EqualsOrder unset = new EqualsOrder(); // array/map fields are null
EqualsOrder defaults = EqualsOrder.newBuilder().build(); // array/map fields are empty
unset.equals(defaults); // main: false PR: NullPointerException ("a1" is null)
List.of(defaults).contains(unset); // main: false PR: NullPointerException
unset.hashCode(); // fine on both (MODEL$.hashCode(null, ...) returns 0)
```
</details>
1.12.0 (`SpecificRecordBase#equals`) threw the same NPE, so this restores the 1.12.0 behaviour. It is still a change for everyone on 1.12.1+, and a nasty one: `assertEquals` / `List.contains` blow up instead of failing / returning `false`.
**Suggestion:** guard against `null` in the generated code before delegating, e.g. `this.f == null ? other.f != null : (other.f == null || MODEL$.compare(...) != 0)`, or centralize it in a null-safe helper (see 3b). And add a test (null vs empty/default in both directions, plus null vs null):
```java
@Test
void nullCollectionFieldsDoNotThrow() {
EqualsOrder unset = new EqualsOrder();
EqualsOrder defaults = EqualsOrder.newBuilder().build();
assertNotEquals(unset, defaults);
assertNotEquals(defaults, unset);
assertEquals(unset, new EqualsOrder());
assertEquals(unset.hashCode(), new EqualsOrder().hashCode());
}
```
## 2. Needs fixing: the same exception (and asymmetries) is still reachable through map values
`GenericData.compare` handles `MAP` by delegating to `compareMaps` (`GenericData.java:1318-1320`), which compares the *values* with `Object.equals` (`value.equals(value2)`, `GenericData.java:1271`), special-casing only `Utf8` vs `String` for scalar values. When a map value is an array or a map, none of the machinery this PR relies on (schema, model conversions, String/Utf8 equivalence) applies:
- `map<array<["null", uuid]>>` and `map<array<record{["null", uuid]}>>`: `decoded.equals(built)` and `decoded.equals(decoded2)` throw the same `AvroRuntimeException: Unknown datum type java.util.UUID` (on `main`, `decoded.equals(decoded2)` throws as well). Stack trace on the PR head:
```
org.apache.avro.AvroRuntimeException: Unknown datum type java.util.UUID: de5714a7-0000-0000-0000-000000000001
at org.apache.avro.generic.GenericData.getSchemaName(GenericData.java:979)
at org.apache.avro.generic.GenericData.resolveUnion(GenericData.java:938)
at org.apache.avro.generic.GenericData.compare(GenericData.java:1323)
at org.apache.avro.generic.GenericData.compare(GenericData.java:1313)
at org.apache.avro.generic.GenericData$AbstractArray.equals(GenericData.java:361) <- GenericData.get(): no conversions
at org.apache.avro.generic.GenericData.compareMaps(GenericData.java:1271) <- value.equals(value2)
at org.apache.avro.generic.GenericData.compare(GenericData.java:1320)
at org.apache.avro.specific.SpecificData.compare(SpecificData.java:594)
at ...EqualsOrder.equals(...)
```
- `map<array<string>>`: asymmetric. `built.equals(decoded)` is `false` (`List<String>.equals(GenericData.Array<Utf8>)`), `decoded.equals(built)` is `true`.
- `map<map<string>>`: `false` in both directions (`Map<String,String>` vs `Map<Utf8,Utf8>`).
This is not a new regression (1.12.0 behaves the same and `main` is worse), but it is the same root cause as the JIRA, and the new `MAP` case in `HashCodeCalculator` is schema-driven while the equality side is not. Since the PR already touches `compare` and the MAP hash, I'd fix it here: thread `s.getValueType()` from the MAP case into `compareMaps` (keeping the 2-arg protected method for subclasses) and compare non-null values with `compare(v1, v2, valueSchema, true)`, keeping the existing String/Utf8 key lookup. One side effect to be aware of: record values would then be compared field-wise by the model rather than through the record's own `equals`, as record elements of arrays already are. If you prefer to leave it out of this PR, please open a follow-up JIRA and make clear that AVRO-4354 does not remove the exception for map values.
Suggested additions to `generated_equals.avsc` (fields of `EqualsOrder`) and tests (all three fail on the PR head today):
```json
{ "name": "idsByKey", "type": { "type": "map", "values": { "type": "array", "items": ["null", { "type": "string", "logicalType": "uuid" }] } }, "default": {} },
{ "name": "tagsByKey", "type": { "type": "map", "values": { "type": "array", "items": "string" } }, "default": {} },
{ "name": "nestedAttributes", "type": { "type": "map", "values": { "type": "map", "values": "string" } }, "default": {} }
```
```java
@Test
void mapOfArraysOfNullableLogicalType() throws IOException {
EqualsOrder before = EqualsOrder.newBuilder().setIdsByKey(Map.of("k", Arrays.asList(ID, null))).build();
EqualsOrder after = roundTrip(before);
assertEqualBothWays(before, after);
assertEqualBothWays(after, roundTrip(before));
}
// same shape for tagsByKey (Map.of("k", List.of("a", "b"))) and nestedAttributes (Map.of("k", Map.of("x", "y")))
```
## 3. Needs an explicit decision / documentation: new public API and runtime coupling
**3a. Widened visibility.** `compare(Object, Object, Schema, boolean)` goes from `protected` to `public` in `GenericData` (`:1289`), `SpecificData` (`:588`) and `ReflectData` (`:983`). That is source-incompatible for any third-party subclass that overrides the protected hook:
```java
public class MyData extends GenericData {
@Override
protected int compare(Object o1, Object o2, Schema s, boolean equals) {
return super.compare(o1, o2, s, equals);
}
}
// compiles against main; against the PR: "attempting to assign weaker access privileges; was public"
```
It stays binary compatible. Nothing under `lang/java` overrides it (I grepped all modules) and the build has no japicmp/revapi, so CI would not flag it. The PR also adds public `Utf8.hashCodeFor`, `SpecificCompiler#requiresModelComparison` and `SpecificCompiler#hashCodeFor(Field, String)`, and the now-public `compare` has no contract in its Javadoc (null handling, the `equals` flag, exceptions).
**3b. An alternative that widens nothing.** `SpecificRecordBase#equals` already calls the protected 4-arg `SpecificData#compare`, which is legal because both live in `org.apache.avro.specific` (`SpecificRecordBase.java:79`); `SpecificExceptionBase`, the base of generated error types, is in the same package. By default generated records/errors extend those bases (`SpecificCompiler#getSchemaParentClass`), so the template could call small `protected` helpers there (e.g. `fieldEquals(int pos, Object a, Object b)` / `fieldHashCode(int pos, Object v)`, names illustrative) that use `getSpecificData()` and the schema of field `pos`. That leaves `GenericData` / `SpecificData` / `ReflectData` untouched, limits the new API surface to the generated-code base classes, shortens the generated code (no `SCHEMA$.getFields().get(n).schema()` repeated per field), and is the natural place for the null handling from item 1.
**3c. Generated code needs the new runtime.** Code generated by the PR's compiler fails at runtime on any runtime that lacks the PR, including a 1.12.2 runtime:
- record with a `CharSequence` field, `hashCode()`: `NoSuchMethodError: 'int org.apache.avro.util.Utf8.hashCodeFor(java.lang.CharSequence)'`
- record with an array/map/union field, `equals()`: `IllegalAccessError: class ...Holder tried to access protected method 'int org.apache.avro.specific.SpecificData.compare(...)'`
For comparison, code generated by 1.12.2/`main` runs on a 1.12.0 runtime for array/map fields; only `CharSequence` fields need >= 1.12.1 (`Utf8.compareSequences`, which the description mentions). Old generated code on the PR runtime works fine. This matters wherever the Avro runtime is dictated by the platform (e.g. Hadoop/Spark/Flink/Kafka classpaths) and the plugin version is chosen independently. I'd call it out in the release notes, and take it into account if this is backported to `branch-1.12`: a 1.12.3 plugin would generate classes that do not run on a 1.12.2 runtime within the same minor line (plus the API changes above in a patch release).
## 4. Needs an explicit decision / documentation: runtime hash codes change beyond the generated code
The new `MAP` case in `HashCodeCalculator` (`GenericData.java:1176`) changes `GenericData.hashCode`, hence `GenericData.Record#hashCode` and `SpecificRecordBase#hashCode` of **classes generated by older compilers** (they inherit it). Measured with classes generated by 1.12.0 (bytecode untouched, only the runtime jar switched from `main` to the PR):
| 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 `GenericRecord`s with the same content keep the same hash code". That is not accurate in a few cases:
- `HashCodeCalculator` only visits the first nine fields/elements (`counter = 10`, `GenericData.java:1146-1205`), whereas the generated `hashCode()` hashes every field. For a record with 12 `string` fields the generated `hashCode()` is 1135094951 and `SpecificData.get().hashCode(record, schema)` is 327880286 (already the case on `main`/1.12.1+, but `hashCodeOfGeneratedRecordMatchesSpecificData` only 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 `String`s 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"]`): `requiresModelComparison` has a dedicated `!nullable` branch that no field in `generated_equals.avsc` exercises (it works, I checked, but it should be covered);
- `stringType=String` (the `CharSequence` branch of `hashCodeFor` is not used there, and arrays/maps hold plain `String`s 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#equals` still uses `GenericData.get()` (`GenericData.java:361`, AVRO-4036). `record.equals` now works, but comparing the extracted list directly still throws: `order.getDates().equals(expected)` -> `Unknown datum type java.time.LocalDate` (`expected.equals(order.getDates())` is `true`). That is typical in assertions like `assertThat(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.
--
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.
To unsubscribe, e-mail: issues-unsubscribe@avro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org