[peco-engineer-bot]

Summary

Automated fix for #507 — ArrowBatchIterator turns a failed FetchResults into io.EOF.

Added an Err() accessor to the ResultPageIterator interface (and stopped HasNext from masking a stored fetch error with io.EOF) so arrowRecordIterator can distinguish a failed page fetch from a finished result set; the arrow iterator now keeps HasNext()==true while a page-fetch error is pending and returns that error from Next() instead of io.EOF (mirrors the database/sql fix #255). Offline-only per the carve-out: this is a client-side error-propagation contract (analogue of #254/#255), anchored in the issue's stated expectation, and not observable end-to-end since it requires forcing a mid-stream FetchResults failure against the live server.

Root cause & plan

Root cause: arrowRecordIterator swallows a failed inter-page FetchResults as io.EOF. When FetchResults fails between pages, resultPageIterator.HasNext() stores the error (rpf.err), closes, sets isFinished, and returns false. arrowRecordIterator.checkFinished() ANDs !resultPageIterator.HasNext() into its finished decision with NO error check, so it treats the failure as end-of-results and calls Close(). arrowRecordIterator.HasNext() then returns false, so a for it.HasNext() caller loop exits without ever calling Next() and never sees the error (lastErr stays nil); the failed page looks like a complete result set. Compounding it, a later resultPageIterator.HasNext() call overwrites the stored error with io.EOF via if rpf.isFinished && rpf.nextResultPage == nil { rpf.err = io.EOF }. This is the Arrow-batch analogue of #254; the database/sql path was fixed in #255 by having rows.Next return the fetch error, but the arrow path decides termination via HasNext() and collapses error and done into one isFinished flag.
Files: internal/rows/rowscanner/resultPageIterator.go, internal/rows/arrowbased/arrowRecordIterator.go, internal/rows/arrowbased/arrowRecordIterator_test.go
Planned coverage:

• TestArrowRecordIteratorSwallowsPageError (in internal/rows/arrowbased/arrowRecordIterator_test.go, next to TestArrowRecordIterator): using the directResultsMultipleFetch fixtures, direct results are page 1, the mock returns FetchResults1 on the first fetch, then fails the second fetch with 'FetchResults: connection reset'. Drive the iterator with for it.HasNext() { rec, err := it.Next(); if err != nil { lastErr = err; break } ... }. Assert lastErr ErrorContains 'connection reset'. Today the loop ends with lastErr == nil (bug); after the fix Next() returns the page-fetch error instead of io.EOF. (ArrowBatchIterator (arrowRecordIterator) turns a failed inter-page FetchResults into io.EOF instead of surfacing the fetch error)

Files changed

• internal/rows/arrowbased/arrowRecordIterator_test.go
• internal/rows/rowscanner/resultPageIterator.go
• internal/rows/arrowbased/arrowRecordIterator.go

Test plan

• TestArrowRecordIteratorSwallowsPageError — fails (red) against the original code, passes (green) after the fix

🤖 Generated by engineer-bot (bug-fix flow) — review before merge.

[peco-review-bot]

Verdict: 1 Medium

Looks mostly good — the error-propagation fix is correct (Err() vs io.EOF, the errReported guard, and surfacing via Next()), but one medium concern: splitting the short-circuited checkFinished() chain into separate statements makes resultPageIterator.HasNext() run unconditionally, turning lazy next-page fetching into eager prefetching on every Next()/HasNext() call.

[peco-review-bot]

Verdict: 1 Medium

Looks good — the core fix is correct and behavior-preserving for the database/sql path (which calls ResultPageIterator.Next() directly, not HasNext()), and Err() correctly filters io.EOF. One medium concern: the parallel arrowIPCStreamIterator shares the identical fetch-error-as-EOF defect and was left unfixed despite the new interface method being available to fix it.

Other findings

• 🟡 Medium — The sibling arrowIPCStreamIterator has the identical error-swallowing bug that this PR fixes in arrowRecordIterator, and it is left unfixed even though Err() was added to the shared ResultPageIterator interface for exactly this purpose.

When an inter-page FetchResults fails, resultPageIterator.HasNext() stores the error and returns false. Here HasNext() (line 70) then returns false, and Next() (line 39: if !ri.HasNext() { return nil, io.EOF }) converts that failure into io.EOF. A for it.HasNext() { it.Next() } caller over GetArrowIPCStreams() therefore exits cleanly and never observes the fetch error — the same #507 symptom, on the exported IPC-stream path.

Consider wiring the new resultPageIterator.Err() into arrowIPCStreamIterator.HasNext()/Next() (mirroring the ri.err handling added to arrowRecordIterator), or scoping it out explicitly with a note/test if it is intentionally deferred.

[peco-review-bot]

Verdict: 1 Medium · 1 Low

The fix looks right for arrowRecordIterator. I found 1 medium concern: the sibling arrowIPCStreamIterator still turns a failed inter-page fetch into io.EOF. I also found 1 low gap in what the new test checks. Nit: the t.Logf debug line in the new test can be removed or replaced with assertions.

[peco-review-bot]

Verdict: 1 High · 2 Nit

Needs changes. One high-severity regression: arrowRecordIterator.checkFinished no longer stops early once the iterator is finished, so calling HasNext() after Close() can send a FetchResults request to an already-closed operation and report a spurious pending error. The description lists 3 changed files, but the diff also changes arrowIPCStreamIterator.go and category_test.go.

Other findings

• ⚪ Nit — TestArrowIPCStreamIteratorSurfacesPageError is weaker than its sibling record-iterator test. It only checks streams > 0. It does not check that every stream from direct results and from the first fetched page arrives before the error, and it does not assert the FetchResults call count (no assert.Equal(t, 2, calls)). So a regression that drops the first fetched page, or that re-fetches after the error, would still pass. Please mirror the expectedRows/calls assertions from TestArrowRecordIteratorSwallowsPageError.
• ⚪ Nit — Keeping the fetch error (instead of replacing it with io.EOF) also changes the database/sql path. rows.fetchResultPage calls ResultPageIterator.Next() (internal/rows/rows.go:570), so a repeated rows.Next() after a failed page fetch now returns the fetch error again instead of io.EOF. That is arguably more correct, but it is a behavior change outside the arrow-iterator scope described in the PR. It deserves a test in rows_test.go (or at least a mention in the description) so the new contract is intentional and pinned.

[peco-review-bot]

Verdict: 1 Low · 1 Nit

The fix looks correct: page-fetch errors are now stored and returned once from Next() instead of being reported as io.EOF, and adding Err() to the interface is safe because resultPageIterator is the only implementation. Two notes: the PR description lists 3 changed files, but the diff also changes arrowIPCStreamIterator.go and category_test.go. And on the database/sql path, resultPageIterator.Next() now keeps returning the stored error after a failure instead of switching to io.EOF; that is intended, but no rows test covers it.

Other findings

• ⚪ Nit — This new Err() branch in fetchNextData can't really be reached on the normal path. Next() always calls HasNext() first, and when resultPageIterator.HasNext() is false, HasNext() already stores the error in ri.err, which Next() then returns before fetchNextData runs. The only other way here is after errReported, and by then isFinished is already true, so the early return io.EOF at the top fires first. Either delete the branch or add a comment saying it's defensive, so a later reader doesn't think it's the main way errors get reported.

[peco-review-bot]

Verdict: 1 Low

The fix looks correct: a failed inter-page FetchResults now reaches the caller through Next() on both the ArrowBatch and IPC-stream iterators, and resultPageIterator.HasNext() no longer replaces the stored error with io.EOF. There is one low-severity edge case in Schema(), where the same error can be reported twice. Nit: the PR description's file list leaves out arrowIPCStreamIterator.go and category_test.go, even though the diff changes both.

[peco-review-bot]

Verdict: 1 Low

Looks good, with one low-severity concern. The fix correctly keeps the fetch error stored in resultPageIterator and reports it once through both the Arrow record iterator and the IPC stream iterator, and the tests cover the main paths well. Nits: the PR description lists 3 changed files, but the diff also changes arrowIPCStreamIterator.go and category_test.go. errReported in arrowRecordIterator is now redundant because of the new isFinished early return in checkFinished. The name TestArrowRecordIteratorSwallowsPageError describes the bug rather than the behaviour being tested.

[peco-review-bot]

Verdict: 1 Low

The fix looks correct, with one low-severity cleanup finding. I traced both arrow iterators and resultPageIterator. A failed inter-page FetchResults is now reported once through Next(), Close() drops any error that hasn't been reported yet, and the lazy page fetch in checkFinished still only runs once local data is used up. Nit: the PR description lists 3 changed files, but the diff also changes arrowIPCStreamIterator.go and category_test.go. The test name TestArrowRecordIteratorSwallowsPageError describes the bug, not the behavior the test checks; something like ...SurfacesPageError would be clearer.

[peco-review-bot]

Verdict: 2 Nit

Looks good, with no correctness concerns found. The new Err() accessor and keeping HasNext() true until the error is read let both Arrow iterators (record and IPC stream) return a failed inter-page FetchResults from Next() exactly once instead of io.EOF. The tests cover surfacing the error, reporting it only once, Close discarding a pending error, and no fetch after an early close. The database/sql rows.Next path still works because it already treats only non-EOF errors as failures.

Other findings

• ⚪ Nit — In arrowRecordIterator.checkFinished, finished := ri.isFinished || noMoreData can never take the ri.isFinished branch, because the function already returned early when ri.isFinished is true. It could just be if noMoreData { ... }.
• ⚪ Nit — The PR description says three files changed, but the diff also changes arrowIPCStreamIterator.go (the same fix applied to the IPC stream iterator) and rowscanner/category_test.go. Updating the description would help reviewers.

[peco-review-bot]

Verdict: 2 Low

Looks good. I checked the fix for correctness: page-fetch errors now surface exactly once through Next()/Schema() on both the record and IPC-stream iterators. No other code implements ResultPageIterator, so adding Err() doesn't break anything, and the tests cover the error, early-close and pending-error-discard paths. One meta nit: the PR description lists only 3 changed files, but the diff also changes arrowIPCStreamIterator.go and rowscanner/category_test.go. Please update the description so reviewers know the IPC-stream path changed as well.

[peco-review-bot]

Verdict: 1 Low

Looks good, with one low-severity concern. The fix keeps HasNext() true while a page-fetch error is waiting to be returned, and Next()/Schema() then return that error exactly once. It covers both the record and IPC iterators, and the resultPageIterator no longer replaces a stored error with io.EOF; tests cover the error path, early close and close-discards-error cases. Nit: the PR description lists 3 changed files, but the diff also touches arrowIPCStreamIterator.go and category_test.go.

[peco-review-bot]

Verdict: 1 Low

The fix looks correct: when a page fetch fails, the record and IPC-stream iterators now return that error once from Next()/Schema() instead of io.EOF, and an explicit Close() discards it. Tests cover errors after partial delivery, the error being reported once, early close, and Err() returning nil on a normal finish. Nit: the name TestArrowRecordIteratorSwallowsPageError describes the old bug rather than what the test checks; a name like ...SurfacesPageError would match its IPC sibling.

[peco-review-bot]

Verdict: 1 Low

The fix looks correct. Arrow record and IPC iterators now return the stored page-fetch error once and then report finished, checkFinished no longer fetches after Close, and the tests cover the fetch error, Close and Schema() cases. I found one low-severity issue (an unneeded new exported rows.Err() method). Nit: the PR description lists 3 changed files, but the diff also changes arrowIPCStreamIterator.go, rows.go, and rows_test.go/category_test.go. Also, the test name TestArrowRecordIteratorSwallowsPageError describes the bug rather than the behavior the test checks.

[peco-review-bot]

Verdict: 1 Low

Looks good. The fix correctly separates a failed inter-page FetchResults from a normal end of results in both Arrow iterators and in resultPageIterator, and the new tests cover the main path, reporting the error only once, and Close-before-read. The one low finding is defensive hardening. Nits: the PR description lists 3 files, but the diff also changes arrowIPCStreamIterator.go and category_test.go. TestArrowRecordIteratorSwallowsPageError is named after the bug rather than the behaviour it now checks (e.g. ...SurfacesPageError). And rows.Next() now keeps returning the stored fetch error on repeated calls instead of io.EOF, which is a reasonable behaviour change but not mentioned in the PR description.

[peco-review-bot]

Verdict: 1 Low

Looks good: the fix correctly stops a failed page fetch from looking like a normal end-of-results in both arrowRecordIterator and arrowIPCStreamIterator, and the tests cover the error being reported once, Close, and Schema. One low concern: the change also alters database/sql rows.Next behavior, and no test covers that. Nit: the PR's "Files changed" list leaves out arrowIPCStreamIterator.go and category_test.go, which the diff does change.

[peco-review-bot]

Verdict: 2 Nit

Looks good: I found no correctness issues. When an inter-page FetchResults fails, resultPageIterator now keeps the error, Err() exposes it, and both the record iterator and the IPC-stream iterator report it exactly once from Next() instead of returning io.EOF. An explicit Close() discards the pending error, and the tests cover the success, error, Schema-path and early-Close orderings.

Other findings

• ⚪ Nit — The PR description lists 3 changed files and one test. The diff actually changes 6 files: it also applies the same fix to arrowIPCStreamIterator.go and adds tests in rows_test.go and category_test.go. Updating the description would help reviewers see the full scope, including the IPC-stream behavior change and the stricter error returned by rows.Next / resultPageIterator.Next.
• ⚪ Nit — TestArrowRecordIteratorSwallowsPageError is named after the bug rather than the expected behavior. The sibling test is named TestArrowIPCStreamIteratorSurfacesPageError. Consider TestArrowRecordIteratorSurfacesPageError for consistency. Also, the IPC test checks the result with assert.ErrorIs(errAfter, io.EOF) while the other tests use assert.Equal(io.EOF, …). Minor, but worth making consistent.