You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
{{ message }}
Repository navigation
docs: make the README examples compile and stop releasing reader-owned batches - #90
Makes every Go example in the README compile, removes a double release from the record-iteration examples, and adds example_test.go so the quickstart is compiler-checked from now on.
Why
The quickstart could not be pasted into a program. The client variable is named spice, so spice.WithApiKey(...) resolves to a method on *SpiceClient rather than the package's option; NewSpiceClient() was unqualified; and two Init(...) calls were missing the trailing comma Go requires before a closing paren on its own line. The same mistakes recur further down (spice.WithHttpAddress, a second unqualified NewSpiceClient).
Compiling each README fragment in a scratch module (wrapped in func main with spice/ctx predeclared, against this branch's package):
trunk README
README.md:25 FAIL: "github.com/spiceai/gospice/v9" imported as gospice and not used | undefined: NewSpiceClient
README.md:32 FAIL: syntax error: unexpected newline in argument list; possibly missing comma or )
README.md:158 FAIL: syntax error: unexpected newline in argument list; possibly missing comma or )
README.md:300 FAIL: spice.WithHttpAddress undefined (type *gospice.SpiceClient has no field or method WithHttpAddress)
README.md:427 FAIL: "github.com/spiceai/gospice/v9" imported as gospice and not used | undefined: NewSpiceClient
this branch
20 fragments compiled, 0 of the above fail
(Two fragments fail in both and aren't code to compile: the before/after import pair in the upgrade section, and the go get line, which this PR moves to a bash fence. docs/parameterized_queries.md Example 2 also imported arrow without using it; fixed.)
The iteration examples also did defer record.Release() on each batch from reader.RecordBatch(). The reader owns that batch and releases it on the next Next(), so every batch was released twice. arrow-go hides this in a normal build, but under its assert build tag it panics. Reproduced with an in-memory IPC stream read through ipc.NewReader using the exact README loop, arrow-go v18.6.0 (this module's version):
$ go run .
3
3
done; allocator bytes outstanding: 0
$ go run -tags assert .
3
3
panic: too many releases
The examples now leave the batch to the reader, check reader.Err() after the loop (otherwise a stream that fails partway looks like a short result), and say when Retain is needed. The same over-release appeared in UPGRADE_V7_TO_V8.md and in the tests, which the README links as examples (query_test.go, adbc_test.go, benchmark_test.go, params_test.go; 25 calls, all on batches from reader.RecordBatch()). Those are removed too. One ADBC test also read NumRows() from a batch it had just released. Records the tests build themselves keep their Release.
README also pointed at go run . for the sample, but package main lives in cmd/; it now says go run ./cmd.
example_test.go carries the quickstart as Example functions. With no // Output: comment they are compiled by go vet / go test but never run, so they need no runtime and appear on pkg.go.dev. To check that the guard works, reintroducing the original spice.WithSpiceCloudAddress() into it fails vet:
vet: ./example_test.go:21:9: spice.WithSpiceCloudAddress undefined (type *gospice.SpiceClient has no field or method WithSpiceCloudAddress)
Verification
go build ./... — exit 0
go vet ./... — exit 0
gofmt -l . — no output
golangci-lint run ./... (v2.13.2; CI pins v2.12) — 0 issues. The first push missed this gate and CI's errcheck flagged defer spice.Close() in example_test.go; fixed in 769f451 with the defer func() { _ = spice.Close() }() form cmd/main.go already uses.
go test -timeout 25m ./... — 5 failures, all needing a local Spice runtime, the same 5 as on trunk on this machine: TestADBCLocalParameterizedQuery, TestADBCLocalBasicQuery, TestAsyncQueryLocal, TestLocalRuntimeDatasetRefresh, TestLocalRuntime. Every other test passed.
go test -run Example -v . — ok (the examples compile; none run, by design)
Integration tests — not run — no runtime available
…d batches
The quickstart could not be pasted into a program: the client variable was
named spice, which shadowed the package's own options (spice.WithApiKey), the
constructor was unqualified, and two Init calls were missing a trailing comma.
Five Go fragments in the README failed to compile; none do now.
The iteration example also deferred Release on each record from
reader.RecordBatch(). The reader owns that batch and releases it on the next
Next, so the example released every batch twice. Drop it, check reader.Err()
after the loop, and say when Retain is needed.
Add example_test.go carrying the quickstart, so go vet and go test compile it.
Next() also returns false when the stream fails, so without the check a
truncated result reads as a complete one. The quickstart already did this;
the parameterized-query, async, and docs examples now do too.
Remove unsafe per-batch releases from remaining examples
README.md:65
This guidance is still contradicted by copyable examples elsewhere: UPGRADE_V7_TO_V8.md:396-398 defers record.Release(), and the linked examples in query_test.go also release batches returned by RecordBatch(). Those paths retain the same double-release/assert-tag panic this PR removes here. Please remove the remaining per-batch releases (or explicitly Retain before taking ownership) so users are not directed to unsafe examples.
Every batch these loops take from reader.RecordBatch() belongs to the
reader - a Flight IPC reader, the ADBC flightsql reader, or the in-memory
reader async results are served from - and is released on the next Next().
The tests released each one again; one ADBC test also read NumRows() from a
batch it had just released. The README points to query_test.go as examples,
so these are copyable too. Records the tests build themselves keep their
Release.
That review had no line-comment thread. Its headline was that other linked documentation examples still release reader-owned batches.
Removed the extra release from UPGRADE_V7_TO_V8.md and from every test that takes a batch from reader.RecordBatch(): query_test.go (6), adbc_test.go (10), benchmark_test.go (7) and params_test.go (2). Each of those readers releases the batch on the next Next(): flight.NewRecordReader is an ipc.Reader ("valid until the next call to Next", arrow/ipc/reader.go:283), and the ADBC flightsql reader's Next starts with r.rec.Release() (driver/flightsql/record_reader.go:200, adbc v1.11.0). leasedRecordReader in adbc.go passes RecordBatch straight through. One ADBC test also read NumRows() from a batch it had just released; that's gone too. The records that session_test.go and async_offline_test.go build themselves keep their Release. async.go was already correct: it Retains each batch before keeping it.
go vet ./... exits 0, golangci-lint run ./... prints 0 issues., and go test ./... has the same 5 runtime-only failures as trunk (TestADBCLocalParameterizedQuery, TestADBCLocalBasicQuery, TestAsyncQueryLocal, TestLocalRuntimeDatasetRefresh, TestLocalRuntime).
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
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.
What
Makes every Go example in the README compile, removes a double release from the record-iteration examples, and adds
example_test.goso the quickstart is compiler-checked from now on.Why
The quickstart could not be pasted into a program. The client variable is named
spice, sospice.WithApiKey(...)resolves to a method on*SpiceClientrather than the package's option;NewSpiceClient()was unqualified; and twoInit(...)calls were missing the trailing comma Go requires before a closing paren on its own line. The same mistakes recur further down (spice.WithHttpAddress, a second unqualifiedNewSpiceClient).Compiling each README fragment in a scratch module (wrapped in
func mainwithspice/ctxpredeclared, against this branch's package):(Two fragments fail in both and aren't code to compile: the before/after
importpair in the upgrade section, and thego getline, which this PR moves to abashfence.docs/parameterized_queries.mdExample 2 also importedarrowwithout using it; fixed.)The iteration examples also did
defer record.Release()on each batch fromreader.RecordBatch(). The reader owns that batch and releases it on the nextNext(), so every batch was released twice. arrow-go hides this in a normal build, but under itsassertbuild tag it panics. Reproduced with an in-memory IPC stream read throughipc.NewReaderusing the exact README loop, arrow-go v18.6.0 (this module's version):The examples now leave the batch to the reader, check
reader.Err()after the loop (otherwise a stream that fails partway looks like a short result), and say whenRetainis needed. The same over-release appeared inUPGRADE_V7_TO_V8.mdand in the tests, which the README links as examples (query_test.go,adbc_test.go,benchmark_test.go,params_test.go; 25 calls, all on batches fromreader.RecordBatch()). Those are removed too. One ADBC test also readNumRows()from a batch it had just released. Records the tests build themselves keep theirRelease.READMEalso pointed atgo run .for the sample, butpackage mainlives incmd/; it now saysgo run ./cmd.example_test.gocarries the quickstart asExamplefunctions. With no// Output:comment they are compiled bygo vet/go testbut never run, so they need no runtime and appear on pkg.go.dev. To check that the guard works, reintroducing the originalspice.WithSpiceCloudAddress()into it fails vet:Verification
go build ./...— exit 0go vet ./...— exit 0gofmt -l .— no outputgolangci-lint run ./...(v2.13.2; CI pins v2.12) —0 issues.The first push missed this gate and CI's errcheck flaggeddefer spice.Close()inexample_test.go; fixed in 769f451 with thedefer func() { _ = spice.Close() }()formcmd/main.goalready uses.go test -timeout 25m ./...— 5 failures, all needing a local Spice runtime, the same 5 as on trunk on this machine:TestADBCLocalParameterizedQuery,TestADBCLocalBasicQuery,TestAsyncQueryLocal,TestLocalRuntimeDatasetRefresh,TestLocalRuntime. Every other test passed.go test -run Example -v .—ok(the examples compile; none run, by design)