Dear All, Surprise! The Boost formal review of the Boost SQLITE library starts *TODAY*, taking place from November 13th, 2024 to November 22nd, 2024 (inclusive). I apologise profusely for springing this on you without prior warning. The error is entirely mine. I am extending the period by one day to compensate. The library is authored by Klemens Morgenstern (@klemens-morgenstern in the CppLang slack). Documentation: https://klemens.dev/sqlite/ <https://anarthal.github.io/mysql/index.html> Source: https://github.com/klemens-morgenstern/sqlite <https://github.com/anarthal/mysql/>
From the documentation:
boost.sqlite is a simple to use C++ sqlite library. It provides a modern interface using facilities like error_code, views (e.g. for blobs) and the ability to use boost.describe or boost.pfr for parameterised queries. Supported features include: - typed queries - prepared statements - json support - custom functions (scalar, aggregate, windows) - event hooks - virtual tables SQLite provides an excellent C-API, so this library does not attempt to hide, but to augment it. Please provide in your review information you think is valuable to explain your choice to ACCEPT or REJECT including SQLITE as a Boost library. Please be explicit about your decision (ACCEPT or REJECT). Some other questions you might want to consider answering: - Will the library bring additional out-of-the-box utility to Boost? - What is your evaluation of the implementation? - What is your evaluation of the documentation? - Will the choice of API abstraction model ease the development of software that must talk to a SQLITE database? - Are there any immediate improvements that could be made after acceptance, if acceptance should happen? - Did you try to use the library? With which compiler(s)? Did you have any problems? - How much effort did you put into your evaluation? A glance? A quick reading? In-depth study? - Are you knowledgeable about the problem domain? More information about the Boost Formal Review Process can be found at: http://www.boost.org/community/reviews.html The review is open to anyone who is prepared to put in the work of evaluating and reviewing the library. Prior experience in contributing to Boost reviews is not a requirement. Thank you for your efforts in the Boost community. They are very much appreciated. Richard Hodges - review manager of the proposed Boost.SQLITE library Klemens is often available on CppLang Slack and of course by email should you require any clarification not covered by the documentation, as am I.
https://klemens.dev/sqlite/ <https://anarthal.github.io/mysql/index.html> Source: https://github.com/klemens-morgenstern/sqlite <https://github.com/anarthal/mysql/>
I think some copy pasting went bad here. These were the links for Boost.MySQL before it became Boost.MySQL. Thanks, Ruben.
On Wed, Nov 13, 2024 at 4:31 AM Richard Hodges via Boost < boost@lists.boost.org> wrote:
The library is authored by Klemens Morgenstern (@klemens-morgenstern in the CppLang slack).
This review is already off to a poor start. A valid complaint from the last review is that discussion took place off-list, on Slack. I suggested that future announcements might remind reviewers to have discussions here instead of elsewhere. I see no such reminder here, and instead I see you are making it easier to contact the library submitter on Slack. At least you didn't also provide the link to the workspace invitation page (which is https://cpp.al/slack) Here are the things being said in the #boost channel regarding this library submission: "404 not found in review email link https://anarthal.github.io/mysql/index.html" "is the reference doc supposed to be complete? E.g. https://klemens.dev/sqlite/structboost_1_1sqlite_1_1connection.html is just mostly empty, except for decorations. For what is worth, I wanted to know how to open the DB read-only, or using URIs, and was thus looking at the doc, and I find none." "these docs are so complete one gets tired of completeness when reading them" "Should these questions be asked on the list instead of here?" "dunno. I'm asking a simple question so far, on expectations I should have" "Expect what you would expect from a complete Boost library." "I was expecting at least the signatures of functions (Ctors in this instance), at least. thus I'm suspecting a ref doc (doxygen based?) generation issue, and was inquiring" "Fwiw, I don't think it's inappropriate to ask about the docs like that here because yeah, that seems kind of just like a simple configuration error I'm guessing" "these are good things to ask on the list and you should state your documentation expectations on the list" "I intend to embed the sources in my code base, instead of compiling Boost. Will Boost.SQLite be compatible with my existing Boost 1.84, for its dependencies (if any)?" "darn, all the classes docs aren't there." "explicit connection(handle_type handle, bool take_ownership = true) that reads inscrutably at the call site. I've taken the habit of using e.g. enum class HasOwnership : bool; inline constexpr HasOwnership cTakeOwnership{ true }; ... so the call site reads well. Bad idea? Is that something you'd consider?" "I think these are great topics for the mailing list" "I confess I was for the ML as well, but in practice, for small things like that, I prefer the interaction here..." "regarding the enum-class-bool idiom, I guess there's .take_ownership = true to make it readable, but C++ doesn't have that for args, does it? Only starting with C++20 for structs, no?" "Well I don't know, introducing a type for a single parameter seems a bit overkill. You usually onl nee the take_ownership = false from plugin code" "I see. In a similar vein, you've decided not to make flags type-safe? E.g. int flags=SQLITE_OPEN_READWRITE|SQLITE_OPEN_CREATE" "right, because the sqlite flags are good enough. i didn't want to wrap everything and duplicate a bunch of code for little gain. the idea is more that the library is an extension of sqlite into C++, not a 100% wrapper" "What about opening a connection with a custom VFS? I've seen that in our code-base. Did you get into that? Both allowing using a custom VFS, and wrappers for VFSs similar to your vtable one?" "nope. I thought about it but couldn't come up with an example that was contrived. I might add it later if someone comes with a use-case" "The designated initializer doesn't work." "Requiring people to move all discussions to the ML is unnecessary friction that benefit a few at the inconvenience of many." "The point is to create an auditable paper trail so people can see why a library was accepted But for something like this where it's: hey, I think you messed up a path, it's not v. pertinent to the quality of the library being reviewed" "I think its very pertinent. The review evaluates the author as much as the library if not more so, since basically Boost is getting married to the author. A consistent pattern of paying great attention to small details such as paths is meaningful." "docs are updated" "The fact is that the previous review was stained by significant discussion taking place off-list. This was discussed after the fact, and now we are repeating this behavior. I agree there is less friction on Slack but I also like having robust reviews of the kind that can be referred to in the future " "Q&A should only be on the list if it's of value to other list members. if a reviewer asks questions for his own education so that he can determine whether to accept the library, these questions do not have to be on the list, although it can be useful for them to be if other reviewers are likely to have the same questions" "connection::prepare has two overloads, and I supposed the 1-arg one throws? The doc doesn't say. (it's kinda obvious, given the 3-args one, but still). Or perhaps there's a global mention in the doc somewhere about the throw-vs-nothrow overloads? (didn't read the manual yet)" "interactive Q&A here is less intimidating than writing to the ML, and more "instant", especially for small questions like mine so far." "nothrow for those overloads is not entirely correct, because the error-code overloads could still throw things like ENOMEM. it basically means that the failure of the main operation is not thrown" Should these discussions stay on Slack or should they be brought to the list? Thanks
Vinnie Falco wrote: ...
"these docs are so complete one gets tired of completeness when reading them"
"Should these questions be asked on the list instead of here?"
"dunno. I'm asking a simple question so far, on expectations I should have"
"Expect what you would expect from a complete Boost library."
"I was expecting at least the signatures of functions (Ctors in this instance), at least. thus I'm suspecting a ref doc (doxygen based?) generation issue, and was inquiring"
"Fwiw, I don't think it's inappropriate to ask about the docs like that here because yeah, that seems kind of just like a simple configuration error I'm guessing"
"these are good things to ask on the list and you should state your documentation expectations on the list"
"I intend to embed the sources in my code base, instead of compiling Boost. Will Boost.SQLite be compatible with my existing Boost 1.84, for its dependencies (if any)?"
"darn, all the classes docs aren't there." ...
If nothing else, this serves as a good demonstration as to exactly how useless a Slack to ML gateway would be.
If nothing else, this serves as a good demonstration as to exactly how useless a Slack to ML gateway would be.
Here's a summary (i.e. it's edited) of the questions I answered regarding this review so far: Q: explicit connection(handle_type handle, bool take_ownership = true) that reads inscrutably at the call site. I've taken the habit of using e.g. enum class HasOwnership : bool; inline constexpr HasOwnership cTakeOwnership{ true }; ... so the call site reads well. Bad idea? Is that something you'd consider? A: Well I don't know, introducing a type for a single parameter seems a bit overkill. You usually onl nee the take_ownership = false from plugin code it's used here ://github.com/klemens-morgenstern/sqlite/blob/9de8655d7e26f9b5d5cb99c05e91b213d4a607a4/include/boost/sqlite/detail/vtable.hpp#L27 Q: In a similar vein, you've decided not to make flags type-safe? E.g. int flags=SQLITE_OPEN_READWRITE|SQLITE_OPEN_CREATE A: right, because the sqlite flags are good enough. i didn't want to wrap everything and duplicate a bunch of code for little gain the idea is more that the library is an extension of sqlite into C++, not a 100% wrapper Q: What about opening a connection with a custom VFS? I've seen that in our code-base. Did you get into that? Both allowing using a custom VFS, and wrappers for VFSs similar to your vtable one? A: nope. I thought about it but couldn't come up with an example that was contrived. I might add it later if someone comes with a use-case Q: Why does the transaction not commit in the destructor if there is no current_exception? It's a reasonable decision, but I'm curious. A: This can for one lead to surprises if an early return happens. Secondly, a COMMIT might fail, where as a ROLLBACK won't. Unhandled errors in a destructor are a problem.
ср, 13 нояб. 2024 г. в 15:31, Richard Hodges via Boost <boost@lists.boost.org>:
The Boost formal review of the Boost SQLITE library starts *TODAY*
I have a few questions 1) I do not understand the boost_sqlite vs. boost_sqlite_ext situation. Should I always use the second one if I want to create an extension? If I want to both create an extension and use SQLite from the same binary, should I link to both? 2) The library can deduce struct composition in C++20. Have you considered using Boost.PFR to also be able to do it in C++14? 3) Regarding custom functions.Have you considered using multiple arguments for the callback rather than span<value, N>? I have overall weird feelings about the aggregate function API. When the passed callable is of a class type it should have particularly named member functions. That gives the impression that the state that those member functions use is supposed to be stored in the callable object. But instead it should be stored in the object of the type of the first parameter, and the callable object itself is used as a sort of database-local global. As I've said this is kind of confusing. Also, SQLite allows overloading based on the number of arguments. The current interface requires having a separate callable type for each such overload, because the special members cannot be overloaded or be templates, otherwise the deduction would not work. There also doesn't seem to be a way to remove functions. I am thinking of an interface like this: template <std::size_t N = boost::dynamic_extent, class F, class Ctx = std::nullptr_t> void create_aggregate_function(F f, Ctx ctx = Ctx{}) And then the callables that need the semi-global context would be called as f.step(ctx, value_0, ... value_n), and if they do not need that context, they'd be called as f.step(value_0, ... value_n). 4) (not a question) You use core::string_view in some APIs. Due to a (very unfortunate IMO) decision by the community that type is not public. So, instead you need to create an alias in your library and reference that alias in the docs. You can document the alias to have a compatible API to std::string_view.
On Thu, Nov 14, 2024 at 8:19 PM Дмитрий Архипов via Boost <boost@lists.boost.org> wrote:
ср, 13 нояб. 2024 г. в 15:31, Richard Hodges via Boost <boost@lists.boost.org>:
The Boost formal review of the Boost SQLITE library starts *TODAY*
I have a few questions
1) I do not understand the boost_sqlite vs. boost_sqlite_ext situation. Should I always use the second one if I want to create an extension? If I want to both create an extension and use SQLite from the same binary, should I link to both?
If you create an extension, you'll need to call into the database differently using the indirections in sqlite3ext.h. In order to avoid conflicts, this library puts everything you include into an inline namespace (sqlite::ext), to avoid linker issues. That way you get access to the host db with the same API, but the internal calls are different, so you'll need to link to sqlite3-ext. You can then use a regular sqlite3 library from the same binary, but not the same TU. If you accidentally include boost/sqlite in both modes, but only link one you'll get linker errors.
2) The library can deduce struct composition in C++20. Have you considered using Boost.PFR to also be able to do it in C++14?
I don't exactly know what you mean here. You can use pfr in C++20 for a static_result_set (and describe for earlier standards). struct author { std::string last_name; std::string first_name; }; for (auto author : conn.query<author>("select first_name, last_name from author"));
3) Regarding custom functions.Have you considered using multiple arguments for the callback rather than span<value, N>? I have overall weird feelings about the aggregate function API. When the passed callable is of a class type it should have particularly named member functions. That gives the impression that the state that those member functions use is supposed to be stored in the callable object. But instead it should be stored in the object of the type of the first parameter, and the callable object itself is used as a sort of database-local global. As I've said this is kind of confusing. Also, SQLite allows overloading based on the number of arguments. The current interface requires having a separate callable type for each such overload, because the special members cannot be overloaded or be templates, otherwise the deduction would not work. There also doesn't seem to be a way to remove functions.
I don't entirely disagree, but this behaviour is directly inherited from sqlite3. That is: every overload of a function has a state like this. And the most likely use is that you can pass state into the function Sqlite also guarantees that final will be called whenever the function is used, so this state can be used to share data between the invocations of step.
I am thinking of an interface like this:
template <std::size_t N = boost::dynamic_extent, class F, class Ctx = std::nullptr_t> void create_aggregate_function(F f, Ctx ctx = Ctx{})
And then the callables that need the semi-global context would be called as f.step(ctx, value_0, ... value_n), and if they do not need that context, they'd be called as f.step(value_0, ... value_n).
I don't think I fully understand. What would `f`/`this` be pointing to when `step` gets called?
4) (not a question) You use core::string_view in some APIs. Due to a (very unfortunate IMO) decision by the community that type is not public. So, instead you need to create an alias in your library and reference that alias in the docs. You can document the alias to have a compatible API to std::string_view.
Unfortunate, indeed.
чт, 14 нояб. 2024 г. в 16:13, Klemens Morgenstern via Boost <boost@lists.boost.org>:
If you create an extension, you'll need to call into the database differently using the indirections in sqlite3ext.h. In order to avoid conflicts, this library puts everything you include into an inline namespace (sqlite::ext), to avoid linker issues.
That way you get access to the host db with the same API, but the internal calls are different, so you'll need to link to sqlite3-ext.
You can then use a regular sqlite3 library from the same binary, but not the same TU. If you accidentally include boost/sqlite in both modes, but only link one you'll get linker errors.
All of this should be in the documentation.
I don't exactly know what you mean here. You can use pfr in C++20 for a static_result_set (and describe for earlier standards).
Boost.PFR is a C++14 library, so I don't understand where the C++20 restriction comes from.
I don't think I fully understand. What would `f`/`this` be pointing to when `step` gets called?
This is a simplified implementation of what I meant: https://godbolt.org/z/Mb5eqfvW4. Here, the state of the aggregate function is stored in the object whose member functions are called, and also the arguments are passed separately.
I don't exactly know what you mean here. You can use pfr in C++20 for a static_result_set (and describe for earlier standards).
Boost.PFR is a C++14 library, so I don't understand where the C++20 restriction comes from.
It's because structs get assigned by name (which is C++20 in pfr) The reason here is that it's too easy to get this wrong, especially when queries change over time. If you have small examples with a single table this isn't an issue, but once you start doing joins, assigning the result by name will catch bugs.
I don't think I fully understand. What would `f`/`this` be pointing to when `step` gets called?
This is a simplified implementation of what I meant: https://godbolt.org/z/Mb5eqfvW4. Here, the state of the aggregate function is stored in the object whose member functions are called, and also the arguments are passed separately.
Ok, so you want to implicitly create a new instance of the function every time. The example you posted could be done like this atm (ignoring the args for now): struct Count { int n = 0; void step(span<sqlite::value>) { ++n; } int final() { return n; } }; struct CountImpl { void step(Count & c, span<sqlite::value> sp) { c.step(sp); } auto final(Count & c) {return c.final(): } }; Would you think it would help if `CountImpl` could be made a template, so a user could just write `dynamic_aggregate<Count>()` ? What's the advantage of using multiple arguments of the same type over a span?
The Boost formal review of the Boost SQLITE library starts *TODAY*, taking place
Documentation: https://klemens.dev/sqlite/
Several questions to the author regarding documentation and API (not a review yet) 1. Transactions. I noticed that you call explicitly begin/connit conn.query("begin transaction;"); ... conn.query("commit;"); Both soci [1] and cppdb [2] and most database wrappers I have seen provide scopes that allow to commit or rollback in case of failure/exception. It is something very common and allowshandling translations in an exception safe way. Do you provide such a feature? 2 Example boost::sqlite::row r; boost::sqlite::query q = conn.query(...) do { auto r = q.current();'' std::cout << r.at(0u).get_text() << " authored " << r.at(1u).get_text() << std::endl; } while (q.read_next()); What happens when the result is empty? I'd rather expect to see for(;;) or while loop. How do I check if the result is valid? How do I manually iterate over the result without an automatic "for" loop. 3. What happens if you do not read the entire result? How do you abort the query? As far as I remember if you'll execute another statement when you hadn't finished you'll get SQLITE_BUSY error 4. Both soci and cppdb have some syntactic sugar to query a single row - for example when querying something by primary key. Can you do it without a loop? 5. How do you handle locks/collisions when two processes try to update the same DB. It is a tricky bit in sqlite - sometimes you need to wait, sometimes to abort. I noticed the word "error" appears only once in a tutorial... Please add documentation on error handling 6. Are objects copyable or only moveable? Please state explicitly Disclosure: I'm the author of cppdb and contributed to/used in past soci. Artyom Beilis 1) https://soci.sourceforge.net/doc/master/transactions/ 2) http://cppcms.com/sql/cppdb/transaction.html
On Thu, Nov 14, 2024 at 9:37 PM Artyom Beilis via Boost <boost@lists.boost.org> wrote:
The Boost formal review of the Boost SQLITE library starts *TODAY*, taking place
Documentation: https://klemens.dev/sqlite/
Several questions to the author regarding documentation and API (not a review yet)
1. Transactions.
I noticed that you call explicitly begin/connit
conn.query("begin transaction;"); ... conn.query("commit;");
Both soci [1] and cppdb [2] and most database wrappers I have seen provide scopes that allow to commit or rollback in case of failure/exception. It is something very common and allowshandling translations in an exception safe way.
Do you provide such a feature?
yes, sqlite::transaction: https://klemens.dev/sqlite/structboost_1_1sqlite_1_1transaction.html It does not commit in the destructor, but always calls rollback if there was no explicit commit.
2 Example
boost::sqlite::row r; boost::sqlite::query q = conn.query(...) do { auto r = q.current();'' std::cout << r.at(0u).get_text() << " authored " << r.at(1u).get_text() << std::endl; } while (q.read_next());
What happens when the result is empty? I'd rather expect to see for(;;) or while loop. How do I check if the result is valid? How do I manually iterate over the result without an automatic "for" loop.
boost::sqlite::query q = conn.query(...); if (q.read_next()) // read another row If the result is empty q.done() is true. Invalid results would lead to exception unless you use the `error_code&` overloads, e.g. boost::system::error_code ec; boost::sqlite::error_info ei; boost::sqlite::query q = conn.query(...,, ec, ei); if (!ec) if (q.read_next(ec, ei)) // read another row
3. What happens if you do not read the entire result? How do you abort the query? As far as I remember if you'll execute another statement when you hadn't finished you'll get SQLITE_BUSY error
Aborting the query is done by letting the `query` object go out of scope.
4. Both soci and cppdb have some syntactic sugar to query a single row - for example when querying something by primary key. Can you do it without a loop?
sqlite::row r = conn.query(...).row() ; or author a = conn.query<author>(...).row();
5. How do you handle locks/collisions when two processes try to update the same DB. It is a tricky bit in sqlite - sometimes you need to wait, sometimes to abort.
That would just be an error, there's no explicit handling in the library.
I noticed the word "error" appears only once in a tutorial... Please add documentation on error handling
Will do. Most functions have an overload taking an error_code & error_info by reference for the error and one that will throw an exception.
6. Are objects copyable or only moveable? Please state explicitly
Most are move-only. It's documented in the reference.
Disclosure: I'm the author of cppdb and contributed to/used in past soci.
Artyom Beilis
1) https://soci.sourceforge.net/doc/master/transactions/ 2) http://cppcms.com/sql/cppdb/transaction.html
_______________________________________________ Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost
I have some questions for the author: 1. Many functions are overloaded to accept an error_code and error_info, effectively having throwing and non-throwing overloads. Have you considered unifying these by using system::result? 2. What compilers and C++ standards are supported by the library? It looks like C++14, but docs don't say it. 3. What's the rationale behind BOOST_SQLITE_NO_VIRTUAL? Wouldn't setting it cause trouble, being a compiled library? 4. I'm interested in the use case of exposing an in-memory data structure through a virtual table. I'm looking at multi_index.cpp, the Doxygen comments and the official sqlite docs on virtual tables. Is there any additional docs on the topic that can help me? 5. Docs for field::get_text() state "Returns the value as text, i.e. a string_view. Note that this value may be invalidated" - what does this mean? Thanks, Ruben.
What's the current recommended way to consume the library? Installing it with CMake seems to only install the binaries (and not the headers/cmake find_package scripts), and I haven't found a Jamfile to build the library as part of the Boost superproject. Thanks, Ruben. On Sat, 16 Nov 2024 at 10:18, Ruben Perez <rubenperez038@gmail.com> wrote:
I have some questions for the author:
1. Many functions are overloaded to accept an error_code and error_info, effectively having throwing and non-throwing overloads. Have you considered unifying these by using system::result? 2. What compilers and C++ standards are supported by the library? It looks like C++14, but docs don't say it. 3. What's the rationale behind BOOST_SQLITE_NO_VIRTUAL? Wouldn't setting it cause trouble, being a compiled library? 4. I'm interested in the use case of exposing an in-memory data structure through a virtual table. I'm looking at multi_index.cpp, the Doxygen comments and the official sqlite docs on virtual tables. Is there any additional docs on the topic that can help me? 5. Docs for field::get_text() state "Returns the value as text, i.e. a string_view. Note that this value may be invalidated" - what does this mean?
Thanks, Ruben.
On Sat, Nov 16, 2024 at 5:18 PM Ruben Perez <rubenperez038@gmail.com> wrote:
I have some questions for the author:
1. Many functions are overloaded to accept an error_code and error_info, effectively having throwing and non-throwing overloads. Have you considered unifying these by using system::result?
I have. error_info contains a string, which allows you to minimizing reallocation by passing it in by reference. A result containing an error_info can't do that.
2. What compilers and C++ standards are supported by the library? It looks like C++14, but docs don't say it.
Yes, 14. Sorry for not mentioning this.
3. What's the rationale behind BOOST_SQLITE_NO_VIRTUAL? Wouldn't setting it cause trouble, being a compiled library?
It would. The idea is that you can use that for constrained (i.e. embedded environments) to avoid the unnecessary vtable and to enforce devirtualization if your compiler misses this.
4. I'm interested in the use case of exposing an in-memory data structure through a virtual table. I'm looking at multi_index.cpp, the Doxygen comments and the official sqlite docs on virtual tables. Is there any additional docs on the topic that can help me?
The only effective way I found is to read examples or try the API out, which is why I provided 5 examples of how to use it. The sqlite docs themselves are pretty good, it's just a bit overwhelming at first.
5. Docs for field::get_text() state "Returns the value as text, i.e. a string_view. Note that this value may be invalidated" - what does this mean?
It means the string_view points into the database or the state of a query. If it's the former it may get invalidated when the DB changes, in case of the latter it might get invalidated by moving the query to the next row. So it means: use it now, before you do anything else.
Thanks, Ruben.
3. What's the rationale behind BOOST_SQLITE_NO_VIRTUAL? Wouldn't setting it cause trouble, being a compiled library?
It would. The idea is that you can use that for constrained (i.e. embedded environments) to avoid the unnecessary vtable and to enforce devirtualization if your compiler misses this.
If I have read the code correctly, these functions seem to only be used in headers. Why do they need to be virtual? It looks like it could have been made a compile-time interface (i.e. a concept), completely avoiding the need for the macro. What's the rationale behind this?
On Sat, Nov 16, 2024 at 2:52 AM Klemens Morgenstern via Boost <boost@lists.boost.org> wrote:
I have. error_info contains a string, which allows you to minimizing reallocation by passing it in by reference.
1. Do you have any measurements to show the performance gains for this, and 2. Are you telling us that you "optimized" the case where errors occur? Thanks
On Sat, Nov 16, 2024 at 10:41 PM Vinnie Falco via Boost <boost@lists.boost.org> wrote:
On Sat, Nov 16, 2024 at 2:52 AM Klemens Morgenstern via Boost <boost@lists.boost.org> wrote:
I have. error_info contains a string, which allows you to minimizing reallocation by passing it in by reference.
1. Do you have any measurements to show the performance gains for this, and 2. Are you telling us that you "optimized" the case where errors occur?
This is not about performance. You can use sqlite in very resource constrained environments and you can configure it so it never mallocs. The error_info allocates its memory from the same pool and if you constantly realloc, you'll potentially end up fragmenting this memory. It's a very common pattern in those cases to allocate all the memory up front to avoid this fragmentation, and this interface allows that. It also uses realloc internally for the same reason.
Thanks
_______________________________________________ Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost
Hi people, Happy to begin my first review of a Boost library ! Thanks to the author for this opportunity - and the lib ;) I've had some tiny problems compiling and running the tests with Apple Clang (14 and 16). It seems linking to boost_url was missing so I had to edit the CMakelists.txt. I don't know if it's a mistake from my side or an edge-case for the build system, but anyway now that it builds I can not find how to have the tests pass: 1/1 Test #2: boost_test_extension_simple_scalar ...***Failed 0.02 sec Parse error near line 1: no such function: load_extension SELECT load_extension('./simple_scalar'); I would understand the author wants to avoid the rabbit hole of covering "how to build and run for every possible system", but having at least some indications in the docs could helpful ! :D It feels a bit weird to ask such frivolity on the ML, thanks for your patience, All have fun with the review ! Arno
Happy to begin my first review of a Boost library ! Thanks to the author for this opportunity - and the lib ;)
I've had some tiny problems compiling and running the tests with Apple Clang (14 and 16). It seems linking to boost_url was missing so I had to edit the CMakelists.txt. I don't know if it's a mistake from my side or an edge-case for the build system, but anyway now that it builds I can not find how to have the tests pass:
That was my mistake, I wrote that before url was a compiled lib and didn't add the examples to CI.
1/1 Test #2: boost_test_extension_simple_scalar ...***Failed 0.02 sec Parse error near line 1: no such function: load_extension SELECT load_extension('./simple_scalar');
I would understand the author wants to avoid the rabbit hole of covering
"how to build and run for every possible system", but having at least some indications in the docs could helpful ! :D
It feels a bit weird to ask such frivolity on the ML, thanks for your
This looks like load_extensions is disabled in your sqlite instance. Can you check by typing `.dbconfig` in the sqlite3 cli? ``` klemens@fedora:~$ sqlite3 SQLite version 3.46.1 2024-08-13 09:16:08 Enter ".help" for usage hints. Connected to a transient in-memory database. Use ".open FILENAME" to reopen on a persistent database. sqlite> .dbconfig defensive on dqs_ddl off dqs_dml off enable_fkey off enable_qpsg off enable_trigger on enable_view on fts3_tokenizer off legacy_alter_table off legacy_file_format off load_extension on no_ckpt_on_close off reset_database off reverse_scanorder off stmt_scanstatus off trigger_eqp off trusted_schema off writable_schema off sqlite> ``` See the `load_extension` line. If it's false the test just won't work; I will need to add a workaround for this either way. patience, There's nothing frivolous about your question.
I've finally got to understand multi_index.cpp and adapt it to my data structure. I understand the use case that this example is trying to cover is exposing an in-memory data structure as a sqlite interface. Say I've got a server process holding such a multi-index. I've got a working module for the multi-index. I can load it and call it from the server process. I'd now like to somehow use the module from outside the process (e.g. from the sqlite3 command line tool) to inspect my data. Is such a use possible? What would be the recommended architecture? Or is it best to keep such modules scoped to the host process? Thanks, Ruben.
vtable.hpp contains the following piece of code: auto r = mod.insert(value{argv[1]}, boost::span<value>{reinterpret_cast<value *>(argv + 2), static_cast<std::size_t>(argc - 2)}, sqlite3_vtab_on_conflict(db)); Where argv is a sqlite3_value **, and sqlite3::value is a wrapper class roughly looking like this: struct value { // More member functions explicit value(sqlite3_value * value_) noexcept : value_(value_) {} private: sqlite3_value * value_ = nullptr; }; I had the impression that such a cast is UB (although it seems to work and seems to be accepted by ubsan). Could someone with more knowledge about the language point out whether this is allowed or not (and why) Thanks, Ruben.
On Sat, Nov 16, 2024 at 9:18 PM Ruben Perez <rubenperez038@gmail.com> wrote:
vtable.hpp contains the following piece of code:
auto r = mod.insert(value{argv[1]}, boost::span<value>{reinterpret_cast<value *>(argv + 2),
static_cast<std::size_t>(argc - 2)}, sqlite3_vtab_on_conflict(db));
Where argv is a sqlite3_value **, and sqlite3::value is a wrapper class roughly looking like this:
struct value { // More member functions explicit value(sqlite3_value * value_) noexcept : value_(value_) {} private: sqlite3_value * value_ = nullptr; };
I had the impression that such a cast is UB (although it seems to work and seems to be accepted by ubsan). Could someone with more knowledge about the language point out whether this is allowed or not (and why)
I have a static assert in the code, to make sure this works out. static_assert(sizeof(value) == sizeof(sqlite3_value*), "value must be same as sqlite3_value* pointer"); If the struct is aligned to the pointer it should be fine. But it might technically still be UB, I am not sure either.
Thanks, Ruben.
**Boost.Sqlite review** It is more than ten years ago that I did a Boost review, but seeing Boost.Sqlite entering the stage I thought it good to evaluate it, because I’m a sqlite fan and used some other wrappers in the past. My compliments to Klemens to propose this awesome library. Below you will see that I vote for acceptance. Also I will answer the suggested questions, but first I’ll write my findings here. **Installation** I used the CMake flavour. That didn’t go smoothly, among others because Doxygen is required and not in my standard path, and because I’ve two or more locations with Boost. Anyway, that all is personal and not really relevant because in the end it will be shipped with Boost. And I managed to solve it quickly by using an adapted CMake file. **Documentation** I started by going through the documentation. The documentation looks good and the left side menu is convenient. However, it is a bit concise and goes too early into the tiny details, while skipping the happy path workflow. Describing the sqlite connection first is fine of course. But then it inserts 4 rows immediately, goes to a transaction where rows are inserted differently by an unnecessary construction and then goes to a custom aggregate option right away, in the first query - without having presented a “normal” query…. It looks more like a showcase than a quick start. It would IMO be good to split it into a Tutorial part and a Functionality (or so) part (the showcase part). This could also be an Advanced section in the tutorial. Please remove the names of Boost Authors from the documentation, and all unit tests, as this might raise eyebrows. It is better to take a more neutral example (Chinook is often used for sample databases). **Step 1** My step 1 would probably be a candidate for a slow-paced introductory start for a tutorial - going through the standard options slowly. Just by making a connection and doing some basic inserts and queries in different ways. All I tried worked, either immediately, or by trial and error or consulting source code Creating a connection Inserting Using indexed parameters Using named parameters Entering fields one by one (not documented! Please add it!) Either using a transaction, or without Querying Simple using row.at Prepared with a named parameter (not documented for queries! Please add it!) With tuples (awesome!) With a struct in C++ (this is really cool!) (it would also be cool if a struct could be inserted!) My code is attached. **Step 2** In my step 2 I used a blob, trying to insert an image and get it back. Using blobs should go into the documentation as well, not just in the reference part, but how to insert them, how to get them, what is the difference between a blob_view and a blob / when to use what, … Anyway, it’s simple. The next line is all needed (where get_blob is a local lambda reading it from a file). conn.prepare("insert into italy (regio_name, image) values (?1, ?2)").execute({"Veneto", get_blob("veneto.png")}); This “normal” workflow works conveniently and correctly, I can insert the image, get it back, and using an SQLite viewer in VS Code I can see the inserted image there. Cool. Then I try to use the tuple approach (fields one by one). This does not work here. I’m not sure if it’s not supported, or the way I do it is wrong. The main issue is that a blob is not constructible without parameters. Therefore you need to enter it where you declare it. See below. After that I query for the blob. Walking manually as per documentation, this works and I get the blobs using row.at(2u).get_blob() But using the query_result (described query), it does not work. According to the documentation, it should work, blob and blob_view are explicitly mentioned there in the bullet list. But a compiler error stops me here… “note: default constructor of 'query_result' is implicitly deleted because field 'image' has no default constructor boost::sqlite::blob_view image;” Maybe I need tag_invoke, also listed in the compiler errors. But the documentation does not mention it for that purpose. I didn’t try it. So I now have: struct query_result { sqlite_int64 id; boost::sqlite::string_view regio_name; boost::sqlite::string_view image; // COMPILES, but wrong result // boost::sqlite::blob_view image; // DOES NOT COMPILE // boost::sqlite::blob image; // DOES NOT COMPILE // boost::sqlite::value image; // DOES NOT COMPILE }; Anyway, that was the second option. The first option works correctly so I can do what I want to do. **Step 3** In this step I evaluated column meta information, and I added a custom SQL function (cool!). For this effort I used an existing database, a geopackage, which is a geospatial database using an sqlite3 database as its storage medium. One of its columns is typed geometry, which is not listed in this sqlite documentation (that’s OK), but apparently it can be handled as a blob by the library. Awesome. Because of this geometry-typed column, I verified the column meta information (please add to the quick start how to do these things! Neither is it in the reference). const auto meta = table_column_meta_data(conn, "limits_IT_regions", "geom"); std::cout << "Column: " << meta.data_type << " " << meta.collation << " " << meta.not_null << " " << meta.primary_key << " " << meta.auto_increment << std::endl; This works easily, all suggested by copilot, and gives me: Column: GEOMETRY BINARY 0 0 0 By the way, how can I get table names, and the whole schema? (Didn’t look it up). Then I added a custom sql scalar function my_blob_size boost::sqlite::create_scalar_function(conn, "my_blob_size", [](boost::sqlite::context<std::size_t> ctx, boost::span<boost::sqlite::value, 1u> args) -> std::size_t { return args[0].get_blob().size(); }); This is awesome functionality and works simple and as expected. My query is now select fid,geom,my_blob_size(geom) as blob_size,reg_name from limits_IT_regions And it gives, obviously, the size of the blob. The method get_blob returns a blob_view, maybe this might need a renaming. As mentioned before, creating a function is not documented in the quick start (it starts with aggregate right away - but this simpler scalar function with a lambda misses). The reference has an example how to create it - but a small example how to use it would be nice. **Implementation** I was very happy to evaluate the library from a user perspective in some simple steps. I only glanced through the implementation, stepping through it with the debugger. It looks clear, and like a small-enough wrapper around the sqlite code. Also, I had to look some things up because they were not documented. **Typos** thos associated → those “// a case insensitive string omparison” → comparison The statemens need to be kept alive. → statements (various places) This can be a map, a vector or a stuple → tuple for the exception less overload. → “exceptionless” or “overload with error code” (it’s actually clear enough without) “The following types are allowed in a static query result:” → static_result_set It's interfaces -> Its interfaces or include boost/sqlite/extensions.hpp first → called `extension.hpp` auto r = q.current();’’ → the two quotes should be removed sqlite::create_function(conn, "my_sum",, → the double comma should go (In source): Perform a query without parametert, It execute a multiple statement. → parameter, executes (probably meant here is: “It can execute multiple statements”) The handle is shared between the statement & resultset. The statemens need to be kept alive. → statements (as mentioned above) and the & looks to drafty. The last two typos indicate that the sample code is not compiled! That would be very welcome. I noticed some other errors as well. **Errors in documentation** 1: sqlite::connection conn{":memory:"}; { sqlite::connection read{"./read_only_db.db", SQLITE_READONLY}; backup(read, target); } → it doesn’t use conn - it doesn’t define target, I don’t think it’s right 2: sqlite::create_function(conn, "win_char_counter", aggregate_func{}); It’s incorrect, because right above, the window_func is defined. Also it’s not called create_function, it’s called create_window_function 3: sqlite::create_function( conn, "to_upper", [](sqlite::context<> ctx, boost::span<sqlite::value, 1u> args) -> std::string { std::string res; auto txt = val[0].get_text(); res.resize(txt.size()); std::transform(txt.begin(), txt.end(), res.begin(), [](char c){return std::toupper(c);}); return value; }); args → val (or vice versa) return value → return res; ::create_function → create_scalar_function This clearly indicates that the documentation samples should be compiled, automatically (as done for example in Boost.Geometry). **Unclarity in documentation** 1: The documentation should make clear that these lines struct query_result { std::string first_name, lib_name;}; BOOST_DESCRIBE_STRUCT(query_result, (), (first_name, lib_name)); // this can be omitted with C++20. should be declared OUTSIDE the scope where the next lines go. It does not compile, the way it is presented in the documentation. 2: Samples how to use JSON. Do we really need it in this library? Isn’t it an extension that can be documented, but not included? That would also avoid a dependency. 3: Samples how to use BLOB I had to inspect the unit test for it What is open_blob ? 4: About ”supports variants & json”(mentioned in the library comparison) → Where is the variant in the documentation? How to do this? What does it? → Isn’t tuple a better argument for this comparison? 5: The tag_invoke should be explained better. Why do we need it? How do we define it? A sample would be very welcome. I ignored it further, so maybe I don’t need it. Or did I need it for described structures with blobs? **Other topics** Documentation: I think the library comparison can be omitted in the end, but during the review phase, it is fine. CPP code lines: 614. HPP code lines: 4179. If it is like this, can’t we make it header only? On the other hand - many structures are not templated. Shouldn’t their implementations then be moved to the sources? There is some unused functionality, for example bool glob. I can’t find any usage or mentioning of it Because there is a bind implementation anyway - can that be exposed? I think it is common practice to bind parameter by parameter, at least I missed it here (therefore I concentrated on the tuple-way, which is a nice alternative). **Other answers**
Will the library bring additional out-of-the-box utility to Boost? Yes, sqlite is tremendously popular and used by nearly every programmer. It might become one of the most used Boost libraries.
- What is your evaluation of the implementation? I only glanced through the implementation. It looks clean. I did the review mostly from a user perspective. I judged the API which looks very simple, still powerful, and useful to me. - What is your evaluation of the documentation? The layout looks very good. The typesetting is very good. There are some typos, errors and there are some improvements possible, as described above. - Will the choice of API abstraction model ease the development of software that must talk to a SQLITE database? For sure it will make it much easier. - Are there any immediate improvements that could be made after acceptance, if acceptance should happen? The errors in the documentation should be fixed. I also encountered some unclarities in behaviour, that might be addressed, either by fixing them, or by pointing me out what I did wrong and/or improving the documentation. - Did you try to use the library? With which compiler(s)? Did you have any problems? I made 3 small test programs, all attached, on MacOS using clang, with C++14 and C++20. There were no real problems. Details are described above. - How much effort did you put into your evaluation? A glance? A quick reading? In-depth study? I spent more than a full day on this review. - Are you knowledgeable about the problem domain? Yes, I know SQL databases and I have used sqlite a lot. Also I normally use geospatial databases a lot. As also tried in my review research, step 3. And I’m a Boost author (Boost.Geometry).
Please provide in your review information you think is valuable to explain your choice to ACCEPT or REJECT including SQLITE as a Boost library. Please be explicit about your decision (ACCEPT or REJECT).
**Summary** Certainly I ACCEPT the library. Boost.Sqlite is a great addition to Boost, it is easy to use, it is powerful, the API makes sense, and it looks better to me than any other SQL API I used before (but I didn’t compare it during this review). There are some errors and inconveniences in the documentation, but it is relatively minor and I expect that this will all be handled. The library is already very useful right away, as is. This acceptance is therefore UNCONDITIONAL. Thanks for submitting the library, and thanks for managing the review. Kind regards, Barend Gehrels
On Mon, Nov 18, 2024 at 1:03 AM Barend Gehrels via Boost < boost@lists.boost.org> wrote:
**Boost.Sqlite review**
It is more than ten years ago that I did a Boost review, but seeing Boost.Sqlite entering the stage I thought it good to evaluate it, because I’m a sqlite fan and used some other wrappers in the past.
My compliments to Klemens to propose this awesome library. Below you will see that I vote for acceptance. Also I will answer the suggested questions, but first I’ll write my findings here.
Thanks for the long & detailed review. I got a few minor comments below.
**Installation** I used the CMake flavour. That didn’t go smoothly, among others because Doxygen is required and not in my standard path, and because I’ve two or more locations with Boost. Anyway, that all is personal and not really relevant because in the end it will be shipped with Boost. And I managed to solve it quickly by using an adapted CMake file.
**Documentation** I started by going through the documentation. The documentation looks good and the left side menu is convenient. However, it is a bit concise and goes too early into the tiny details, while skipping the happy path workflow. Describing the sqlite connection first is fine of course. But then it inserts 4 rows immediately, goes to a transaction where rows are inserted differently by an unnecessary construction and then goes to a custom aggregate option right away, in the first query - without having presented a “normal” query…. It looks more like a showcase than a quick start.
It would IMO be good to split it into a Tutorial part and a Functionality (or so) part (the showcase part). This could also be an Advanced section in the tutorial.
Please remove the names of Boost Authors from the documentation, and all unit tests, as this might raise eyebrows. It is better to take a more neutral example (Chinook is often used for sample databases).
**Step 1** My step 1 would probably be a candidate for a slow-paced introductory start for a tutorial - going through the standard options slowly. Just by making a connection and doing some basic inserts and queries in different ways.
All I tried worked, either immediately, or by trial and error or consulting source code Creating a connection Inserting Using indexed parameters Using named parameters Entering fields one by one (not documented! Please add it!) Either using a transaction, or without Querying Simple using row.at Prepared with a named parameter (not documented for queries! Please add it!) With tuples (awesome!) With a struct in C++ (this is really cool!) (it would also be cool if a struct could be inserted!)
Do you mean using the member of the structs by name for a parametrized query?
My code is attached.
**Step 2** In my step 2 I used a blob, trying to insert an image and get it back. Using blobs should go into the documentation as well, not just in the reference part, but how to insert them, how to get them, what is the difference between a blob_view and a blob / when to use what, …
Anyway, it’s simple. The next line is all needed (where get_blob is a local lambda reading it from a file).
conn.prepare("insert into italy (regio_name, image) values (?1, ?2)").execute({"Veneto", get_blob("veneto.png")});
This “normal” workflow works conveniently and correctly, I can insert the image, get it back, and using an SQLite viewer in VS Code I can see the inserted image there. Cool.
Then I try to use the tuple approach (fields one by one). This does not work here. I’m not sure if it’s not supported, or the way I do it is wrong. The main issue is that a blob is not constructible without parameters. Therefore you need to enter it where you declare it. See below.
After that I query for the blob. Walking manually as per documentation, this works and I get the blobs using row.at(2u).get_blob() But using the query_result (described query), it does not work. According to the documentation, it should work, blob and blob_view are explicitly mentioned there in the bullet list. But a compiler error stops me here… “note: default constructor of 'query_result' is implicitly deleted because field 'image' has no default constructor boost::sqlite::blob_view image;” Maybe I need tag_invoke, also listed in the compiler errors. But the documentation does not mention it for that purpose. I didn’t try it.
So I now have: struct query_result { sqlite_int64 id; boost::sqlite::string_view regio_name; boost::sqlite::string_view image; // COMPILES, but wrong result // boost::sqlite::blob_view image; // DOES NOT COMPILE // boost::sqlite::blob image; // DOES NOT COMPILE // boost::sqlite::value image; // DOES NOT COMPILE };
Anyway, that was the second option. The first option works correctly so I can do what I want to do.
I'll look into that.
**Step 3** In this step I evaluated column meta information, and I added a custom SQL function (cool!).
For this effort I used an existing database, a geopackage, which is a geospatial database using an sqlite3 database as its storage medium. One of its columns is typed geometry, which is not listed in this sqlite documentation (that’s OK), but apparently it can be handled as a blob by the library. Awesome.
This is probably a subtype, which would make a great example for custom conversions. Are you using spatialite?
[...snip]
**Unclarity in documentation**
1: The documentation should make clear that these lines
struct query_result { std::string first_name, lib_name;}; BOOST_DESCRIBE_STRUCT(query_result, (), (first_name, lib_name)); // this can be omitted with C++20.
should be declared OUTSIDE the scope where the next lines go. It does not compile, the way it is presented in the documentation.
2: Samples how to use JSON. Do we really need it in this library? Isn’t it an extension that can be documented, but not included? That would also avoid a dependency.
I have used json data extensively in sqlite and it's usually enabled by default these days, so I think it's a good default to enable it. 3:
Samples how to use BLOB I had to inspect the unit test for it What is open_blob ?
You can open a blob field directly without a query and read it's data. That can be especially advantageous if you just want to access part of it. 4: About ”supports variants & json”(mentioned in the library comparison)
→ Where is the variant in the documentation? How to do this? What does it? → Isn’t tuple a better argument for this comparison?
5: The tag_invoke should be explained better. Why do we need it? How do we define it? A sample would be very welcome. I ignored it further, so maybe I don’t need it. Or did I need it for described structures with blobs?
You need it for custom conversions. It's in the url example, but under documented.
**Other topics**
Documentation: I think the library comparison can be omitted in the end, but during the review phase, it is fine. CPP code lines: 614. HPP code lines: 4179. If it is like this, can’t we make it header only? On the other hand - many structures are not templated. Shouldn’t their implementations then be moved to the sources? There is some unused functionality, for example bool glob. I can’t find any usage or mentioning of it Because there is a bind implementation anyway - can that be exposed? I think it is common practice to bind parameter by parameter, at least I missed it here (therefore I concentrated on the tuple-way, which is a nice alternative).
**Other answers**
Will the library bring additional out-of-the-box utility to Boost? Yes, sqlite is tremendously popular and used by nearly every programmer. It might become one of the most used Boost libraries.
- What is your evaluation of the implementation? I only glanced through the implementation. It looks clean. I did the review mostly from a user perspective. I judged the API which looks very simple, still powerful, and useful to me.
- What is your evaluation of the documentation? The layout looks very good. The typesetting is very good. There are some typos, errors and there are some improvements possible, as described above.
- Will the choice of API abstraction model ease the development of software that must talk to a SQLITE database? For sure it will make it much easier.
- Are there any immediate improvements that could be made after acceptance, if acceptance should happen? The errors in the documentation should be fixed. I also encountered some unclarities in behaviour, that might be addressed, either by fixing them, or by pointing me out what I did wrong and/or improving the documentation.
- Did you try to use the library? With which compiler(s)? Did you have any problems? I made 3 small test programs, all attached, on MacOS using clang, with C++14 and C++20. There were no real problems. Details are described above.
- How much effort did you put into your evaluation? A glance? A quick reading? In-depth study? I spent more than a full day on this review.
- Are you knowledgeable about the problem domain? Yes, I know SQL databases and I have used sqlite a lot. Also I normally use geospatial databases a lot. As also tried in my review research, step 3. And I’m a Boost author (Boost.Geometry).
Please provide in your review information you think is valuable to explain your choice to ACCEPT or REJECT including SQLITE as a Boost library. Please be explicit about your decision (ACCEPT or REJECT).
**Summary**
Certainly I ACCEPT the library. Boost.Sqlite is a great addition to Boost, it is easy to use, it is powerful, the API makes sense, and it looks better to me than any other SQL API I used before (but I didn’t compare it during this review).
There are some errors and inconveniences in the documentation, but it is relatively minor and I expect that this will all be handled. The library is already very useful right away, as is. This acceptance is therefore UNCONDITIONAL.
Thanks for submitting the library, and thanks for managing the review.
Thank you for taking the time to write a review.
Kind regards, Barend Gehrels _______________________________________________ Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost
On Mon, Nov 18, 2024 at 10:34 AM Klemens Morgenstern via Boost <boost@lists.boost.org> wrote:
On Mon, Nov 18, 2024 at 1:03 AM Barend Gehrels via Boost <boost@lists.boost.org> wrote:
One of its columns is typed geometry, which is not listed in this sqlite documentation (that’s OK), but apparently it can be handled as a blob by the library. Awesome.
This is probably a subtype, which would make a great example for custom conversions. Are you using spatialite?
SQLite has 5 types, period [1]. And that includes NULL as a type. [1]: https://www.sqlite.org/datatype3.html#storage_classes_and_datatypes Unless you use STRICT tables [2], you can name your types anything. So having a types named GEOMETRY means nothing to SQLite, and its affinity rules kick in [3] to see how the value is handled, often as a blob. [2]: https://www.sqlite.org/stricttables.html [3] https://www.sqlite.org/datatype3.html#type_affinity The nothing of subtype [4] exist in SQLite, but is very *transient*. It is never stored in DBs (in memory or otherwise), and only used to convey transient typing information from the result of one function, to the argument of another function. Subtypes are not even preserved by views using functions adding subtype information on their results. [4]: https://www.sqlite.org/c3ref/result_subtype.html SQLite typing is weird and full of gotchas. Sorry for being pedantic, but better to avoid misconception about subtyping, in SQLite at least. Perhaps Klemens meant it in a different context. --DD
On Mon, Nov 18, 2024 at 6:10 PM Dominique Devienne <ddevienne@gmail.com> wrote:
On Mon, Nov 18, 2024 at 10:34 AM Klemens Morgenstern via Boost <boost@lists.boost.org> wrote:
On Mon, Nov 18, 2024 at 1:03 AM Barend Gehrels via Boost < boost@lists.boost.org> wrote:
One of its columns is typed geometry, which is not listed in this sqlite documentation (that’s OK), but apparently it can be handled as a blob by the library. Awesome.
This is probably a subtype, which would make a great example for custom conversions. Are you using spatialite?
SQLite has 5 types, period [1]. And that includes NULL as a type. [1]: https://www.sqlite.org/datatype3.html#storage_classes_and_datatypes
Unless you use STRICT tables [2], you can name your types anything. So having a types named GEOMETRY means nothing to SQLite, and its affinity rules kick in [3] to see how the value is handled, often as a blob. [2]: https://www.sqlite.org/stricttables.html [3] https://www.sqlite.org/datatype3.html#type_affinity
The nothing of subtype [4] exist in SQLite, but is very *transient*. It is never stored in DBs (in memory or otherwise), and only used to convey transient typing information from the result of one function, to the argument of another function. Subtypes are not even preserved by views using functions adding subtype information on their results. [4]: https://www.sqlite.org/c3ref/result_subtype.html
SQLite typing is weird and full of gotchas. Sorry for being pedantic, but better to avoid misconception about subtyping, in SQLite at least. Perhaps Klemens meant it in a different context. --DD
I meant it in the context you said: return values of functions, which includes vtables. It would be good to have an example explaining what you just said, because I certainly fell for the subtype gotchas in the past.
n Mon, Nov 18, 2024 at 1:09 PM Klemens Morgenstern <klemensdavidmorgenstern@gmail.com> wrote:
On Mon, Nov 18, 2024 at 6:10 PM Dominique Devienne <ddevienne@gmail.com> wrote:
On Mon, Nov 18, 2024 at 10:34 AM Klemens Morgenstern via Boost <boost@lists.boost.org> wrote:
On Mon, Nov 18, 2024 at 1:03 AM Barend Gehrels via Boost <boost@lists.boost.org> wrote:
One of its columns is typed geometry This is probably a subtype, which would make a great example for custom conversions. Are you using spatialite? The notion of subtype [4] exist in SQLite, but is very *transient*.
It would be good to have an example explaining what you just said, because I certainly fell for the subtype gotchas in the past.
https://sqlite.org/forum/forum?s=subtype in general, and https://sqlite.org/forum/forumpost/340b2fde7a30de50bf3189d6813289f80e6462132... in particular for the fact subtypes from views are lost. --DD
Thanks for the long & detailed review. I got a few minor comments below.
Thanks - and also sorry for the layout of the review, I had it earlier locally but it didn't take over all lists etc correctly.
With a struct in C++ (this is really cool!) (it would also be cool if a struct could be inserted!)
Do you mean using the member of the structs by name for a parametrized query?
Yes! I realize this goes into the direction of a ORM. But you have it for a select query as well. So this would be nice (now, in the future, or maybe it doesn't fit in the library approach): struct query_result { int64_tid; std::string regio_name; int64_t population; double latitude; double longitude; }; auto stat = conn.prepare("insert into italy (regio_name, population, latitude, longitude) values (?1, ?2, ?3, ?4)"); query_result qr{0, "Calabria", 1947131, 38.905, 16.5944}; stat.execute(qr);
This is probably a subtype, which would make a great example for custom conversions. Are you using spatialite?
No, I'm using a geopackage. But it's similar, it's sqlite including geometry. https://en.wikipedia.org/wiki/GeoPackage
2: Samples how to use JSON. Do we really need it in this library? Isn’t it an extension that can be documented, but not included? That would also avoid a dependency.
I have used json data extensively in sqlite and it's usually enabled by default these days, so I think it's a good default to enable it.
All right, clear
3: Samples how to use BLOB I had to inspect the unit test for it What is open_blob ?
You can open a blob field directly without a query and read it's data. That can be especially advantageous if you just want to access part of it.
Then it would be nice if it is documented, including a sample!
5: The tag_invoke should be explained better. Why do we need it? How do we define it? A sample would be very welcome. I ignored it further, so maybe I don’t need it. Or did I need it for described structures with blobs?
You need it for custom conversions. It's in the url example, but under documented.
All right! Cheers, Barend
What's the best way to specify a nullable field in the static interface? std::optional<T> and boost::optional<T> don't seem to work, nor does having a sqlite::field member. Thanks, Ruben.
On Tue, Nov 19, 2024 at 4:30 PM Ruben Perez <rubenperez038@gmail.com> wrote:
What's the best way to specify a nullable field in the static interface? std::optional<T> and boost::optional<T> don't seem to work, nor does having a sqlite::field member.
sqltie:;value would work. I should add optional support though.
Thanks, Ruben.
Am 19.11.2024 um 09:37 schrieb Klemens Morgenstern via Boost:
sqltie:;value would work. I should add optional support though.
maybe a related question - how to you handle overflows - for example reading a too large value into a float? is there a way to define how safe the conversion should be (plain cast, check for overflow and throw exception)?
On Tue, Nov 19, 2024 at 4:46 PM Dennis Luehring via Boost < boost@lists.boost.org> wrote:
Am 19.11.2024 um 09:37 schrieb Klemens Morgenstern via Boost:
sqltie:;value would work. I should add optional support though.
maybe a related question - how to you handle overflows - for example reading a too large value into a float?
That's not supported, you'll need to provide a double or a 64 bit int.
is there a way to define how safe the conversion should be (plain cast, check for overflow and throw exception)?
It's a minimal set of types at the moment and these are all safe. A user can add his own conversions and will then be responsible to assure it's safe.
_______________________________________________ Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost
maybe a related question - how to you handle overflows - for example reading a too large value into a float? That's not supported, you'll need to provide a double or a 64 bit int. What do you mean by "not supported"? What happens when you read a value larger than INT_MAX to an int? Or a value exceeding FLOAT_MAX but still using a float? Will those be silently truncated, UB in case of int, wrapped to inf for float? Haven't checked yet, but I guess reading an int value to a string will cause an error/exception to be generated by the library, doesn't it? I'd argue the same should be done if the value doesn't fit in the numeric target type is there a way to define how safe the conversion should be (plain cast, check for overflow and throw exception)?
It's a minimal set of types at the moment and these are all safe. A user can add his own conversions and will then be responsible to assure it's safe. See above: In which way are they "safe"? Is this in the documentation?
On Tue, Nov 19, 2024 at 6:13 PM Alexander Grund via Boost < boost@lists.boost.org> wrote:
maybe a related question - how to you handle overflows - for example reading a too large value into a float? That's not supported, you'll need to provide a double or a 64 bit int. What do you mean by "not supported"? What happens when you read a value larger than INT_MAX to an int? Or a value exceeding FLOAT_MAX but still using a float? Will those be silently truncated, UB in case of int, wrapped to inf for float?
It won't compile. You can't read into an `int` implicitly. Haven't checked yet, but I guess reading an int value to a string will
cause an error/exception to be generated by the library, doesn't it? I'd argue the same should be done if the value doesn't fit in the numeric target type
No, you can freely convert as much as you want. sqlite is not strictly typed. Ideally you'd check the type beforehand.
is there a way to define how safe the conversion should be (plain cast, check for overflow and throw exception)?
It's a minimal set of types at the moment and these are all safe. A user can add his own conversions and will then be responsible to assure it's safe. See above: In which way are they "safe"? Is this in the documentation?
Truncating doesn't compile, but conversions do, following sqlite rules.
_______________________________________________ Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost
Am 19.11.2024 um 11:26 schrieb Klemens Morgenstern via Boost:
Haven't checked yet, but I guess reading an int value to a string will
haven't checked yet is not a good statement in a review process :) you should have at least all "possible" use-case checked - at best with a unit-test
On Tue, Nov 19, 2024, 6:36 PM Dennis Luehring via Boost < boost@lists.boost.org> wrote:
Am 19.11.2024 um 11:26 schrieb Klemens Morgenstern via Boost:
Haven't checked yet, but I guess reading an int value to a string will
haven't checked yet is not a good statement in a review process :)
That wasn't my statement, but Alexanders.
you should have at least all "possible" use-case checked - at best with a unit-test
_______________________________________________ Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost
On Tue, Nov 19, 2024 at 6:26 PM Klemens Morgenstern < klemensdavidmorgenstern@gmail.com> wrote:
On Tue, Nov 19, 2024 at 6:13 PM Alexander Grund via Boost < boost@lists.boost.org> wrote:
maybe a related question - how to you handle overflows - for example reading a too large value into a float? That's not supported, you'll need to provide a double or a 64 bit int. What do you mean by "not supported"? What happens when you read a value larger than INT_MAX to an int? Or a value exceeding FLOAT_MAX but still using a float? Will those be silently truncated, UB in case of int, wrapped to inf for float?
It won't compile. You can't read into an `int` implicitly.
Correction: I misremembered that part and didn't double check. You can read into an int implicitly, but you cannot read into a float, only double. So you can get an integer overflow if you're not careful.
On Tue, 19 Nov 2024 at 23:04, Klemens Morgenstern via Boost <boost@lists.boost.org> wrote:
On Tue, Nov 19, 2024 at 6:26 PM Klemens Morgenstern < klemensdavidmorgenstern@gmail.com> wrote:
On Tue, Nov 19, 2024 at 6:13 PM Alexander Grund via Boost < boost@lists.boost.org> wrote:
maybe a related question - how to you handle overflows - for example reading a too large value into a float? That's not supported, you'll need to provide a double or a 64 bit int. What do you mean by "not supported"? What happens when you read a value larger than INT_MAX to an int? Or a value exceeding FLOAT_MAX but still using a float? Will those be silently truncated, UB in case of int, wrapped to inf for float?
It won't compile. You can't read into an `int` implicitly.
Correction: I misremembered that part and didn't double check. You can read into an int implicitly, but you cannot read into a float, only double.
So you can get an integer overflow if you're not careful.
I think that the library should be checking it and emitting an error, then.
_______________________________________________ Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost
ср, 20 нояб. 2024 г. в 13:18, Ruben Perez via Boost <boost@lists.boost.org>:
I think that the library should be checking it and emitting an error, then.
FWIW, in a similar situation Boost.JSON checks if the value is within the range of the integer type it is assigned to, and sets an error if it does not.
On Tue, 19 Nov 2024 at 09:37, Klemens Morgenstern <klemensdavidmorgenstern@gmail.com> wrote:
On Tue, Nov 19, 2024 at 4:30 PM Ruben Perez <rubenperez038@gmail.com> wrote:
What's the best way to specify a nullable field in the static interface? std::optional<T> and boost::optional<T> don't seem to work, nor does having a sqlite::field member.
sqltie:;value would work. I should add optional support though.
It doesn't seem to work. Apparently, some part of the machinery requires the row type to be default-constructible, making sqlite::value unusable in that context. Anyway, sqlite::field explicitly states it's for query results, while sqlite::value is for usage in internal APIs. This is holding the results of a query, so why is it sqlite::value the one that looks to be supported?
On Tue, Nov 19, 2024 at 5:05 PM Ruben Perez <rubenperez038@gmail.com> wrote:
On Tue, 19 Nov 2024 at 09:37, Klemens Morgenstern <klemensdavidmorgenstern@gmail.com> wrote:
On Tue, Nov 19, 2024 at 4:30 PM Ruben Perez <rubenperez038@gmail.com>
wrote:
What's the best way to specify a nullable field in the static interface? std::optional<T> and boost::optional<T> don't seem to work, nor does having a sqlite::field member.
sqltie:;value would work. I should add optional support though.
It doesn't seem to work. Apparently, some part of the machinery requires the row type to be default-constructible, making sqlite::value unusable in that context.
Yes, that's a bug in the way the static_result is implemented. I wanted to not use index_sequences and all that fun stuff to make this fast, but that now requires default-constructibility.
Anyway, sqlite::field explicitly states it's for query results, while sqlite::value is for usage in internal APIs. This is holding the results of a query, so why is it sqlite::value the one that looks to be supported?
The logic was that the static_result strips the column/row/table information and just gives you a value. The equivalent here would be the value instead of the field. But if I add optional (and maybe variant) support I should probably remove that.
On Tue, Nov 19, 2024 at 9:37 AM Klemens Morgenstern via Boost <boost@lists.boost.org> wrote:
On Tue, Nov 19, 2024 at 4:30 PM Ruben Perez <rubenperez038@gmail.com> wrote:
What's the best way to specify a nullable field in the static interface? std::optional<T> and boost::optional<T> don't seem to work, nor does having a sqlite::field member. sqlite::value would work. I should add optional support though.
I use optional extensively in my SQLite and PostgreSQL wrappers. And on result-sets, trying to read / access a NULL into a non-optional value throws. I.e. the client must expect the NULL and code accordingly. Not sure what the current situation is given Ruben's question, but support for optional is important to my use cases at least, FWIW. --DD
Am 19.11.2024 um 11:51 schrieb Dominique Devienne via Boost:
I use optional extensively in my SQLite and PostgreSQL wrappers. And on result-sets, trying to read / access a NULL into a non-optional value throws. I.e. the client must expect the NULL and code accordingly.
i would prefer a default check-for-null behavior but the possiblity to ommit the checks if wanted
On 13 Nov 2024, at 14:30, Richard Hodges via Boost <boost@lists.boost.org> wrote: The Boost formal review of the Boost SQLITE library starts *TODAY*, taking place from November 13th, 2024 to November 22nd, 2024 (inclusive).
This is not a review, just documentation errata. Overall, pretty good. 1) This library provides a simple C++ sqlite library. sqlite -> SQLite, https://www.sqlite.org <https://www.sqlite.org/> 2) "link against boost_sqlite for embedding it" Not clear what embedding means here, "dynamically linking to it" or maybe you mean statically? 3) conn.prepare("insert into author (first_name, last_name) values (?1, ?2), (?3, ?4), (?5, ?6), (?7, ?8)") .execute({"vinnie", "falco", "richard", "hodges", "ruben", "perez", "peter", "dimov"}); Bad style for SQL? I would simplify: auto st = conn.prepare("insert into author ... st.execute( ... st.execute( ... st.execute( ... Incidentally, what is the difference, if any, between placeholders beginning with ? and $ Also, why the quotes in insert into library (\"name\", author Is name a reserved keyword in sqlite? 4) in the tuple example: std::cout << std::get<0>(q) << " authored " << std::get<0>(q) << std::endl; The second one should be std::get<1> ? 5) "a custom aggregate function" This is fantastic functionality, is it a wrapper for sqlite-native sqlite3_create_window_function? Perhaps an example creating a scalar sql function should go before it? 6) Someone already pointed this out, it is confusing that sqlite recognizes just five types but this API has eight very different-looking ones. Some explanation is due, presumably about how boost_sqlite decides to map types? 7) In the vtables section "module" is mentioned but does it refer to something entrirely different from the next section, also called modules? This one is presumably a DLL of some sort, is it persistent between program invokations? Also a "prototype" is mentioned but unclear what that refers to? 8) Is it true that create_scalar_function can be used with or without creating a module? 7) Library Comparisons Here the author deserves a commendation, as this provides a short overview of the state-of-the-art in this field and raison d'etre. Should be a required topic for every candidate library! Looking forward to actually doing something useful with this! Cheeers, Kostas
Hi all, This is my review of the proposed Boost.Sqlite. First of all, thanks Klemens for submitting the library, and Richard for managing the review.
- Will the library bring additional out-of-the-box utility to Boost?
I think it will. SQLite is very popular, and many people will likely benefit from the library.
- Did you try to use the library? With which compiler(s)? Did you have any problems?
I had two use cases in mind: regular SQL use, and virtual tables as a way to expose in-memory data structures. I've implemented both. My experiences with them follow. To try regular SQL use, I adapted an existing example I used to have in Boost.MySQL. It simulates an order management system for an online store, with a command-line interface. It's intentionally simplistic. I know that SQLite is more targeted towards resource-constrained systems, rather than web applications, but it's been enough to have a taste of how using the API looks like. For reference, the resulting code is here: https://gist.github.com/anarthal/40c018cc4d133d0c9a082814d99d2a7a I've used connection::prepare, connection::query, connection::execute, statement, static_resultset and transaction. I've found the API mostly satisfactory. Some gotchas: 1. I've found myself running into lifetime issues with code that looked well-formed. At one point, I had the following: sqlite::static_resultset<order_with_items> get_order_with_items(std::int64_t order_id) { sqlite::statement stmt = conn.prepare("SELECT ..."); return conn.execute<order_with_items>({order_id}); } This is undefined behavior (as stated in the docs), and needs to be rewritten as: sqlite::static_resultset<order_with_items> get_order_with_items(std::int64_t order_id) { return conn.prepare("SELECT ...") .execute<order_with_items>({order_id}); } The problem being that both sqlite::static_resultset and sqlite::statement are implemented in terms of a sqlite3_statement*. The second snippet makes the static_resultset own the statement handle, while the first version makes it a view. This problem will probably appear much more frequently if you're using error codes instead of exceptions. IMO the problem arises due to a mismatch between the SQLite and the Boost.Sqlite object model regarding statements and resultsets. Resultsets don't exist in the C API, and thus may become problematic in the C++ API. Making a type sometimes owning and sometimes non-owning can be misleading. As a quick idea (I haven't implemented it), you may have a reader class, with a similar interface to the current resultset, that allows iterating over the rows, but never takes ownership of the statement. For example: auto stmt = conn.prepare("SELECT ..."); stmt.execute({order_id}); for (const order& ord: stmt.reader<order>()) { // } You can probably make it a one-liner if you make execute return a reference to the statement. The main point would be making statement always owning, and reader/resultset always a view. 2. I've found it impossible to use the static interface with fields that may be NULL. I've tried the following: a. Using std::optional<T> and boost::optional<T> - these are not implemented yet. b. Using sqlite::value is documented to work, but doesn't. The author has stated that this is a bug. c. Using sqlite::field doesn't work either. I've hot-patched the library to include the relevant tag_invoke overload to make sqlite::field work. The tag_invoke mechanism looks like a private interface, so as a regular user I would have not been able to make use of it. Nullable fields are very common, and should likely be supported by the static interface. 3. I tried using std::int64_t for my 64-bit integer fields and it didn't work. You need to use sqlite3_int64. In my machine, sqlite3_int64 is defined as a long long, while std::int64_t is defined to be a long. I found this annoying, since it makes database implementation types leak into business logic types. The compiler errors when using the static interface are not as informative as I'd have liked. When using a struct with an unsupported type, it'd be very helpful if the offending type name appeared first in the compiler message - diagnosing the sqlite3_int64 problem would have been much easier. For example, these are the first lines that get emitted when trying to use a struct with a wchar_t field (unsupported) in the Boost.MySQL static interface: /opt/boost-1.87.0-b1-rc2/include/boost/mysql/detail/typing/row_traits.hpp:172:13: error: static assertion failed due to requirement 'is_readable_field<wchar_t>::value': You're trying to use an unsupported field type in a row type. Review your row type definitions. 172 | is_readable_field<T>::value, | ^~~~~~~~~~~~~~~~~~~~~~~~~~~ 4. In the static interface, type mismatches are silent. If one of your field types doesn't match what your query returns, the library doesn't emit any error and silently sets the field to its default value. While this matches what the sqlite3_column_xxxx functions do, I think it'd be more helpful to emit an error. Such type mismatches are likely due to programming errors, and erroring helps to detect them soon. For instance, Boost.MySQL does this by checking metadata (you can probably use sqlite3_column_type to perform the check). My second use case involves exposing in-memory data structures to foreign processes. In one of my previous jobs, we had a high-severity bug due to one of such tables getting stuck with incorrect data. Diagnosing the problem was quite involved. A tool that made it easy to inspect the data in such tables could have been a reasonable measure after the bugfix. I have a similar data structure in my servertech chat project (https://github.com/anarthal/servertech-chat/blob/master/server/src/services/...), so I thought on adapting the multi_index.cpp example to suit my needs. I have no prior experience with SQLite virtual tables, but I've been able to learn about them using SQLite's official documentation. Adapting multi_index.cpp has been challenging, though, because: a. Virtual tables are inherently complex. b. There is no discussion page on virtual tables in the proposed library. c. The provided examples are inherently complex and don't have many comments. As a result, I've found myself reverse-engineering the library and examples, which is not very rewarding. I was under the wrong impression that loading the module in one process would make it immediately available to other processes loading the database file onto which it was loaded. This is my fault, as there is no way for SQLite to do this. This implies that, once I have my module, I need to write a protocol to accept and execute SQLite queries (e.g. by using a UNIX socket). Unfortunately, the overall architecture is too complex and has security implications that need to be addressed, and thus would have never gone into production in the company I used to work for. This left me questioning whether using virtual tables to expose in-memory data structures to SQLite is really strong. If I'm not mistaken, the library has 3 examples on this, so I'm likely missing something. I think a good motivation for these three examples would be very valuable - that is, some comments stating what real world situations would require me to write code like the one in the examples. I'm actually not convinced of the value added by the library in terms of virtual tables. My impression is that providing tested, robust virtual table implementations for concrete use cases would add more value than what the current infrastructure offers. In other words, is writing virtual table implementations really something that happens in day to day development? I feel that the answer to this question is no, so I find it surprising that all examples except one are about virtual tables. Take this with perspective, though, as I'm no expert in this topic.
- Will the choice of API abstraction model ease the development of software that must talk to a SQLITE database?
The API simplifies regular SQL usage, as explained above. A couple of comments: 1. The virtual table infrastructure uses a macro (BOOST_SQLITE_NO_VIRTUAL) to conditionally make functions virtual. I think this is a potential problem in the long-run. If virtualization is not required, appropriate compile-time checks should be performed, instead. 2. sqlite::transaction features a throwing destructor, which I find surprising.
- What is your evaluation of the implementation?
I've only peaked at it in particular sites, and have the following comments: 1. transaction::commit throwing and non-throwing overloads seem to have different behavior on error. One will call rollback() and the other one won't. This case doesn't look to be covered by unit tests. 2. The virtual table infrastructure may be invoking undefined behavior by violating strict aliasing, casting sqlite3_value* into sqlite::value via a reinterpret_cast. Some conversations have happened in the Slack workspace about it, with conflicting opinions whether this is actual undefined behavior or not. 3. Compiling with -Wall generates a lot of warnings, most of which are located in headers. I'd advise building with -Wall and -Werror in the CI to clean up these. 4. Passing incorrect parameter types to .execute() and .query() doesn't yield clean error messages. It would be beneficial implementing concepts (or a static_assert) for better messages. As I mentioned earlier, this also applies for the static interface. 5. Compiler coverage in CI looks improvable. The asan job seems commented out, and I can't see any build targeting C++20 or 23. 6. If BOOST_SQLITE_NO_VIRTUAL is to stay, it needs to be documented and tested.
- What is your evaluation of the documentation?
In my opinion, the documentation is insufficient for the level required by a Boost library. I've found the virtual table functionality specially involved because of this. Some of my major comments: 1. All supported, public functionality should be documented, both in the reference pages and in the discussion. This is not the case for most of the library: virtual tables, hooks, json, metadata, mutexes, transactions, blobs and memory management (sqlite::memory_tag) are not explained in the discussion. 2. Excepting virtual tables, the rest of the functionality isn't explained using examples, either. 3. Some Doxygen comments seem outdated, referencing functions that have been removed or renamed. See the bottom of this email for a list of what I found. 4. Many Doxygen comments don't provide the information I'd expect to find, like exception safety, lifetime rules, or the underlying sqlite3_xxx functions they call.
- How much effort did you put into your evaluation? A glance? A quick reading? In-depth study?
I've spent around 12h doing inspecting the library, building the examples, writing the online store simulator, adapting the multi_index.cpp example and writing this review.
- Are you knowledgeable about the problem domain?
I've had mild SQLite experience in the past, mainly in prototypes. I didn't have prior experience with virtual tables. I have experience with SQL in general, and am the author of Boost.MySQL.
Final decision
Unfortunately, my final recommendation is to REJECT Boost.Sqlite in its current form. I think the concept is interesting and could benefit many people, but the library still requires some work before it gets into Boost. The documentation needs a lot of work. Getting some field experience would also be beneficial. Thanks again Klemens and Richard for your effort. I'd love to see the library being proposed again in the future. Typos and documentation comments: * The documentation should state which C++ standards and compilers are tested and supported. * The examples on virtual tables require step-by-step comments, since they implement non-trivial logic. * The examples on virtual tables need a rationale. That is, as a user, when would I find myself writing code like this? * The discussion seems to jump to advanced details too quickly. There are very few docs on everyday tasks. * The following types don't have any documentation at all but appear in the public namespace. They should be either documented or made private: allocator, unique_ptr, msize, make_unique, set_variant_result, like, glob, icmp, * Functionality that allocates using sqlite3-specific memory allocations should likely state that fact it in the reference. * Intro: "This library provides a simple C++ sqlite library." Do you mean "simple C++ SQLite wrapper library"? * Discussion: "Prepared statements can also be used multiple time and used with named parameters instead of indexed." should be "multiple times" and "named parameters instead of indexed ones." * Discussion: "The result of a query is a field type,": this is technically not true - you get a resultset, which yields rows that have fields. * Discussion: "Fields & values can have subtypes, while parameter to prepared statements do not have thos associated.": should be "parameters" and "do not have associated subtypes". * In general: avoid & as a way to state "and" * vtable.hpp: "@tparam T The implementation type of the module. It must inherit": I don't know what this phrase means. The type requirements for T likely need a page on their own. * vtable.hpp: "It's lifetime is managed by the database.": should be "Its" * vtable.hpp: "@param The requirements for `module`.": this creates a bogus parameter entry, looks like a leftover * field.hpp and value.hpp: "Returns the value as text, i.e. a string_view. Note that this value may be invalidated`.": this is not enough information. It should list when it gets invalidated. * statement.hpp: "The handle is shared between the statement & resultset. The statemens need to be kept alive.": should be "statement" and "needs". * statement.hpp: "This can be a map, a vector or a stuple": should be "tuple" * vtable.hpp (create function): "The instance_type gets used & managed" references an instance_type that does not exist (presumably you meant table_type). * vtable.hpp: "Destroy the storage = this function needs to be present for non eponymous tables": syntax error * vtable.hpp: "index info used by the find_index function": no find_index function exists, you presumably mean best_index * cstring_ref.hpp: doesn't have any function documented * connection.hpp: "Perform a query without parametert, It execute a multiple statement.": should be "parameter" * collation.hpp: "a case insensitive string omparison, e.g. from boost.urls": should be "comparison" * README.md: "auto r = q.current();''" there's an extra '' there. I'd advise to run documentation snippets to prevent such errors * multi_index.cpp: the create table statement uses the "url" name, which doesn't harm but is misleading * multi_index.cpp: I'd advise to mark overriding methods with the "override" keyword * multi_index.cpp: "static_assert(sizeof(const_iterator) <= sizeof(sqlite3_int64), "");": why is the static_assert needed? Please add a comment documenting it * multi_index.cpp and ordered_map.cpp: declares an "enum indices" that presumably was thought to make bit fiddling more clear, but don't seem to be used, increasing user confusion. On Wed, 13 Nov 2024 at 13:31, Richard Hodges via Boost <boost@lists.boost.org> wrote:
Dear All,
Surprise!
The Boost formal review of the Boost SQLITE library starts *TODAY*, taking place
from November 13th, 2024 to November 22nd, 2024 (inclusive).
I apologise profusely for springing this on you without prior warning. The error is entirely mine. I am extending the period by one day to compensate.
The library is authored by Klemens Morgenstern (@klemens-morgenstern in the CppLang slack).
Documentation: https://klemens.dev/sqlite/ <https://anarthal.github.io/mysql/index.html> Source: https://github.com/klemens-morgenstern/sqlite <https://github.com/anarthal/mysql/>
From the documentation:
boost.sqlite is a simple to use C++ sqlite library. It provides a modern interface using facilities like error_code, views (e.g. for blobs) and the ability to use boost.describe or boost.pfr for parameterised queries.
Supported features include:
- typed queries - prepared statements - json support - custom functions (scalar, aggregate, windows) - event hooks - virtual tables
SQLite provides an excellent C-API, so this library does not attempt to hide, but to augment it.
Please provide in your review information you think is valuable to explain your choice to ACCEPT or REJECT including SQLITE as a Boost library. Please be explicit about your decision (ACCEPT or REJECT).
Some other questions you might want to consider answering:
- Will the library bring additional out-of-the-box utility to Boost? - What is your evaluation of the implementation? - What is your evaluation of the documentation? - Will the choice of API abstraction model ease the development of software that must talk to a SQLITE database? - Are there any immediate improvements that could be made after acceptance, if acceptance should happen? - Did you try to use the library? With which compiler(s)? Did you have any problems? - How much effort did you put into your evaluation? A glance? A quick reading? In-depth study? - Are you knowledgeable about the problem domain?
More information about the Boost Formal Review Process can be found at: http://www.boost.org/community/reviews.html
The review is open to anyone who is prepared to put in the work of evaluating and reviewing the library. Prior experience in contributing to Boost reviews is not a requirement.
Thank you for your efforts in the Boost community. They are very much appreciated.
Richard Hodges - review manager of the proposed Boost.SQLITE library
Klemens is often available on CppLang Slack and of course by email should you require any clarification not covered by the documentation, as am I.
_______________________________________________ Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost
Ruben Perez wrote on Tuesday, November 19, 2024 1:25 PM ... snip ...
In other words, is writing virtual table implementations really something that happens in day to day development? I feel that the answer to this question is no, so I find it surprising that all examples except one are about virtual tables. Take this with perspective, though, as I'm no expert in this topic.
Not C++, but I use virtual tables in Python to expose Numpy memory structures so users can query them directly. It's quite handy. Erik ---------------------------------------------------------------------- This message, and any attachment(s), is for the intended recipient(s) only, may contain information that is privileged, confidential and/or proprietary and subject to important terms and conditions available at http://www.bankofamerica.com/electronic-disclaimer. If you are not the intended recipient, please delete this message. For more information about how Bank of America protects your privacy, including specific rights that may apply, please visit the following pages: https://business.bofa.com/en-us/content/global-privacy-notices.html (which includes global privacy notices) and https://www.bankofamerica.com/security-center/privacy-overview/ (which includes US State specific privacy notices such as the http://www.bankofamerica.com/ccpa-notice).
On Tue, 19 Nov 2024, 19:46 Nelson, Erik - 2 via Boost, < boost@lists.boost.org> wrote:
Ruben Perez wrote on Tuesday, November 19, 2024 1:25 PM
... snip ...
In other words, is writing virtual table implementations really something that happens in day to day development? I feel that the answer to this question is no, so I find it surprising that all examples except one are about virtual tables. Take this with perspective, though, as I'm no expert in this topic.
Not C++, but I use virtual tables in Python to expose Numpy memory structures so users can query them directly. It's quite handy.
Erik
This is pretty interesting, thanks for sharing. Did you get to write the virtual table implementation, or is it part of an already existing package? Regards, Ruben.
Ruben Perez wrote on Tuesday, November 19, 2024 3:52 PM On Tue, 19 Nov 2024, 19:46 Nelson, Erik - 2 via Boost, <boost@lists.boost.org<mailto:boost@lists.boost.org>> wrote: Ruben Perez wrote on Tuesday, November 19, 2024 1:25 PM ... snip ...
In other words, is writing virtual table implementations really something that happens in day to day development? I feel that the answer to this question is no, so I find it surprising that all examples except one are about virtual tables. Take this with perspective, though, as I'm no expert in this topic.
Not C++, but I use virtual tables in Python to expose Numpy memory structures so users can query them directly. It's quite handy. Erik
This is pretty interesting, thanks for sharing. Did you get to write the virtual table implementation, or is it part of an already existing package?
Our team wrote it, I haven’t seen anything comparable. It uses Numba to jit-compile the python into C-callable functions. Then pointers to those can be passed to the sqlite3 API. We basically extended the concepts pioneered in the numbsql project (https://github.com/cpcloud/numbsql) to the sqlite3 virtual table API. Erik ---------------------------------------------------------------------- This message, and any attachment(s), is for the intended recipient(s) only, may contain information that is privileged, confidential and/or proprietary and subject to important terms and conditions available at http://www.bankofamerica.com/electronic-disclaimer. If you are not the intended recipient, please delete this message. For more information about how Bank of America protects your privacy, including specific rights that may apply, please visit the following pages: https://business.bofa.com/en-us/content/global-privacy-notices.html (which includes global privacy notices) and https://www.bankofamerica.com/security-center/privacy-overview/ (which includes US State specific privacy notices such as the http://www.bankofamerica.com/ccpa-notice).
On Tue, 19 Nov 2024, 19:24 Ruben Perez, <rubenperez038@gmail.com> wrote:
Hi all,
This is my review of the proposed Boost.Sqlite. First of all, thanks Klemens for submitting the library, and Richard for managing the review.
I've forgotten to disclose my affiliation with the C++ Alliance. Thanks, Ruben.
No dia 19 de nov. de 2024, às 21:51, Ruben Perez via Boost <boost@lists.boost.org> escreveu:
On Tue, 19 Nov 2024, 19:24 Ruben Perez, <rubenperez038@gmail.com> wrote:
Hi all,
This is my review of the proposed Boost.Sqlite. First of all, thanks Klemens for submitting the library, and Richard for managing the review.
I've forgotten to disclose my affiliation with the C++ Alliance.
I don’t think this is needed here as there’s no extant conflict of interest? Joaquin M Lopez Munoz
On Wed, Nov 20, 2024 at 2:25 AM Ruben Perez <rubenperez038@gmail.com> wrote:
Hi all,
This is my review of the proposed Boost.Sqlite. First of all, thanks Klemens for submitting the library, and Richard for managing the review.
Thank you for investing the time to write a review.
- Will the library bring additional out-of-the-box utility to Boost?
I think it will. SQLite is very popular, and many people will likely benefit from the library.
- Did you try to use the library? With which compiler(s)? Did you have any problems?
I had two use cases in mind: regular SQL use, and virtual tables as a way to expose in-memory data structures. I've implemented both. My experiences with them follow.
To try regular SQL use, I adapted an existing example I used to have in Boost.MySQL. It simulates an order management system for an online store, with a command-line interface. It's intentionally simplistic. I know that SQLite is more targeted towards resource-constrained systems, rather than web applications, but it's been enough to have a taste of how using the API looks like. For reference, the resulting code is here: https://gist.github.com/anarthal/40c018cc4d133d0c9a082814d99d2a7a
I've used connection::prepare, connection::query, connection::execute, statement, static_resultset and transaction. I've found the API mostly satisfactory. Some gotchas:
1. I've found myself running into lifetime issues with code that looked well-formed. At one point, I had the following:
sqlite::static_resultset<order_with_items> get_order_with_items(std::int64_t order_id) { sqlite::statement stmt = conn.prepare("SELECT ..."); return conn.execute<order_with_items>({order_id}); }
This is undefined behavior (as stated in the docs), and needs to be rewritten as:
sqlite::static_resultset<order_with_items> get_order_with_items(std::int64_t order_id) { return conn.prepare("SELECT ...") .execute<order_with_items>({order_id}); }
The problem being that both sqlite::static_resultset and sqlite::statement are implemented in terms of a sqlite3_statement*. The second snippet makes the static_resultset own the statement handle, while the first version makes it a view. This problem will probably appear much more frequently if you're using error codes instead of exceptions.
IMO the problem arises due to a mismatch between the SQLite and the Boost.Sqlite object model regarding statements and resultsets. Resultsets don't exist in the C API, and thus may become problematic in the C++ API. Making a type sometimes owning and sometimes non-owning can be misleading.
Do you think this could be solved with ref-qualified overloads, i.e. .execute() && would transfer ownership, while .execute() & would not? That would seem intuitive to me. <snip>
4. In the static interface, type mismatches are silent. If one of your field types doesn't match what your query returns, the library doesn't emit any error and silently sets the field to its default value. While this matches what the sqlite3_column_xxxx functions do, I think it'd be more helpful to emit an error. Such type mismatches are likely due to programming errors, and erroring helps to detect them soon. For instance, Boost.MySQL does this by checking metadata (you can probably use sqlite3_column_type to perform the check).
Note that this reflects sqlite's behaviour, its tables aren't strictly typed. That is, a column might be defined as INT, but a user could insert a "foobar" in it. And then it's going to be a string. While odd, that's not a programming, but user error. I think adding a strict mode for the static_resultset would be the way to go, i.e. an opt in similar to strict tables in sqlite. sqlite only added type checking as an option in 2021 with strict tables. <snip>
- Will the choice of API abstraction model ease the development of software that must talk to a SQLITE database?
The API simplifies regular SQL usage, as explained above. A couple of comments:
1. The virtual table infrastructure uses a macro (BOOST_SQLITE_NO_VIRTUAL) to conditionally make functions virtual. I think this is a potential problem in the long-run. If virtualization is not required, appropriate compile-time checks should be performed, instead. 2. sqlite::transaction features a throwing destructor, which I find surprising.
That's for backwards compatibility, ROLLBACK won't fail these days, but it did in older versions. So in practice it won't throw, unless you're using some ancient sqlite instance. (https://www.sqlite.org/lang_transaction.html)
- What is your evaluation of the implementation?
I've only peaked at it in particular sites, and have the following comments:
1. transaction::commit throwing and non-throwing overloads seem to have different behavior on error. One will call rollback() and the other one won't. This case doesn't look to be covered by unit tests. 2. The virtual table infrastructure may be invoking undefined behavior by violating strict aliasing, casting sqlite3_value* into sqlite::value via a reinterpret_cast. Some conversations have happened in the Slack workspace about it, with conflicting opinions whether this is actual undefined behavior or not.
It's not, the sqlite::value class has a standard-layout class and the element it's cast from is the first in the class. https://eel.is/c++draft/basic.compound#5.3 <snip>
- How much effort did you put into your evaluation? A glance? A quick reading? In-depth study?
I've spent around 12h doing inspecting the library, building the examples, writing the online store simulator, adapting the multi_index.cpp example and writing this review.
I really appreciate the time invested, thank you!
- Are you knowledgeable about the problem domain?
I've had mild SQLite experience in the past, mainly in prototypes. I didn't have prior experience with virtual tables. I have experience with SQL in general, and am the author of Boost.MySQL.
Final decision
Unfortunately, my final recommendation is to REJECT Boost.Sqlite in its current form. I think the concept is interesting and could benefit many people, but the library still requires some work before it gets into Boost. The documentation needs a lot of work. Getting some field experience would also be beneficial.
Thanks again Klemens and Richard for your effort. I'd love to see the library being proposed again in the future.
1. I've found myself running into lifetime issues with code that looked well-formed. At one point, I had the following:
sqlite::static_resultset<order_with_items> get_order_with_items(std::int64_t order_id) { sqlite::statement stmt = conn.prepare("SELECT ..."); return conn.execute<order_with_items>({order_id}); }
This is undefined behavior (as stated in the docs), and needs to be rewritten as:
sqlite::static_resultset<order_with_items> get_order_with_items(std::int64_t order_id) { return conn.prepare("SELECT ...") .execute<order_with_items>({order_id}); }
The problem being that both sqlite::static_resultset and sqlite::statement are implemented in terms of a sqlite3_statement*. The second snippet makes the static_resultset own the statement handle, while the first version makes it a view. This problem will probably appear much more frequently if you're using error codes instead of exceptions.
IMO the problem arises due to a mismatch between the SQLite and the Boost.Sqlite object model regarding statements and resultsets. Resultsets don't exist in the C API, and thus may become problematic in the C++ API. Making a type sometimes owning and sometimes non-owning can be misleading.
Do you think this could be solved with ref-qualified overloads, i.e. .execute() && would transfer ownership, while .execute() & would not? That would seem intuitive to me.
Isn't this what you have right now?
4. In the static interface, type mismatches are silent. If one of your field types doesn't match what your query returns, the library doesn't emit any error and silently sets the field to its default value. While this matches what the sqlite3_column_xxxx functions do, I think it'd be more helpful to emit an error. Such type mismatches are likely due to programming errors, and erroring helps to detect them soon. For instance, Boost.MySQL does this by checking metadata (you can probably use sqlite3_column_type to perform the check).
Note that this reflects sqlite's behaviour, its tables aren't strictly typed. That is, a column might be defined as INT, but a user could insert a "foobar" in it. And then it's going to be a string. While odd, that's not a programming, but user error.
I think adding a strict mode for the static_resultset would be the way to go, i.e. an opt in similar to strict tables in sqlite.
sqlite only added type checking as an option in 2021 with strict tables.
You got me in this one - I didn't know this was legal. I still think this should be an error. If you actually expect different types in a single field, an appropriate representation (like sqlite::field or a variant) should be used.
2. sqlite::transaction features a throwing destructor, which I find surprising.
That's for backwards compatibility, ROLLBACK won't fail these days, but it did in older versions. So in practice it won't throw, unless you're using some ancient sqlite instance.
If that's the case, I think it's best to just don't use a throwing destructor, and don't communicate the error anyway.
2. The virtual table infrastructure may be invoking undefined behavior by violating strict aliasing, casting sqlite3_value* into sqlite::value via a reinterpret_cast. Some conversations have happened in the Slack workspace about it, with conflicting opinions whether this is actual undefined behavior or not.
It's not, the sqlite::value class has a standard-layout class and the element it's cast from is the first in the class.
I know the cast is well-formed, but the target object doesn't technically exist (as opposed to a member subobject, which starts existing when the parent object is created). Thanks, Ruben.
1. I've found myself running into lifetime issues with code that looked well-formed. At one point, I had the following:
sqlite::static_resultset<order_with_items> get_order_with_items(std::int64_t order_id) { sqlite::statement stmt = conn.prepare("SELECT ..."); return conn.execute<order_with_items>({order_id}); }
This is undefined behavior (as stated in the docs), and needs to be rewritten as:
sqlite::static_resultset<order_with_items> get_order_with_items(std::int64_t order_id) { return conn.prepare("SELECT ...") .execute<order_with_items>({order_id}); } Is the 1st example supposed to be `return stmt.execute<...`?
Answered by Klemens as:
Do you think this could be solved with ref-qualified overloads, i.e. .execute() && would transfer ownership, while .execute() & would not? That would seem intuitive to me. From what the examples look like this already seems to be the case. Otherwise there would be no difference between the 2. Or am I missing anything?
I also agree that those 2 code examples look like they would be the same behavior and people would likely just use the first and expect it to work. I.e. from the code alone it is not clear that there is an issue and it can be easily overlooked in code reviews. Is it possible to use shared ownership here, such that the resultset in example 1 remains valid after the return? If shared_ownership is not possible, could a weak ownership we used? Example 1 would lead a segmentation fault right now. If the library could detect that the `sqlite3_stmt`was deleted at this point and report that it would make it easer to understand. This is especially dangerous as the destructor of the resultset triggers the segfault too.
On Thu, Nov 21, 2024 at 5:19 PM Alexander Grund via Boost < boost@lists.boost.org> wrote:
1. I've found myself running into lifetime issues with code that looked well-formed. At one point, I had the following:
sqlite::static_resultset<order_with_items> get_order_with_items(std::int64_t order_id) { sqlite::statement stmt = conn.prepare("SELECT ..."); return conn.execute<order_with_items>({order_id}); }
This is undefined behavior (as stated in the docs), and needs to be rewritten as:
sqlite::static_resultset<order_with_items> get_order_with_items(std::int64_t order_id) { return conn.prepare("SELECT ...") .execute<order_with_items>({order_id}); } Is the 1st example supposed to be `return stmt.execute<...`?
Answered by Klemens as:
Do you think this could be solved with ref-qualified overloads, i.e. .execute() && would transfer ownership, while .execute() & would not? That would seem intuitive to me. From what the examples look like this already seems to be the case. Otherwise there would be no difference between the 2. Or am I missing anything?
It is - I shouldn't respond to emails as 4:30 a.m. and get confused by typos in the code examples.
I also agree that those 2 code examples look like they would be the same behavior and people would likely just use the first and expect it to work. I.e. from the code alone it is not clear that there is an issue and it can be easily overlooked in code reviews. Is it possible to use shared ownership here, such that the resultset in example 1 remains valid after the return?
If shared_ownership is not possible, could a weak ownership we used? Example 1 would lead a segmentation fault right now. If the library could detect that the `sqlite3_stmt`was deleted at this point and report that it would make it easer to understand. This is especially dangerous as the destructor of the resultset triggers the segfault too.
Neither is possibly without overhead. I could potentially make Example 2 a single function call,prepare_and_execute.
Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost
On Wed, Nov 13, 2024 at 4:31 AM Richard Hodges via Boost <boost@lists.boost.org> wrote:
Question for the author: * Who has tried integrating this library into their existing code, before the review period? Thanks
# boost_sqlite review Please accept my apologies for the poor formatting. And a bit thank you to everyone involved in this process. I have long wished for a sqlite wrapper with critical mass. ## Author's Background I am a professional C++ software developer. I have been a professional user and proponent of Boost libraries since the late 2000s. I have been using SQLite for more than 10 years and at one time wrote my own C++ wrapper. ## Initial Thoughts and Use Case At the time I saw the announcement for this review, I was actively evaluating libraries for a project I was (and continue to be) working on. I successfully used the boost_sqlite library in a limited capacity (create, insert, and pragmas only for on disk and in memory DBs). My first impression of the library is that it's extremely lightweight. This simplicity has value but also leaves the end user reinventing the wheel for certain repetitive operations. For example, using class enums for pragma get and set moves defect finding from runtime (bad) to compile time (better!). My second thought was how painful it was to include in my professional project. Our legacy software does not build Boost.json and, while it was trivial to add it for my sandbox app, it is an unnecessary extra dependency. ## Design Evaluation Simple, straight forward, and intuitive. I believe it meets my needs. ### Wishlist Here are some wishlist items I have. They may be inappropriate for this library and boost in general; however, they are still on my wishlist. #### connection It would be great if connection had a simple enum so I could do this: ```cpp boost::sqlite::connection mydb(boost::sqlite::memory); ``` Also, given C++17 `std::fstream` takes `std::filesystem::path`, it would be nice if `connection()` et all did as well. And I wouldn't complain if `boost::filesystem::path` was supported as well. ## Implementation Evaluation I reviewed this library as a user. ## Documentation Evaluation The readme.md file was adequate for me to quickly build a simple SQLite DB application. ## Potential Usefulness This is a useful library. I intend to incorporate this into professional projects as soon as it becomes available in a stable boost release. ## Personal Usage I used the following compiler in my development: gcc (Ubuntu 11.4.0-1ubuntu1~22.04) 11.4.0 I included Boost and Boost.sqlite using CPM.cmake (fetchcontent). It was a trivial operation to include and link. ## Final Thoughts For the past few years I've lamented the lack of a Modern C++ SQLite wrapper with a critical mass of user backing. This library has the potential to fill that gap. I would like to see this library build without the sin dependency. With or without the build dependency change, this library should be accepted into Boost. Scott Bailey On Wed, Nov 13, 2024 at 6:30 AM Richard Hodges via Boost < boost@lists.boost.org> wrote:
Dear All,
Surprise!
The Boost formal review of the Boost SQLITE library starts *TODAY*, taking place
from November 13th, 2024 to November 22nd, 2024 (inclusive).
I apologise profusely for springing this on you without prior warning. The error is entirely mine. I am extending the period by one day to compensate.
The library is authored by Klemens Morgenstern (@klemens-morgenstern in the CppLang slack).
Documentation: https://klemens.dev/sqlite/ <https://anarthal.github.io/mysql/index.html> Source: https://github.com/klemens-morgenstern/sqlite <https://github.com/anarthal/mysql/>
From the documentation:
boost.sqlite is a simple to use C++ sqlite library. It provides a modern interface using facilities like error_code, views (e.g. for blobs) and the ability to use boost.describe or boost.pfr for parameterised queries.
Supported features include:
- typed queries - prepared statements - json support - custom functions (scalar, aggregate, windows) - event hooks - virtual tables
SQLite provides an excellent C-API, so this library does not attempt to hide, but to augment it.
Please provide in your review information you think is valuable to explain your choice to ACCEPT or REJECT including SQLITE as a Boost library. Please be explicit about your decision (ACCEPT or REJECT).
Some other questions you might want to consider answering:
- Will the library bring additional out-of-the-box utility to Boost? - What is your evaluation of the implementation? - What is your evaluation of the documentation? - Will the choice of API abstraction model ease the development of software that must talk to a SQLITE database? - Are there any immediate improvements that could be made after acceptance, if acceptance should happen? - Did you try to use the library? With which compiler(s)? Did you have any problems? - How much effort did you put into your evaluation? A glance? A quick reading? In-depth study? - Are you knowledgeable about the problem domain?
More information about the Boost Formal Review Process can be found at: http://www.boost.org/community/reviews.html
The review is open to anyone who is prepared to put in the work of evaluating and reviewing the library. Prior experience in contributing to Boost reviews is not a requirement.
Thank you for your efforts in the Boost community. They are very much appreciated.
Richard Hodges - review manager of the proposed Boost.SQLITE library
Klemens is often available on CppLang Slack and of course by email should you require any clarification not covered by the documentation, as am I.
_______________________________________________ Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost
On Fri, Nov 22, 2024 at 7:40 AM Scott Bailey via Boost < boost@lists.boost.org> wrote:
# boost_sqlite review
Please accept my apologies for the poor formatting. And a bit thank you to everyone involved in this process. I have long wished for a sqlite wrapper with critical mass.
Thank you for writing a review.
## Initial Thoughts and Use Case
At the time I saw the announcement for this review, I was actively evaluating libraries for a project I was (and continue to be) working on. I successfully used the boost_sqlite library in a limited capacity (create, insert, and pragmas only for on disk and in memory DBs).
My first impression of the library is that it's extremely lightweight. This simplicity has value but also leaves the end user reinventing the wheel for certain repetitive operations. For example, using class enums for pragma get and set moves defect finding from runtime (bad) to compile time (better!).
That seems like a reasonable addition.
My second thought was how painful it was to include in my professional project. Our legacy software does not build Boost.json and, while it was trivial to add it for my sandbox app, it is an unnecessary extra dependency.
You are the second person to bring this up. I think making json support a header only addition would solve the dependency issue.
## Design Evaluation
Simple, straight forward, and intuitive. I believe it meets my needs.
### Wishlist
Here are some wishlist items I have. They may be inappropriate for this library and boost in general; however, they are still on my wishlist.
#### connection
It would be great if connection had a simple enum so I could do this:
```cpp boost::sqlite::connection mydb(boost::sqlite::memory); ```
Also, given C++17 `std::fstream` takes `std::filesystem::path`, it would be nice if `connection()` et all did as well. And I wouldn't complain if `boost::filesystem::path` was supported as well.
I'll see if that can be added without including additional headers.
## Implementation Evaluation
I reviewed this library as a user.
## Documentation Evaluation
The readme.md file was adequate for me to quickly build a simple SQLite DB application.
## Potential Usefulness
This is a useful library. I intend to incorporate this into professional projects as soon as it becomes available in a stable boost release.
## Personal Usage
I used the following compiler in my development:
gcc (Ubuntu 11.4.0-1ubuntu1~22.04) 11.4.0
I included Boost and Boost.sqlite using CPM.cmake (fetchcontent). It was a trivial operation to include and link.
## Final Thoughts
For the past few years I've lamented the lack of a Modern C++ SQLite wrapper with a critical mass of user backing. This library has the potential to fill that gap.
I would like to see this library build without the sin dependency.
I assume you meant json here?
With or without the build dependency change, this library should be accepted into Boost.
Thank you.
On Fri, Nov 22, 2024 at 4:35 AM Klemens Morgenstern < klemensdavidmorgenstern@gmail.com> wrote:
On Fri, Nov 22, 2024 at 7:40 AM Scott Bailey via Boost < boost@lists.boost.org> wrote:
# boost_sqlite review
Please accept my apologies for the poor formatting. And a bit thank you to everyone involved in this process. I have long wished for a sqlite wrapper with critical mass.
Thank you for writing a review.
## Initial Thoughts and Use Case
At the time I saw the announcement for this review, I was actively evaluating libraries for a project I was (and continue to be) working on. I successfully used the boost_sqlite library in a limited capacity (create, insert, and pragmas only for on disk and in memory DBs).
My first impression of the library is that it's extremely lightweight. This simplicity has value but also leaves the end user reinventing the wheel for certain repetitive operations. For example, using class enums for pragma get and set moves defect finding from runtime (bad) to compile time (better!).
That seems like a reasonable addition.
My second thought was how painful it was to include in my professional project. Our legacy software does not build Boost.json and, while it was trivial to add it for my sandbox app, it is an unnecessary extra dependency.
You are the second person to bring this up. I think making json support a header only addition would solve the dependency issue.
Yes, please. Or simply not adding json files and dependencies if `BOOST_SQLITE_USE_JSON` or similar is "falsy".
## Design Evaluation
Simple, straight forward, and intuitive. I believe it meets my needs.
### Wishlist
Here are some wishlist items I have. They may be inappropriate for this library and boost in general; however, they are still on my wishlist.
#### connection
It would be great if connection had a simple enum so I could do this:
```cpp boost::sqlite::connection mydb(boost::sqlite::memory); ```
Also, given C++17 `std::fstream` takes `std::filesystem::path`, it would be nice if `connection()` et all did as well. And I wouldn't complain if `boost::filesystem::path` was supported as well.
I'll see if that can be added without including additional headers.
I suspect `boost::sqlite::memory` as a static member of const some-string-type could be easily added to move defect finding for in-memory-dbs from runtime to compile time. Adding `xxx::filesystem::path` can always be added in a future revision. Though `boost::iostream::file_descriptor` may have an example of how to do it correctly. But I'm not a Boost dev, just a user.
## Implementation Evaluation
I reviewed this library as a user.
## Documentation Evaluation
The readme.md file was adequate for me to quickly build a simple SQLite DB application.
## Potential Usefulness
This is a useful library. I intend to incorporate this into professional projects as soon as it becomes available in a stable boost release.
## Personal Usage
I used the following compiler in my development:
gcc (Ubuntu 11.4.0-1ubuntu1~22.04) 11.4.0
I included Boost and Boost.sqlite using CPM.cmake (fetchcontent). It was a trivial operation to include and link.
## Final Thoughts
For the past few years I've lamented the lack of a Modern C++ SQLite wrapper with a critical mass of user backing. This library has the potential to fill that gap.
I would like to see this library build without the sin dependency.
I assume you meant json here?
Yes, absolutely: json
With or without the build dependency change, this library should be accepted into Boost.
Thank you.
Dear all, It has been suggested to me that ending the review period prior to a weekend may not be the best timing. In hindsight I agree, and since I will be travelling next week, I propose to leave the period open until EOD Sunday 1st December. Please feel free to continue the review process. Apologies for the inconvenience, Sincerely, Richard On Wed, 13 Nov 2024 at 13:30, Richard Hodges <hodges.r@gmail.com> wrote:
Dear All,
Surprise!
The Boost formal review of the Boost SQLITE library starts *TODAY*, taking place
from November 13th, 2024 to November 22nd, 2024 (inclusive).
I apologise profusely for springing this on you without prior warning. The error is entirely mine. I am extending the period by one day to compensate.
The library is authored by Klemens Morgenstern (@klemens-morgenstern in the CppLang slack).
Documentation: https://klemens.dev/sqlite/ <https://anarthal.github.io/mysql/index.html> Source: https://github.com/klemens-morgenstern/sqlite <https://github.com/anarthal/mysql/>
From the documentation:
boost.sqlite is a simple to use C++ sqlite library. It provides a modern interface using facilities like error_code, views (e.g. for blobs) and the ability to use boost.describe or boost.pfr for parameterised queries.
Supported features include:
- typed queries - prepared statements - json support - custom functions (scalar, aggregate, windows) - event hooks - virtual tables
SQLite provides an excellent C-API, so this library does not attempt to hide, but to augment it.
Please provide in your review information you think is valuable to explain your choice to ACCEPT or REJECT including SQLITE as a Boost library. Please be explicit about your decision (ACCEPT or REJECT).
Some other questions you might want to consider answering:
- Will the library bring additional out-of-the-box utility to Boost? - What is your evaluation of the implementation? - What is your evaluation of the documentation? - Will the choice of API abstraction model ease the development of software that must talk to a SQLITE database? - Are there any immediate improvements that could be made after acceptance, if acceptance should happen? - Did you try to use the library? With which compiler(s)? Did you have any problems? - How much effort did you put into your evaluation? A glance? A quick reading? In-depth study? - Are you knowledgeable about the problem domain?
More information about the Boost Formal Review Process can be found at: http://www.boost.org/community/reviews.html
The review is open to anyone who is prepared to put in the work of evaluating and reviewing the library. Prior experience in contributing to Boost reviews is not a requirement.
Thank you for your efforts in the Boost community. They are very much appreciated.
Richard Hodges - review manager of the proposed Boost.SQLITE library
Klemens is often available on CppLang Slack and of course by email should you require any clarification not covered by the documentation, as am I.
On Sat, Nov 23, 2024 at 6:54 AM Richard Hodges via Boost < boost@lists.boost.org> wrote:
Dear all,
It has been suggested to me that ending the review period prior to a weekend may not be the best timing.
In hindsight I agree, and since I will be travelling next week, I propose to leave the period open until EOD Sunday 1st December.
Please feel free to continue the review process.
Since I already got great feedback, I started incorporating it into the library. If you are interested in the shape I think it should go after the review, here's the relevant PR https://github.com/klemens-morgenstern/sqlite/pull/4 and here's the new documentation: https://klemens.dev/sqlite-new/ Please note that the master branch is under review and not a potential future version. However, after consulting with the RM and due to the extended review period I consider mentioning these improvements to be proper.
Hi All, What was the result of this review? TY, Scott Bailey On Wed, Nov 13, 2024 at 6:30 AM Richard Hodges via Boost < boost@lists.boost.org> wrote:
Dear All,
Surprise!
The Boost formal review of the Boost SQLITE library starts *TODAY*, taking place
from November 13th, 2024 to November 22nd, 2024 (inclusive).
I apologise profusely for springing this on you without prior warning. The error is entirely mine. I am extending the period by one day to compensate.
The library is authored by Klemens Morgenstern (@klemens-morgenstern in the CppLang slack).
Documentation: https://klemens.dev/sqlite/ <https://anarthal.github.io/mysql/index.html> Source: https://github.com/klemens-morgenstern/sqlite <https://github.com/anarthal/mysql/>
From the documentation:
boost.sqlite is a simple to use C++ sqlite library. It provides a modern interface using facilities like error_code, views (e.g. for blobs) and the ability to use boost.describe or boost.pfr for parameterised queries.
Supported features include:
- typed queries - prepared statements - json support - custom functions (scalar, aggregate, windows) - event hooks - virtual tables
SQLite provides an excellent C-API, so this library does not attempt to hide, but to augment it.
Please provide in your review information you think is valuable to explain your choice to ACCEPT or REJECT including SQLITE as a Boost library. Please be explicit about your decision (ACCEPT or REJECT).
Some other questions you might want to consider answering:
- Will the library bring additional out-of-the-box utility to Boost? - What is your evaluation of the implementation? - What is your evaluation of the documentation? - Will the choice of API abstraction model ease the development of software that must talk to a SQLITE database? - Are there any immediate improvements that could be made after acceptance, if acceptance should happen? - Did you try to use the library? With which compiler(s)? Did you have any problems? - How much effort did you put into your evaluation? A glance? A quick reading? In-depth study? - Are you knowledgeable about the problem domain?
More information about the Boost Formal Review Process can be found at: http://www.boost.org/community/reviews.html
The review is open to anyone who is prepared to put in the work of evaluating and reviewing the library. Prior experience in contributing to Boost reviews is not a requirement.
Thank you for your efforts in the Boost community. They are very much appreciated.
Richard Hodges - review manager of the proposed Boost.SQLITE library
Klemens is often available on CppLang Slack and of course by email should you require any clarification not covered by the documentation, as am I.
_______________________________________________ Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost
participants (17)
-
Alexander Grund -
Arnaud Becheler -
Artyom Beilis -
barend＠xs4all.nl -
Dennis Luehring -
Dominique Devienne -
Joaquín M López Muñoz -
Klemens Morgenstern -
Klemens Morgenstern -
Kostas Savvidis -
Nelson, Erik - 2 -
Peter Dimov -
Richard Hodges -
Ruben Perez -
Scott Bailey -
Vinnie Falco -
Дмитрий Архипов