Handle runtime error on Base.iterate #353
Conversation
src/dbinterface.jl
Outdated
| if status == API.SQL_ERROR || status == API.SQL_INVALID_HANDLE | ||
| error(API.diagnostics(x.stmt)) | ||
| end | ||
| status == API.SQL_STILL_EXECUTING || break | ||
| end | ||
| status == API.SQL_SUCCESS || status == API.SQL_SUCCESS_WITH_INFO || return nothing |
There was a problem hiding this comment.
| status == API.SQL_SUCCESS || status == API.SQL_SUCCESS_WITH_INFO || return nothing |
Hmmm, I think we can remove this now? Since we're already erroring if it's an error?
There was a problem hiding this comment.
I think we can't, because it gets triggered after the last row is fetched. We need it to return nothing as requested by Base.iterate API here
There was a problem hiding this comment.
that code is SQL_NO_DATA; I've updated the PR to take recognize its existence implicitly 😄
|
Hmmm, I wonder if I messed up the logic there since the tests are now failing consistently. |
I had a bug with the scoping of |
src/dbinterface.jl
Outdated
| status == API.SQL_STILL_EXECUTING && continue | ||
| status == API.SQL_SUCCESS && break | ||
| status == API.SQL_NO_DATA && return nothing | ||
| status == API.SQL_NTS && return nothing |
There was a problem hiding this comment.
This is based on the previous behaviour, which may be hiding a small bug. Not sure what we need to do if we get this (and I haven't stumbled upon it)
pankgeorg
force-pushed
the
pg/checkerrors-on-iterate
branch
2 times, most recently
from
425db32 to
56e94de
Compare
Codecov Report
@@ Coverage Diff @@
## main #353 +/- ##
==========================================
+ Coverage 75.00% 76.86% +1.86%
==========================================
Files 6 6
Lines 856 860 +4
==========================================
+ Hits 642 661 +19
+ Misses 214 199 -15
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
pankgeorg
force-pushed
the
pg/checkerrors-on-iterate
branch
from
56e94de to
d710aaf
Compare
API.SQLFetchScroll(API.getptr(x.stmt), API.SQL_FETCH_NEXT, 0)may return an error after only some of the results have been loaded. In the current implementation the error is ignored and iterate just stops the iteration, returning all the rows up to this point.With this edit, the iteration will either warn the user or throw an error, consistent with the behaviour of
@checkresult.Should fix #352
In a weird sense this is technically a breaking change - as queries that returned early with a subset of results will now raise an error. In the grand scheme of things though I think tihs is a good change.