feat(arrgen): Add code gen go generate machinery for zero allocation and high performance - #1274
JayJamieson wants to merge 6 commits into
Conversation
|
It looks like the files here are duplicated in the tree at |
zeroshade
changed the title
zeroshade
left a comment
zeroshade
left a comment
There was a problem hiding this comment.
Three issues need to be addressed:
- Generated schemas diverge from
arreflectfor supported untaggedtime.Time,decimal128.Num, anddecimal256.Numfields, despite the documented equivalence. Please either restore strict parity or clearly reject/document these cases and remove the equivalence-test skips. - The repository CI does not build, race-test, vet, or verify clean generation for the new
arrgenmodule because root module traversal excludes nested modules. Please add explicit CI coverage for it. - The Quick Start's unversioned
go:generatedirective fails in a normal consumer module unlessarrgenhas already been added as a dependency. Please document the required setup or use a valid pinned-version workflow.
This review was drafted by an AI-assisted tool and
confirmed by an Apache Arrow Go maintainer. After you've
addressed the points above and pushed an update, an Apache Arrow Go
maintainer — a real person — will take the next look
at the PR. The findings cite the project's review criteria;
if you think one of them is mis-applied, please reply on the
PR and a maintainer will weigh in.More on how Apache Arrow Go handles maintainer review:
CONTRIBUTING.md.
|
Also can we use testable examples rather than creating a separate "examples" folder? |
Ah bugger, I'm not sure how I managed that sorry, will fix. |
- Remove the duplicated arrgen/arrgen subtree. The two copies were functionally identical; they differed only in comment prose. - Reject the field spellings arreflect cannot infer as a struct field rather than generating a column that diverges from it: untagged (and ,timestamp) time.Time, and untagged decimal128.Num/decimal256.Num. arreflect's inferArrowType switches on reflect.Kind before the types it matches by identity, so all three infer an empty struct<> and drop the value. Every skip is gone from the equivalence tests, which now compare all 45 fixture columns and all three schemas strictly. TestArreflectCannotInferStructScalars pins the upstream behaviour so a fix there tells us to relax the rejections. - Replace the public example package with testable examples on arrgen itself, driven by the internal/gentypes fixture. - Give the nested module explicit CI coverage in ci/scripts/build.sh and ci/scripts/test.sh: build, vet, tests under the same -race/-asan args as the rest of the repo, and go generate followed by git diff --exit-code. A second pass runs the equivalence tests in a workspace over the local tree, so an arreflect change cannot break them silently. golangci-lint now covers the module in pre-commit too. - Document a go:generate workflow that resolves: go get -tool plus go tool arrgen. The unversioned go run spelling only works inside a module that already requires arrgen.
arreflect's appendValue walks every pointer level to the value and nulls the column if any level along the way is nil, so **int64 and deeper were supported there while arrgen rejected them. Strip every level instead and emit one nil check per level, plus the existing check on a nil []byte value behind them. resolveColumn now reports pointer depth rather than a nullable flag; nullability is still read from the outermost pointer alone, which is arreflect's rule. The Row fixture grows **int64, **string, **[]byte and ***float64 columns, with rows that nil an inner level while the outer one stays set, so each generated nil check is exercised separately.
Two claims were wrong. The comment on timeSpec said arreflect's own tests only pass these types as the top-level element of a slice; they in fact assert the mapping against inferPrimitiveArrowType, which a struct field never reaches. The README said buildArray matches these types before it switches on reflect.Kind, which holds for the decimals but not for time.Time, matched inside the struct branch. Both now say what the code does, with the verified FromSlice output.
A local run on the previous base reproduced none of these four failures. The Example printed the day column through the %v form of an array. Upstream v18.7.0 that arrgen/go.mod names, but not for this tree. Read each value through ValueStr instead, which formats a date the same way in both. TestGenerateGolden and TestCheckedInFilesAreUpToDate compared generator output against files on disk, byte for byte. Git checks those files out with CRLF line endings on Windows, and the generator always writes LF, so both tests failed there. Normalize the line endings on read. The Release Audit Tool rejected arrgen/go.sum. The exclude list held the bare pattern "go.sum", which fnmatch does not match against a nested path. Widen the pattern to "*go.sum". golangci-lint rewrites an if/else chain in equivalence_test.go as a switch. Apply the rewrite, so the hook makes no change.
b2d5e73 to
c3be0a0
Compare
The release verification extracts the source archive to a temporary directory and runs ci/scripts/test.sh there. That directory holds no repository, so "git diff --exit-code" reported a usage error and exited 129. Both RC Verify jobs failed on it. Run the comparison only where a repository exists. "go generate" still runs everywhere, so the archive still covers the command wrapper and its flags, and TestCheckedInFilesAreUpToDate still covers drift.
zeroshade
left a comment
zeroshade
left a comment
There was a problem hiding this comment.
Thanks for this — it's well-built, and the equivalence framing holds up under scrutiny. I read arrgen alongside arreflect rather than taking the claims on faith, and the temporal math does match exactly: timeOfDayNanos and the generated inline block both do .UTC() → midnight-in-UTC → Sub().Nanoseconds(), both divide-then-cast, and Date32/64FromTime is called identically on either path with no .UTC() on either side. The multi-level-pointer nil-check codegen in renderAppend is correct too, including the || short-circuit ordering the deref depends on.
Test quality is above average for this repo: the Row fixture covers every supported shape, row counts include 0, CheckedAllocator.AssertSize catches leaks, there's a golden test and a drift test and a CI go generate && git diff --exit-code, and the allocation assertions correctly stand down under -race/-asan. The go.work CI pass is the right call — without it the equivalence tests would silently validate against released v18.7.0's arreflect instead of the tree.
One bug to fix, plus a few nits.
Bug: generic struct types silently emit uncompilable code
findStruct (arrgen/generate.go) accepts any *types.Named whose underlying type is a struct, without checking for type parameters. Given:
type Gen[T any] struct {
A int64 `arrow:"a"`
B string `arrow:"b"`
}Generate returns no error and writes a file containing func (a *GenAppender) Append(v *Gen), which then fails to build:
gen_arrow.go:68:33: cannot use generic type Gen[T any] without instantiation
gen_arrow.go:74:40: cannot use generic type Gen[T any] without instantiation
gen_arrow.go:98:48: cannot use generic type Gen[T any] without instantiation
This is the exact failure mode doc.go promises not to have — "anything arrgen cannot map ... is a generate-time error naming the field." The fix is one condition in findStruct:
if named.TypeParams().Len() > 0 {
return nil, fmt.Errorf("generic type; arrgen generates for concrete struct types only")
}testdata/errors/errors.go is otherwise impressively exhaustive, so a Generic[T any] case belongs alongside the others.
Nits
-
findStructdoesn't unalias.type Alias = Realerrors withnot a named type, which points the reader at the wrong thing.resolveColumnalready callstypes.Unaliason field types; doing the same onobj.Type()would either make aliases work or at least let the error say "type alias". -
Dead branch in
tagOpts.validate(arrgen/tag.go). Theif o.REEearly return meanso.REEinside the following[]bool{o.Dict, o.View, o.REE}loop is always false. Harmless, but the slice literal is also an allocation on a path that runs once per field. -
Append(v *T)panics on a nilv, which the generated doc comment doesn't mention.
|
Aside from the issues above, what's the reasoning for making this an entirely separate module? I'm pretty sure all the dependencies are already part of the main arrow-go module so there's no real benefit I can see from making it a separate module that gets versioned separately. Is there a particular reason to do so? |
Thanks for the review, I'll make the fixes in follow-up commit. I opted for a separate module as I wasn't sure how open the team would be to maintaining and how often arrow-go does releases. I figured a separate module would at least allow it to easily be disabled if release/build fails for some reason and not block anyone. If it's not a problem I can rework it without being a separate module 🙏 |
2 participants
Rationale for this change
Reduce allocations and performance in hot loops and a suggestion in issue #44.
What changes are included in this PR?
New go module arrgen that is opt-in.
Are these changes tested?
Yes, equivalence tests and performance/allocation oriented
Are there any user-facing changes?
Not really, module is opt-in.