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