Open piodul opened 1 year ago
By the way, the same logic also applies to the names of the Session
methods - query()
and execute()
. When I started using the Rust driver (yesterday ;-)) it wasn't immediately obvious why the unprepared version of execute()
is called "query" and not execute_unprepared
, or something like it. It's not a huge problem (I just read the whole Session
documentation and found everything) but it was strange.
Sadly, Rust doesn't seem to have function overloading, so it's not possible to allow the same name execute()
for both prepared and unprepared versions, as done in Python for example, but at least the names could be closer, and perhaps also physically closer in the sorted documentation.
By the way, the same logic also applies to the names of the
Session
methods -query()
andexecute()
. When I started using the Rust driver (yesterday ;-)) it wasn't immediately obvious why the unprepared version ofexecute()
is called "query" and notexecute_unprepared
, or something like it. It's not a huge problem (I just read the wholeSession
documentation and found everything) but it was strange.Sadly, Rust doesn't seem to have function overloading, so it's not possible to allow the same name
execute()
for both prepared and unprepared versions, as done in Python for example, but at least the names could be closer, and perhaps also physically closer in the sorted documentation.
Function overloading can be simulated to some extent via traits. For example, PreparedStatement
and UnpreparedStatement
could implement a trait (let's say it's called Executable
), and then you could have:
pub async fn execute(&self, stmt: &impl Executable) {
// ...
}
Maybe this could be extended to batches as well.
Function overloading can be simulated to some extent via traits. For example, PreparedStatement and UnpreparedStatement could implement a trait (let's say it's called Executable), and then you could have.
I'm not a big fan of stuff like this. I feel like it's going to devolve into an unreadable web of traits, Executables
, Executors
etc. Kind of like what has happened in cdrs
:
A separate function for each type of query is fine, it's easy to read and easy to use, even if we have to use a bunch of different names. KISS.
Query
is a bit weird after serialization refactor. Due to the need to know column types in order to serialize values, we can't just send Query
with values, so the driver internally prepares it - but only if values passed are non-empty.
Hiding this preparation step may be surprising, even though there are warnings in the documentation about it. I think that preparing should be explicit - for implicit preparing there is CachingSession
.
It is a problematic footgun for batches, where each query needs to be prepared.
There is also potential use case of BoundStatement
(https://github.com/scylladb/scylla-rust-driver/issues/941).
Batch interface is also a bit weird. Even right now it requires empty elements in values iterator on the positions corresponding to Query
, it would be the same with BoundStatement
.
It also always seemed to me to be a bit clunky - Rust is all about type-level safety and making illegal states irrepresentable, but here we have 2 vectors, one for pairs and one for values,
instead of one vector of statements.
I see 2 main paths forward., correct me if I missed any.
If we decide to do so, remove values
arguments from query
, query_paged
, query_iter
.
For BoundStatement
introduce another set of functions - like execute_bound
, execute_bound_paged
, execute_bound_iter
.
We would also need to add new variant to BatchStatement
enum so that BoundStatements
can be used there.
Advantages:
Disadvantages:
Query
/ BoundStatement
Create some trait, let's call it Executable
as @piodul proposed. execute
/ execute_paged
/ execute_iter
would accept impl Executable
instead of statement + values.
batch
would accept iterator of Executable.
For PreparedStatement
executable would be implemented for (PreparedStatement, impl SerializeRow)
to allow passing values.
BoundStatement
would just implement Executable
itself.
Query
would do either, depends on wether we decide to allow it to take values.
Batch
would also implement Executable
which would eliminate need for a separate method. There would be just execute
/ execute_paged
/ execute_iter
.
Advantages:
Session
interface, without separate method set for each query type and batch.Session
/ Connection
to trait implementations.Disadvantages:
Executable
trait look right now), more API-breaking changessession.execute((statement, values))
).I like Executable
because of simplification and improved batch interface, but I'm not yet fully convinced it's the best way to go.
I also have no idea what methods should this trait have, I'd have to think about it.
WDYT @piodul ?
I like the idea of Executable
trait, too. Further analysis is needed to design an API of such trait and then probably some try-on implementation.
Confusingly, the driver calls unprepared statement a
Query
(contrary toPreparedStatement
). It's not a correct term and can lead to confusion. "Queries" are statements that ask for data and receive some data in return. "Statement" is a broader term and encompasses items that don't return data but rather modify data or schema, e.g.INSERT
orCREATE
. TheQuery
object has no problems with statements that are not queries.I suggest to rename
Query
to something that fits its purpose better, e.g.UnpreparedStatement
orSimpleStatement
- need to decide upon something, it's probably the best to look at the naming in other drivers. To ease transition to the new name, we will re-introduceQuery
as a type alias, then deprecate and eventually remove at some point.The documentation will have to be adjusted as well as it seems to use "query" and "statement" interchangeably, which is wrong due to the reasons described above but was probably caused by confusion resulting from the existing
Query
name.