Re: [review] [sqlite]

From: Klemens Morgenstern (klemensdavidmorgenstern_at_[hidden])
Date: 2024-11-18 09:34:16

On Mon, Nov 18, 2024 at 1:03 AM Barend Gehrels via Boost <
boost_at_[hidden]> 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
>