fix: convert numeric types inside array/map union branches in Avro se… - #410
Conversation
|
@mostafa please review. |
mostafa
left a comment
There was a problem hiding this comment.
Review
The diagnosis is right, and this is the correct layer to fix #408.
["null","int"] itself already works in convertUnionField (see TestConvertUnionFieldPrimitiveInt). The failure in #408 is the parent field:
{"name": "lines", "type": ["null", {"type": "array", "items": {… int unions …}}]}
convertUnionField only handled primitives and named types, so the array/map branch fell through as-is. Nested float64 values from the JSON round-trip never became int32/int64, and hamba reported unknown union type double / float64 is unsupported for Avro int. Recursing with convertFloat64ToIntForIntegerFields and returning the value unwrapped matches how hamba resolves unnamed composites.
Please don't merge yet.
Blocking
-
No tests. This needs coverage in
avro_conversion_test.gofor the #408 shape, at least:["null", {"type":"array","items": record with ["null","int"]}]+ barelineNumber: 1- the same with wrapped
{"int": 1} ["null", {"type":"map","values":"int"}](and/or a map of records with int unions)- a full
AvroSerde.Serializeround-trip, not onlyconvertUnionField
Existing tests already cover plain arrays/maps and primitive unions. The missing case is those types as a union branch.
-
gofmt. The new block is space-indented; the rest of the file is tabs. CI lint will fail. Run
gofmt(orgolangci-lint fmt) onpkg/kafka/avro.go.
Non-blocking
- Link the PR to #408 (
Fixes #408) if that is the intended close. - Swallowing
convertFloat64ToIntForIntegerFieldserrors withcontinuematches the named-schema path. If the only non-null branch is the array/map, a real conversion error (e.g.1.5forint) becomes "return as-is" and a later hamba error. Fine for consistency; returning the error would be clearer when there is no other branch. - First matching
[]any/map[string]anywins. Ambiguous unions like["null", array<int>, array<string>]can pick the wrong branch. Same first-match pattern as primitives; worth a comment if we keep it.
After tests + gofmt this looks good to merge.
|
@sagarkancharla19 Will you address this? Or do you want me to take it from here? |
|
I'll fix them today. |
- Wrap map branch values as {"map": value}: hamba/avro only encodes
map[string]any union values in the type-name-wrapped form, while bare
slices encode via the nullable-union path. The unwrapped map branch
failed Serialize with "cannot encode union map with multiple entries".
- Add tests for the mostafa#408 shape: ["null", array<record>] branches with
plain ["null", "int"] item fields (bare, wrapped, and null values),
map branches (int values and map of records), and full
AvroSerde.Serialize round-trips for both composite branch types.
- Fix gofmt indentation and document first-match branch selection.
c4ec12d to
bfba641
Compare
v2.12.2 does not recognize the exhaustruct_v5 linter name (renamed in v2.13.x), failing config validation. v2.13.2 matches the version mise installs locally (golangci-lint = "2"), keeping CI and local lint in sync.
mostafa
left a comment
There was a problem hiding this comment.
@sagarkancharla19 Thank you for your contribution.
I made some changes and will merge it now.
convertUnionField only handled primitive and named-schema (record, enum,
fixed) union branches. A branch of type array or map fell through to the
"return as-is" fallback, so the float64 values produced by the JSON
round-trip were never converted to int32/int64 for the nested int/long
fields inside the array/map elements.
As a result, any schema shaped like
{"name": "lines", "type": ["null", {"type": "array", "items": {...int fields...}}]}
failed to serialize with:
avro: unknown union type double
Recurse into array/map union branches via
convertFloat64ToIntForIntegerFields and return the value unwrapped,
since hamba/avro resolves unnamed composite branches by Go type.
fixes #408