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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
