refactor(any): Implement fetch_optional in terms of fetch_many

[iamjpotts]

• De-duplicates implementations of AnyConnectionBackend::fetch_optional to be in terms of fetch_many
• Standardizes all fetch_optional implementations to loop over the fetch_many result stream until the first row is found (previously, some implementations could return None if a row was present in the stream, but a statistics entry AnyQueryResult was returned first).

New commit with this pr is the last one, refactor(any): Implement fetch_optional in terms of fetch_many

Builds on:
#3960

Does your PR solve an issue?

No

Is this a breaking change?

No, but it does add a default implementation for AnyConnectionBackend::fetch_optional which only the sqlite implementation overrides.

[bobozaur]

Wouldn't fetch be a more correct choice than fetch_many? Asking because multi statement queries (which you'd use with fetch_many) cannot be prepared and drivers tend to implement fetch_optional on top of fetch. The fact that fetch is implemented on top of fetch_many is in my view an implementation detail. sqlx-exasol, for instance, does not/can not do that: https://github.com/bobozaur/sqlx-exasol/blob/3c874343e3e75110f66a4260a74b1e0425120bf4/src/connection/executor.rs#L69

[iamjpotts]

Wouldn't fetch be a more correct choice than fetch_many? Asking because multi statement queries (which you'd use with fetch_many) cannot be prepared and drivers tend to implement fetch_optional on top of fetch. The fact that fetch is implemented on top of fetch_many is in my view an implementation detail. sqlx-exasol, for instance, does not/can not do that: https://github.com/bobozaur/sqlx-exasol/blob/3c874343e3e75110f66a4260a74b1e0425120bf4/src/connection/executor.rs#L69

Thanks for bringing this up.

fetch_optional can still have a database platform specific implementation if this change were merged; any existing implementations of AnyConnectionBackend::fetch_optional should be unaffected. This change is intended to DRY / de-duplicate some of the existing implementations.

Though not a part of this PR, I am exploring an implementation of named parameters and this being de-duplicated helps with it (as well as simplifying the existing code).

[bobozaur]

You're absolutely right, my bad. My main fear was that this would alter existent behavior but, given that the method did not have a default impl, it won't. Also silly me because AnyConnectionBackend doesn't even expose a fetch method. Perhaps it should?

Just stumbled upon this issue as I was searching for something else and wanted to give my 2 cents. The whole reason I'm interested in this is because there seems not to be a clear separation between fetch/execute and fetch_many/execute_many in the sense that the former is meant to be used with single statement queries which can be prepared and can have parameters bound to them where as the latter is meant to be used with multi-statement queries and cannot be prepared and thus cannot have parameters bound to them.

I was even considering going to the extent of erroring out early in sqlx-exasol if a query passed to fetch_many/execute_many has arguments or if query.persistent() == true because the database would reject it anyway. But I'm second guessing my understanding now 😅 .

Anyway, just to wrap up my brainstorming, while AnyConnectionBackend does not need a separate fetch method, I think adding it would help better separate things. And then the default fetch_optional impl could be on top of fetch.

[iamjpotts]

You're absolutely right, my bad.

Please feel free to add more comments even if you're not sure. I have a few more prs that you may be interested in a heads up on since you maintain an implementation of another db platform:

#3957
#3958
#3960

While I am not a maintainer, an extra set of eyes and comments may be helpful to the maintainer if/when he gets around to looking at these.