[sqlite] Review Result
First of all, I would like to thank everyone who participated in the review process, and Klemens for submitting the library. In total, we received 9 reviews: 3 ACCEPT, 3 CONDITIONALLY ACCEPT, and 3 REJECT. The individual reviews are as follows: Barend Gehrels: ACCEPT [1] Ruben Perez: REJECT [2] Scott Bailey: ACCEPT [3] Mungo Gill: CONDITIONALLY ACCEPT [4] Amlal El Mahrouss: ACCEPT [5] Harold Bott: CONDITIONALLY ACCEPT [6] Darryl Green: CONDITIONALLY ACCEPT [7] Andrzej Krzemienski: REJECT [8] Maximilian Riemensberger: REJECT [9] After carefully examining all reviews, going through the mailing list discussions, and consulting with Ruben Perez (author of Boost.MySQL) and Klemens, I have decided to REJECT the library due to critical flaws in the API and insufficient documentation. I strongly encourage the author to make another submission after addressing the issues listed below: - connection ownership ambiguity: A connection object can currently be constructed in two forms, owning and non-owning, which makes reasoning about it difficult. It was suggested on the mailing list to introduce a connection_ref type, which not only resolves this problem but also makes it easier for a user to wrap an existing sqlite3* (acquired from another library or C API) and benefit from the type safety provided by this library. - resultset ownership ambiguity: A resultset can either own an sqlite3_stmt or act as a non-owning reference to an existing sqlite3_stmt, providing only an access interface. This dual behavior makes reasoning about ownership difficult and allows multiple resultset objects to reference the same sqlite3_stmt, giving the false impression that they exist independently of the main statement object. Further exploration is needed to determine the right API. One suggestion from the mailing list is to remove the resultset type entirely and merge its functionality into the statement type, with a non-owning view/range type that can be used for iterating without having to pass the statement object around. - Wrong usage of cstring_ref (minor): As noted in the reviews, field::get_text could return a core::string_view instead of a cstring_ref, since SQLite already provides an API for determining the length of a column. - CI and test coverage: Currently, the CI supports only a limited set of platforms. It should, at a minimum, build and test the project on Linux, Windows, and macOS using Clang, GCC, and MSVC, with a matrix of configurations (RTTI on/off, exceptions on/off, shared/static). It should also include sanitizer builds and coverage reports. I would recommend that the author use the coverage report results to improve the tests, as the current tests do not appear to cover all interfaces. - Build scripts: The Jamfile should allow using libsqlite3 even when it is not installed system-wide and exists only in package manager paths, such as vcpkg. The necessary Jamfile for locating libsqlite3 should likely be added to the Boost.Build repository. - Examples: The existing examples cover advanced usage, such as SQLite virtual tables, but lack clear and simple use cases. I would recommend that the author focus on the most frequently used APIs and provide examples that quickly convey the knowledge needed to use the library for simple tasks. - Documentation: Currently, there is very little documentation around important types such as connection, statement, resultset, row, field, and value. I would recommend that the author provide proper documentation and usage examples for each type, and place extra emphasis on the quick-start section, as it is likely the part most users will read. - Real-world users: Considering the nature of this library, I would recommend that the author seek feedback from real-world users before submitting it again, as many design flaws in the libraries of this kind only become apparent through practical use. Without such feedback, it is difficult to anticipate usage patterns and design interfaces accordingly. Additionally, receiving reviews from users who have successfully used the library in their applications provides extra confidence to the release manager regarding the library's future success. [1] https://lists.boost.org/archives/list/boost@lists.boost.org/message/MGOXOV6S... [2] https://lists.boost.org/archives/list/boost@lists.boost.org/message/57ZZ7IIW... [3] https://lists.boost.org/archives/list/boost@lists.boost.org/message/BD3OQOWO... [4] https://lists.boost.org/archives/list/boost@lists.boost.org/message/O477WE5S... [5] https://lists.boost.org/archives/list/boost@lists.boost.org/message/BUL75BOX... [6] https://lists.boost.org/archives/list/boost@lists.boost.org/message/CS3FT3E7... [7] https://lists.boost.org/archives/list/boost@lists.boost.org/message/VKZMFA65... [8] https://lists.boost.org/archives/list/boost@lists.boost.org/message/DIP5GYYP... [9] https://lists.boost.org/archives/list/boost@lists.boost.org/message/TVXSK4DP... Best regards, Mohammad Nejati Review Manager for the proposed Boost.SQLite
First of all, I would like to thank everyone who participated in the review process, and Klemens for submitting the library.
In total, we received 9 reviews: 3 ACCEPT, 3 CONDITIONALLY ACCEPT, and 3 REJECT.
The individual reviews are as follows:
Barend Gehrels: ACCEPT [1] Ruben Perez: REJECT [2] Scott Bailey: ACCEPT [3] Mungo Gill: CONDITIONALLY ACCEPT [4] Amlal El Mahrouss: ACCEPT [5] Harold Bott: CONDITIONALLY ACCEPT [6] Darryl Green: CONDITIONALLY ACCEPT [7] Andrzej Krzemienski: REJECT [8] Maximilian Riemensberger: REJECT [9]
After carefully examining all reviews, going through the mailing list discussions, and consulting with Ruben Perez (author of Boost.MySQL) and Klemens, I have decided to REJECT the library due to critical flaws in the API and insufficient documentation. I strongly encourage the author to make another submission after addressing the issues listed below:
Best regards, Mohammad Nejati Review Manager for the proposed Boost.SQLite _______________________________________________
Thank you to: Klemens for your submission, Mohammad for managing the review, and all those that participated. Matt
On Fri, Sep 12, 2025 at 2:43 AM Mohammad Nejati via Boost < boost@lists.boost.org> wrote:
First of all, I would like to thank everyone who participated in the review process
Very nicely written summary, thank you. - connection ownership ambiguity:
- resultset ownership ambiguity:
I'd be curious to see how other libraries solve this problem, if at all. Thanks
On Fri, Sep 12, 2025 at 3:57 PM Vinnie Falco <vinnie.falco@gmail.com> wrote:
- connection ownership ambiguity: - resultset ownership ambiguity:
I'd be curious to see how other libraries solve this problem, if at all.
Klemens could probably answer this question better, but from my research, sqlite_orm (a C++ library) has a connection_ref (which seems to be used only internally), while the others libs have only a single owning connection type. rusqlite, (a Rust library) uses the same owning/non-owning model as the proposed Boost.Sqlite, but connection::from_handle is marked unsafe, even though I do not think it helps the user reason about ownership. Regarding the resultset issue, the functionality is usually implemented in the statement class because there is no standalone resultset object in SQLite and it always needs to refer to a statement.
On Fri, Sep 12, 2025 at 4:54 PM Mohammad Nejati via Boost <boost@lists.boost.org> wrote:
On Fri, Sep 12, 2025 at 3:57 PM Vinnie Falco <vinnie.falco@gmail.com> wrote: Regarding the resultset issue, the functionality is usually implemented in the statement class because there is no standalone resultset object in SQLite and it always needs to refer to a statement.
Yes, no resultset, only a statement. But the same applies to row, used in the proposed range-for loop. There's also no row in SQLite. And that pseudo row is also just a statement in disguise, the same very one that was in resultset. So how is that any different? I'm still confused by that aspect. binding happens on the statement. _step(), i.e. "iterating" happens on the statement, to go to the next row. getting the values ("columns") for that current row happens on the statement. The pattern for a query is: auto stmt = conn.prepare("some SQL..."); stmt.bind(1, some_value); // binds are 1-based while (stmt.step() == SQLITE_ROW) { auto v1 = stmt.get_text(0); // gets are 0-based double v2; stmt.get(1, v2); // overloads that support generic code ... // use that row for something } The above assume RAII on statement, and a move-friend stmt type. But my point is that there's no resultset, no row, no field, nothing. statement is it, nothing else. This is idiomatic SQLite, barely above the official C API equivalent. So what do you do for a nice range-for API? The LHS of a range-for is supposed to be a value. What's the value here? In the native SQLite model, it's the statement itself! Lifetime confision again. If you force extracting the values (for the current row) into a tuple (or struct, via PFR or Describe magic), then you do have a real value, and the semantic is clear IMHO. That forces you to say on the RHS of the range-for what the current row (hidding inside the step'd statement...) should be extracted and converted to. If IMHO, it's some kind of generic sqlite::row, that's internally just a reference to the "outer" statement we're iterating, then the semantic are confusing, just like in the resultset's case IMHO. This is the tension above that must be resolved IMHO. BTW, Klemens, the pseudo code above is the low-level wrapper API I was talking about previously. --DD
On Fri, Sep 12, 2025 at 6:48 PM Dominique Devienne <ddevienne@gmail.com> wrote:
But the same applies to row, used in the proposed range-for loop. There's also no row in SQLite. And that pseudo row is also just a statement in disguise, the same very one that was in resultset. So how is that any different? I'm still confused by that aspect.
There have been no objections to having either a single view type or two distinct types for owning and non_owning ranges. The concern is more about a type that could act as both owning and non_owning at the same time. For example the following could provide that same functionality without the mentioned ownership issue: auto stmt = conn.prepare("select ..."); for(const order& ord : range<order>(stmt))
pt., 12 wrz 2025 o 17:21 Dominique Devienne via Boost <boost@lists.boost.org> napisał(a):
On Fri, Sep 12, 2025 at 4:54 PM Mohammad Nejati via Boost <boost@lists.boost.org> wrote:
On Fri, Sep 12, 2025 at 3:57 PM Vinnie Falco <vinnie.falco@gmail.com> wrote: Regarding the resultset issue, the functionality is usually implemented in the statement class because there is no standalone resultset object in SQLite and it always needs to refer to a statement.
Yes, no resultset, only a statement. But the same applies to row, used in the proposed range-for loop. There's also no row in SQLite. And that pseudo row is also just a statement in disguise, the same very one that was in resultset. So how is that any different? I'm still confused by that aspect.
binding happens on the statement. _step(), i.e. "iterating" happens on the statement, to go to the next row. getting the values ("columns") for that current row happens on the statement.
The pattern for a query is:
auto stmt = conn.prepare("some SQL..."); stmt.bind(1, some_value); // binds are 1-based while (stmt.step() == SQLITE_ROW) { auto v1 = stmt.get_text(0); // gets are 0-based double v2; stmt.get(1, v2); // overloads that support generic code ... // use that row for something }
The above assume RAII on statement, and a move-friend stmt type. But my point is that there's no resultset, no row, no field, nothing. statement is it, nothing else. This is idiomatic SQLite, barely above the official C API equivalent.
So what do you do for a nice range-for API? The LHS of a range-for is supposed to be a value. What's the value here? In the native SQLite model, it's the statement itself! Lifetime confision again.
If you force extracting the values (for the current row) into a tuple (or struct, via PFR or Describe magic), then you do have a real value, and the semantic is clear IMHO. That forces you to say on the RHS of the range-for what the current row (hidding inside the step'd statement...) should be extracted and converted to.
If IMHO, it's some kind of generic sqlite::row, that's internally just a reference to the "outer" statement we're iterating, then the semantic are confusing, just like in the resultset's case IMHO.
This is the tension above that must be resolved IMHO.
BTW, Klemens, the pseudo code above is the low-level wrapper API I was talking about previously. --DD
Having a deeper look at SQLite, I now realize that you actually cannot work with two statements on one connection concurrently (this is not related to multi-threading): ``` auto query1 = conn.prepare("select * FROM table1"); auto query2 = conn.prepare("select * FROM table2"); auto combined = std::views::zip(statement_range<T1>(query1), statement_range<T2>(query2)); for (auto record: combined) // ERROR: two interleaving statements process(record); ``` I would intuitively expect that a Boost-class C++ library would prevent me from easily making this mistake, or at least inform me about this gotcha. Maybe I am fantasizing, but I would imagine an interface that doesn't allow me to create the second statement on a connection until I am done with the previous one: ``` conn.execute_statement( "select * FROM table1", [&](int col1, int col2, std::string_view col3) { // process a table row } ); ``` or alternatively ``` conn.with_statement( "select * FROM table1", [&](boost::sqlite::statement& st) { // do what you will with `st` // but its lifetime is managed within the call to with_statement() } ); ``` Regards, &rzej;
sob., 13 wrz 2025 o 10:28 Klemens Morgenstern via Boost < boost@lists.boost.org> napisał(a):
Having a deeper look at SQLite, I now realize that you actually cannot
work
with two statements on one connection concurrently (this is not related to multi-threading):
That would be news to me. Can you be a bit more specific about what you mean by "cannot"?
Maybe I should have made that a question rather than a statement. Would the following concurrent use of two statements work without any resource-management issues? ``` auto query1 = conn.prepare("select * FROM table1"); auto query2 = conn.prepare("select * FROM table2"); auto combined = std::views::zip(statement_range<T1>(query1), statement_range<T2>(query2)); for (auto record: combined) process(record); ``` ("Concurrent" in the sense that I progress over the second statement without having finished with the first one.) Regards, &rzej;
On Sat, Sep 13, 2025, 7:19 PM Andrzej Krzemienski <akrzemi1@gmail.com> wrote:
sob., 13 wrz 2025 o 10:28 Klemens Morgenstern via Boost < boost@lists.boost.org> napisał(a):
Having a deeper look at SQLite, I now realize that you actually cannot
work
with two statements on one connection concurrently (this is not related to multi-threading):
That would be news to me. Can you be a bit more specific about what you mean by "cannot"?
Maybe I should have made that a question rather than a statement. Would the following concurrent use of two statements work without any resource-management issues?
``` auto query1 = conn.prepare("select * FROM table1"); auto query2 = conn.prepare("select * FROM table2");
auto combined = std::views::zip(statement_range<T1>(query1),
statement_range<T2>(query2));
for (auto record: combined) process(record); ``` ("Concurrent" in the sense that I progress over the second statement without having finished with the first one.)
Yes.
Regards, &rzej;
sob., 13 wrz 2025 o 17:44 Klemens Morgenstern < klemensdavidmorgenstern@gmail.com> napisał(a):
On Sat, Sep 13, 2025, 7:19 PM Andrzej Krzemienski <akrzemi1@gmail.com> wrote:
sob., 13 wrz 2025 o 10:28 Klemens Morgenstern via Boost < boost@lists.boost.org> napisał(a):
Having a deeper look at SQLite, I now realize that you actually cannot
work
with two statements on one connection concurrently (this is not related to multi-threading):
That would be news to me. Can you be a bit more specific about what you mean by "cannot"?
Maybe I should have made that a question rather than a statement. Would the following concurrent use of two statements work without any resource-management issues?
``` auto query1 = conn.prepare("select * FROM table1"); auto query2 = conn.prepare("select * FROM table2");
auto combined = std::views::zip(statement_range<T1>(query1),
statement_range<T2>(query2));
for (auto record: combined) process(record); ``` ("Concurrent" in the sense that I progress over the second statement without having finished with the first one.)
Yes.
Thank you. This was my mistake, of using Copilot as an assistant in investigations. So, looking at this page from SQLite manual -- https://www.sqlite.org/c3ref/stmt.html -- a statement (or stmt or prepared statement) represents the "life cycle" of a compiled procedure: creation/preparation, binding parameters, performing steps, resetting, finalizing. I have some reflections that I can't help sharing. 1. Maybe you do not need the name "prepare". I think that it is implied by the nature of a statement that the only way to get a non-trivial instance of one is to do the preparation by passing the string command and connection. So maybe: sqlite::statement stmt = conn.statement("select * from CLIENTS"); 2. It looks to me that even at this point I could decide whether I will be observing rows from a table, and what types these rows will have: sqlite::statement<tuple<string, int>> stmt = conn.statement<tuple<string, int>>("select NAME, AGE from CLIENTS;"); Am I right about this? I think that no "reset" can change the row type of a once compiled statement. 3. In this scheme where sqlite::statement is parametrized, we would have `statement<void>` for just instructions that do not have any rows and `statement<aggregate>` for reading "rows". In this case there is no harm in providing the range interface directly in `statement<aggregate>`. Is there ever a need to delay or change the decision on how we want to view the rows of a once compiled statement? Regards, &rzej;
On Sat, Sep 13, 2025 at 2:51 PM Andrzej Krzemienski via Boost < boost@lists.boost.org> wrote:
1. Maybe you do not need the name "prepare".
"Prepare" is a term of art: https://en.wikipedia.org/wiki/Prepared_statement. Maybe it is not applicable here? Thanks
On Sun, 14 Sep 2025 at 00:04, Vinnie Falco via Boost <boost@lists.boost.org> wrote:
On Sat, Sep 13, 2025 at 2:51 PM Andrzej Krzemienski via Boost < boost@lists.boost.org> wrote:
1. Maybe you do not need the name "prepare".
"Prepare" is a term of art: https://en.wikipedia.org/wiki/Prepared_statement. Maybe it is not applicable here?
The official API uses the term "prepare" so I think it's the correct name: https://www.sqlite.org/c3ref/prepare.html
Thanks _______________________________________________ Boost mailing list -- boost@lists.boost.org To unsubscribe send an email to boost-leave@lists.boost.org https://lists.boost.org/mailman3/lists/boost.lists.boost.org/ Archived at: https://lists.boost.org/archives/list/boost@lists.boost.org/message/BVU6RQG3...
participants (7)
-
Andrzej Krzemienski -
Dominique Devienne -
Klemens Morgenstern -
Matt Borland -
Mohammad Nejati -
Ruben Perez -
Vinnie Falco