Propagate error in TFetchResults #255
Conversation
vikrantpuppala
requested review from
yunbodeng-db,
andrefurlan-db,
rcypher-databricks,
jadewang-db and
jackyhu-db
as code owners
vikrantpuppala
force-pushed
the
fix-errors-in-fetch-results
branch
from
c7eefc1 to
3225fc0
Compare
Signed-off-by: Vikrant Puppala <[email protected]>
vikrantpuppala
force-pushed
the
fix-errors-in-fetch-results
branch
from
3225fc0 to
ef393cd
Compare
| @@ -456,10 +455,6 @@ func (r *rows) fetchResultPage() error { | |||
| r.RowScanner = nil | |||
| } | |||
|
|
|||
| if !r.ResultPageIterator.HasNext() { | |||
There was a problem hiding this comment.
can you check the r.error and return r.error instead?
There was a problem hiding this comment.
@jackyhu-db I think it is possible to do a check like that, but I would like to clarify what is the use of HasNext if Next itself can just return nil, io.EOF to signify an end of the stream?
My question is more structural: why the additional complexity of HasNext when Next itself can interface and encapsulate the concept of HasNext? It is possible to now check for the error in r.ResultPageIterator.err and bubble that up, but I'm afraid any divergence between Next's logic and HasNext's logic in the future can lead to similar problems
There was a problem hiding this comment.
I think it is worth considering whether HasNext can be simply rewritten as: r.ResultPageIterator.Next() == (nil, io.EOF), and if so, it is not really useful to keep it, any consumer of ResultPageIterator can use Next for the purpose of checking whether there are any items left or not
There was a problem hiding this comment.
@jackyhu-db, we would also need to update the function signature to return (bool, error) and also make changes to https://github.com/databricks/databricks-sql-go/blob/main/internal/rows/arrowbased/arrowRecordIterator.go#L86 to also return the error, do we want to go down that route?
Prepare for the next release of go driver (1.6.2) which adds the following changes: - Support positional query parameters (#247) - Add custom auth headers into cloud fetch request (#249) - Security: GO-2024-2947 - Update go-retryablehttp (#251) - Security: CVE-2025-27144 - Resolve vulnerability in go-jose (#258) - Bugfix: Handle incorrect EOF in fetchResultPage when TFetchResults call fails with an error (#255) Signed-off-by: Vikrant Puppala <[email protected]>
Currently, an error thrown in TFetchResults within getNextPage in resultPageIterator will be swallowed and an EOF err will be returned instead. This PR solves #254