sharanggupta commented on PR #4045: URL: https://github.com/apache/avro/pull/4045#issuecomment-6196940000
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 `tools`: the golden files of `TestSpecificCompilerTool` didn't match the new template, so CI would have failed. All of that is in the new commit 81ef4c0a0b. **Fixed** 1. **`NullPointerException` for `null` array/map fields.** The generated `equals()` now checks for `null` before delegating: `this.f == null ? other.f != null : other.f == null || MODEL$.compare(...) != 0`. `nullCollectionFieldsDoNotThrow` is your test: null vs defaults in both directions, null vs null, and the hash. 2. **Map values.** `compare()` passes the value schema to a new `compareMaps(m1, m2, valueSchema)`, which compares non-null values with `compare(v1, v2, valueSchema, true)`. The `String`/`Utf8` key lookup is unchanged. The two-argument protected `compareMaps()` keeps its behaviour for subclasses by delegating with a `null` schema. `generated_equals.avsc` has your three fields, and `mapOfArraysOfNullableLogicalType`, `mapOfArraysOfStrings` and `mapOfMapsOfStrings` cover them: they fail on the previous head (the `Unknown datum type` exception and the two asymmetries) and pass now. As you noted, record map values are now compared field-wise by the model, as record elements of arrays already are. 3. **Non-nullable union.** A `choice: ["null", "string", "int"]` field exercises the `!nullable` branch of `requiresModelComparison` (`unionOfStringAndInt`). 4. **Golden files.** The generated `Player`, `NoSettersTest` and `OptionalGetters...` classes in `tools/src/test/compiler/output*` are regenerated. The diff is only the `equals()`/`hashCode()` lines. **Description corrected** I rewrote the hash-stability paragraph: the reference point is 1.12.1+, the nine-element/field bound of `HashCodeCalculator`, and the `MAP` case of `GenericData.hashCode` changing the hash of `GenericRecord`s and of classes generated by older compilers, with your examples. I also listed the new public API, the runtime coupling of generated code (3c), the cost of `Utf8.hashCodeFor(String)` and the `BigDecimal` scale note, and turned the out-of-scope items into explicit follow-ups (AVRO-4036, the opt-out, a backport, AVRO-4198). **Open for the maintainers** - **3a/3b, public `compare()` vs protected helpers.** I haven't changed this yet, because it shapes the API. Your 3b looks better to me: protected `fieldEquals`/`fieldHashCode` helpers on `SpecificRecordBase` and `SpecificExceptionBase`, used by the template. It leaves `GenericData`/`SpecificData`/`ReflectData` untouched and shortens the generated code. One question is what the template should do for a record whose configured parent class isn't one of those bases. I'm happy to switch if a committer prefers it. - **4, the `MAP` hash change.** I can split it into its own JIRA and PR if that makes the release notes cleaner. It's needed here only for the generated `hashCode()` of map fields to agree with `equals()`. Locally (JDK 21, build cache disabled): `avro`, `compiler`, `tools`, `ipc`, `idl`, `maven-plugin` and `integration-test/codegen-test` pass with `install`, and `spotless:check` is clean. -- 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]
