Skip to content

rows/rowscanner: set isFinished=true when last page has no more rows#359

Open
Dev-X25874 wants to merge 1 commit into
databricks:mainfrom
Dev-X25874:fix/result-page-iterator-last-page-finished
Open

rows/rowscanner: set isFinished=true when last page has no more rows#359
Dev-X25874 wants to merge 1 commit into
databricks:mainfrom
Dev-X25874:fix/result-page-iterator-last-page-finished

Conversation

@Dev-X25874
Copy link
Copy Markdown

Problem

In resultPageIterator.HasNext(), when a fetched page has HasMoreRows == false,
the code calls rpf.Close() to close the operation handle on the server but never
sets rpf.isFinished = true.

After Next() consumes that last page and clears nextResultPage, any subsequent
call to HasNext() bypasses the early-exit guard at the top of the function
(which checks isFinished && nextResultPage == nil) and falls through to
getNextPage(). Since isFinished is still false, getNextPage() proceeds to
issue a FetchResults RPC against an already-closed operation handle, resulting
in a spurious server-side error instead of a clean io.EOF.

Fix

Set rpf.isFinished = true alongside rpf.Close() in the no-more-rows branch,
mirroring what the error path in the same function already does correctly:

// before
if !nrp.GetHasMoreRows() {
    rpf.Close()
}

// after
if !nrp.GetHasMoreRows() {
    rpf.isFinished = true
    rpf.Close()
}

Impact

Any caller that invokes HasNext() more than once after the last page is consumed
— a reasonable and common pattern — would previously receive an unexpected RPC
error. After this fix they receive false / io.EOF as intended.

@vikrantpuppala
Copy link
Copy Markdown
Collaborator

This change is functionally a no-op — rpf.isFinished is already true by the time we reach line 137.

Tracing HasNextgetNextPage: the fetch loop at line 177 always runs at least once (the loop entry condition !Contains(Start()+Count()) is always true because Contains uses an inclusive [start, end] range and Start()+Count() == end+1), and lines 202–205 unconditionally write rpf.isFinished from HasMoreRows. So when control returns to HasNext and we enter the !nrp.GetHasMoreRows() branch, isFinished is already true.

The early-exit guard at line 117 (if rpf.isFinished && rpf.nextResultPage == nil) therefore already short-circuits any subsequent HasNext() after the final page is consumed — no spurious FetchResults RPC, no server error. I can't construct a path on main where the described bug actually fires; do you have a concrete repro (stack trace, server log showing FetchResults after CloseOperation, or a failing test)?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants