schema_node: return *SchemaNode from Schema.Root - #46
Merged
Merged
Conversation
Closes the reachability half of issue #42. Root returned a SchemaNode by value, and a function result is not addressable, so the pointer-receiver SchemaNode.Schema could not be called on it at all: s.Root().Schema() // cannot call pointer method Schema on avro.SchemaNode Callers had to bind a temporary first, and the test suite had reached for (&n).Schema() to get around it. Returning a pointer makes the direct form compile. SchemaField.Type stays a VALUE, deliberately. Indexing a slice yields an addressable element no matter how the slice was reached, so .Fields[i].Type is addressable either way and s.Root().Fields[0].Type.Schema() already worked before this change. Making Type a pointer would have bought nothing and cost nil checks at every composite literal. The change is purely syntactic: Root re-parses s.full and builds a new tree on every call, so the pointer it now hands back points at a tree the caller already exclusively owned. No shared state becomes reachable and no caller can mutate anything another caller sees. The library itself has zero non-test callers of Root — every other match in non-test source is a comment. 38 test call sites adapted mechanically in two classes the compiler names exactly: a now-pointer passed where a value is wanted (deref), and &x that became **SchemaNode (drop the &). FOUR sites the compiler CANNOT catch, all in node_ref_schema_test.go: reflect.ValueOf(root).Field(i) panics with "call of reflect.Value.Field on ptr Value" once root is a pointer. The first -race run found one and died there, taking the rest of the package with it; a sweep for the pattern found the other three before a second run. They now go through .Elem(). TestNodeRefSchema_ConvertsOffTheRootExpression pins the forms. It is non-vacuous: reverting the signature fails the build at s.Root().Schema(), verified. It fails there and ONLY there, which is why the comment says the nested cases pin the value-field reach rather than the pointer return. Also trims one clause from SchemaField.Default's doc that told the reader what they could then do with the value.
A first pass at comment noise, prompted by a doc I wrote explaining that
Root returns a pointer so a method could be called on it — telling a Go
programmer how Go works. A sweep found the same class elsewhere.
Cut, in exported godoc:
- Schema's nil-*Schema paragraph, entirely. That a nil pointer panics and
that constructors return a value or an error is not news to anyone.
- "The data on disk doesn't change, but your code expects the new layout"
restated the two sentences before it.
- "(Go itself makes such a field reference a compile error)".
- "Go's map iteration order is randomized" — the library's choice not to
sort keys is the fact worth stating; how Go maps work is not.
- "so you can pass Default straight back to AppendEncode without
conversion" — the preceding clause already says the bytes are decoded.
- "for readability", on Duration.String.
- RatFromBytes' walkthrough of what a callback receives and what it can
then do with it.
- MustZstdCodec's "This is useful for inline codec creation" and its
sample call.
- WithCodec's walkthrough of how a typed nil arises.
And one internal comment: isNilCodec carried 24 lines for a small
predicate, teaching typed-nil and cross-referencing avro.Schema.Decode from
inside ocf. Now 14, keeping only what justifies the code — why the kinds are
enumerated, why Interface cannot occur, and why one predicate answers this
everywhere.
Comments that justify the code next to them were left alone even where they
discuss Go: why a bound is taken in float space before the int conversion,
why the depth limit is 1000, why an .IsValid() guard exists. Deleting those
invites deleting the guard they explain.
A doc round has no oracle — every test passes whichever comments you cut —
so the gate is mechanical: the diff for these six files touches zero
non-comment lines, verified, and schema_node.go's only code change is the
Root signature that -race already covered.
|
Thanks for the heads up! I am now using my own custom implementation to parse the schema; since in this use case I don't use the encoders & decoders I can do it with less overhead. So this change has no impact for me. Currently only using this library for encoding OCF and wire format avro bytes. |
Owner
Author
|
@Willem-J-an when I finally get a minor tag out I'm wondering if the library would be a fine replacement for your custom implementation - or if not, if you're open to sharing some feedback on what would make this better. |
|
I'm doing a low memory low alloc vectorized arrow transcoder, like this but in go: https://arrow.apache.org/blog/2025/10/23/introducing-arrow-avro/ This library is not a good match for various reasons:
Some of the points are not really weaknesses of the library but just my use case being different and not mapping well to a general purpose avro encoder/decoder. Maybe some of this is helpful, maybe not 😁 |
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.
Makes
Schema.Rootreturn*SchemaNodeinstead ofSchemaNode.Why
SchemaNode.Schema()has a pointer receiver, and a function result is notaddressable, so the obvious call never compiled:
Callers had to bind a temporary first. The test suite had resorted to
(&n).Schema()to work around it. This follows on from #42 — extracting ausable sub-schema is the point of that tree, and reaching one shouldn't
require knowing why the direct form fails to build.
SchemaField.Typestays a value, deliberately. Indexing a slice yields anaddressable element regardless of how the slice was reached, so
s.Root().Fields[0].Type.Schema()already worked before this change.Making
Typea pointer would have bought nothing and cost nil checks atevery composite literal.
Yes, this is a breaking API change
Root() SchemaNodehas been public since v1.4.0 and ships in v1.7.2, soI'm not going to pretend otherwise. What actually breaks is narrow:
var n avro.SchemaNode = s.Root()— type mismatchs.Root()to something taking aSchemaNodeby valuereflect.ValueOf(s.Root()).Field(i)— panics rather than failing to buildWhat does not break is everything ordinary:
n := s.Root()followed byany field access works unchanged, because Go dereferences automatically.
I'm deeming the blast radius small to non-existent. This library is not
widely adopted yet, and
Schema.Rootis an introspection path rather thanan encode/decode one, so the set of affected callers is about as small as a
breaking change gets. Shipping it now while that's true is better than
carrying the wart forever. Not strictly kosher, and I'd rather take the hit
here than after adoption grows — I'm trying to get this library to a state
where I can call it done.
Semantics
Purely syntactic.
Rootre-parses and builds a fresh tree on every call, sothe pointer it now returns points at a tree the caller already owned
exclusively. No shared state becomes reachable and no caller can mutate
anything another caller observes.
Verification
Root()match innon-test source is a comment
reflect.ValueOf(root).Field(i)sitesneeded
.Elem()— the one breakage class the compiler cannot catchsignature fails the build at
s.Root().Schema()go test -race ./...green, zero races, zero panicsAlso trims a few doc comments that explained Go language mechanics rather
than library behavior, including one that told the reader what they could
then do with a value.
@Willem-J-an — FYI since you filed #41–#44 and are the most likely person
to be using
Schema.Root. Flagging the break directly rather than lettingyou find it in a release note. If you are relying on the value return
somewhere awkward, say so and I'll reconsider.