refactor(arrow/avro): migrate from hamba/avro to twmb/avro#830
Open
prochac wants to merge 1 commit into
Open
Conversation
Switch the avro reader from hamba/avro to twmb/avro for schema parsing
and value decoding. The new library hands back typed Go values (e.g.
*big.Rat for decimals, [16]byte for fixed-uuid) instead of []byte, which
shortens the data-side helpers and lets us fix a stack of bugs the old
[]byte arms were masking.
The exported arrow/avro surface is preserved: NewOCFReader, OCFReader
and its methods, the Option/With… helpers, and ArrowSchemaFromAvro
all keep their signatures. A new ArrowSchemaFromAvroJSON is added
alongside as the recommended entry point — it doesn't couple callers
to a particular Avro library through its type signature.
ArrowSchemaFromAvro is now marked Deprecated and serializes the hamba
schema via json.Marshal before re-parsing with twmb; the
github.com/hamba/avro/v2 dependency is kept so downstream callers
continue to compile.
Newly supported logical types (the mapping had them commented out
under hamba):
- local-timestamp-millis
- local-timestamp-micros
- local-timestamp-nanos
- timestamp-nanos
Fixes:
- fixed(16)+uuid rows are no longer silently dropped — the
UUIDBuilder now handles [16]byte directly.
- ArrowSchemaFromAvro (deprecated wrapper) preserves logical-type
annotations on fixed (uuid/decimal/duration). The wrapper now uses
json.Marshal, which dispatches to each schema type's MarshalJSON;
hambaAvro.Schema.String() returns Avro Parsing Canonical Form,
which strips logical types by spec, and was the wrong serializer
to bridge between libraries.
- Heterogeneous and non-nullable multi-branch unions (e.g.
["null", A, B], ["int", "string"]) used to silently mis-decode
against the wrong branch or fall through to an opaque downstream
nil-field error. They now fail upfront with "unsupported avro
union at <path>".
- Nullable unions written as ["T", "null"] (with null in the second
position) now resolve to T as expected; the old code always took
Types()[1] and produced a Null-typed column for that ordering.
- Map-of-primitive schemas (e.g. {"type":"map","values":"int"}) now
parse; the old code asserted the value type to NamedSchema and
panicked on primitive values.
- Unknown SchemaNode types used to leave a nameless nil-typed field
that exploded in the record builder; now they fail with
"unhandled avro type %q", surfaced as "invalid avro schema".
- Duration's []byte arm read Uint16 with gaps and overflowed uint32
on the milli multiply — gone; only the avro.Duration arm remains.
- append*Data helpers no longer silently no-op or fmt.Sprint-coerce
unknown inputs; they return "unsupported value of type %T".
- Enum-symbol mismatches in appendBinaryDictData surface a clear
error instead of a generic dictionary builder failure.
- Schema-conversion errors wrap with %w so callers can
errors.Unwrap past arrow.ErrInvalid.
OCF wire format is unchanged: twmb/avro/ocf supports the same codecs
(null, deflate, snappy, zstd) and files written by hamba remain
readable.
zeroshade
requested changes
May 27, 2026
Member
zeroshade
left a comment
zeroshade
left a comment
There was a problem hiding this comment.
Can you rebase and resolve the conflicts please?
Comment on lines
+91
to
+94
| node.namedCache[s.Name] = s | ||
| if s.Namespace != "" { | ||
| node.namedCache[s.Namespace+"."+s.Name] = s | ||
| } |
Member
There was a problem hiding this comment.
It's worth noting that the hamba SchemaCache keyed on full name only, so for schemas with two records named Foo in different namespaces a bare {"type": "Foo"} will resolve to whichever was defined latest as opposed to being an error (the previous behavior under hamba).
We should either restrict to full-name lookups or document the change in behavior here that where a malformed schema may now silently produce something where hamba would have errored.
Comment on lines
-663
to
-676
| case []byte: | ||
| buf := bytes.NewBuffer(dt) | ||
| if len(dt) <= 38 { | ||
| var intData int64 | ||
| err := binary.Read(buf, binary.BigEndian, &intData) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| b.Append(decimal128.FromI64(intData)) | ||
| } else { | ||
| var bigIntData big.Int | ||
| b.Append(decimal128.FromBigInt(bigIntData.SetBytes(buf.Bytes()))) | ||
| } | ||
| case map[string]any: |
Member
There was a problem hiding this comment.
If anyone was using custom-decoders that emitted []byte or map[string]any etc. they are now going to see "unsupported value of type X" errors where old code would have accepted the input. Let's update the description on the PR (and therefore the resulting changelog later) to note this change in behavior for anyone who was using custom-decoders.
Comment on lines
+570
to
+571
| case 36: | ||
| return b.AppendValueFromString(string(dt)) |
Member
There was a problem hiding this comment.
let's add a comment here explaining why we have a case for 36 (i guess, hex-dash UUID string?)
Comment on lines
+350
to
+351
| for i := range s.Branches { | ||
| b := s.Branches[i] |
Member
There was a problem hiding this comment.
Suggested change
| for i := range s.Branches { | |
| b := s.Branches[i] | |
| for _, b := range s.Branches { |
Comment on lines
+144
to
+147
| root := schema.Root() | ||
| n := newSchemaNode() | ||
| n.schema = schema | ||
| c := n.newChild(n.schema.(avro.NamedSchema).Name(), n.schema) | ||
| n.node = root | ||
| c := n.newChild(root.Name, root) |
Member
There was a problem hiding this comment.
OCF requires a named record at the top, we should probably add a guard or explicit "must be named record" check here (the hamba code would have panic'd).
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Rationale for this change
Closes #813
What changes are included in this PR?
Primarily, the lib was changed. New UUID type support was added. Parsing simplified, that's thanks to the new lib that doesn't use
map[string]anywrapper.Are these changes tested?
All tests pass, new test for the UUID type was added
Are there any user-facing changes?
The
ArrowSchemaFromAvrofunction was deprecated, as it causes coupling with 3rd-party Avro module