Re: [review] [sqlite]

From: barend_at_[hidden]
Date: 2024-11-17 16:51:30

**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