Add column_info query for SQLite backend #1079

Sildra · 2023-09-19T21:26:21Z

As SQLite doesn't have a dedicated table scheme we use the associated pragma to get those info.

We defer the column description statement preparation to the backend to take into account others data structures.

vadz

Thanks for working on this, it would indeed be nice to have this functionality for SQLite, but it would be even nicer if we could avoid introducing special API just for it.

Could we define a type_conversion specialization for column_info that would use backend-specific functions to perform the translation instead perhaps?

vadz · 2023-09-19T22:12:46Z

include/soci/session.h

@@ -138,13 +138,16 @@ class SOCI_DECL session
    // it makes sense to bind std::string for the output field.
    details::prepare_temp_type prepare_table_names();

+    [[deprecated("Use the override prepare_column_descriptions with column_info to support more backends (i.e. sqlite)")]]


"overload" was probably meant instead of "override".

vadz · 2023-09-19T22:14:03Z

src/backends/sqlite3/session.cpp

+details::prepare_temp_type prepare_column_descriptions_statement(
+        details::prepare_type& prepare, std::string & table_name, soci::column_info& info)
+{
+    // trick the compiler to use the static type sqlite_column_info instead of the standard one


This cast is undefined behaviour and really shouldn't be done. With the current API the obvious alternative is to create a local sqlite_column_info object and copy it to info later, but it would be great if we could have a nicer API not requiring passing info here at all.

Will change it to reinterpret_cast<sqlite_&> instead, which is not ub.

It's still UB because the static type doesn't match the cast target.

But the problem is not "just" the UB, it's really problematic to perform a cast to a wrong type.

Definitely not an UB, the case is described in cppreference : https://en.cppreference.com/w/cpp/language/reinterpret_cast#Notes

I will correct the code this week-end and propose others implementations but they will likely be worse and introduce larger API changes.
In a static context the only solution I have in mind is :

Calling another function (what I am doing with the reinterpret cast)

Having dynamic type in the parameters that are aware of the backend :

By having a field in the column_info (or another type)

By making the colum_info virtual (or another type)

In the client, it will be something like this :

namespace soci { struct dynamic_column_info : public column_info { virtual void from_base(values const & v, indicator ind) { soci::type_conversion<column_info>::from_base(v, ind, &this); } template<> struct type_conversion<dynamic_column_info> { typedef values base_type; static void from_base(values const & v, indicator ind, dynamic_column_info & ci) { ci.from_base(v, ind); } } /* struct tc */ } /* !namespace soci */ std::string tableName; std::unique_ptr<dynamic_column_info> ci = sql.create_backend_typed_column_info(); soci::statement stmt = (sql.prepare << sql.prepare_column_descriptions(tableName), soci::into(*ci));

I admit, it will be cleaner in the backend but in the user code, not so much.

Sorry, where do you see anything allowing to cast a pointer to an object to a pointer of a type different from the actual run-time type in these notes?

struct S { int x; }; struct S1 : S {}; // standard-layout S1 s1 = {}; auto p1 = reinterpret_cast<S*>(&s1); // value of p1 is "pointer to the S subobject of s1" auto i = p1->x; // OK p1->x = 1; // OK

This cast is in another direction. Of course you can cast a pointer to the derived class to the pointer of a base class. But you're doing it the other way round and when the object being pointed to is not an object of the derived class.

Sildra · 2023-09-19T23:03:05Z

I will try to check if it is possible to go back to the current backend in the type_conversion but do not expect anything. It is still a static method with only minimal type information.

I still need to solve the compilation issues.

vadz · 2023-09-21T12:04:45Z

After really looking at this I wonder why can't we use select whatever from pragma_table_info(:t) instead of pragma table_info(:t). Wouldn't it solve the problem in a much simpler way?

Sildra · 2023-09-21T12:41:45Z

It works but it doesn't solve the fact that type parsing will be missmatched between creation and retrieval of the column info.
I will need to create a huge statement to translate the types to the ones defined by column_info.

vadz · 2023-09-21T13:52:59Z

It works but it doesn't solve the fact that type parsing will be missmatched between creation and retrieval of the column info. I will need to create a huge statement to translate the types to the ones defined by column_info.

It looks like the approach taken so far is to recognize all types for all databases in type_conversion<column_info>, so you could just add SQLite-specific types there too. I don't like it very much, IMO there should be a backend function for translating the database-specific type to data_type (and some way to access the backend from type_conversion), but at least it should be pretty simple to do it like this.

Sildra · 2023-09-21T15:10:42Z

select name as 'COLUMN_NAME',
    0 as 'CHARACTER_MAXIMUM_LENGTH',
    0 as 'NUMERIC_PRECISION',
    CASE WHEN type LIKE '%REAL%' OR type LIKE '%FLOAT%' OR type LIKE '%DOUBLE%' THEN 255 ELSE 0 END as 'NUMERIC_SCALE',
    CASE
        WHEN type LIKE 'TEXT'   OR type LIKE 'CLOB'     OR type LIKE '%CHAR%'    THEN 'TEXT'
        WHEN type LIKE '%INT%'  OR type LIKE '%NUMBER%' OR type LIKE '%NUMERIC%' THEN 'INTEGER'
        WHEN type LIKE '%REAL%' OR type LIKE '%FLOAT%'  OR type LIKE '%DOUBLE%'  THEN 'NUMBER'
    ELSE UPPER(type)
    END as 'DATA_TYPE',
    CASE WHEN "notnull" = 0 THEN 'YES' ELSE 'NO' END as 'IS_NULLABLE'
    from pragma_table_info(:t);

Should do the trick, I don't have access to git for the PR ATM.

vadz

Looks good to me, thanks!

I can merge it as is or we can switch to using raw string if you prefer.

vadz · 2023-09-27T11:39:33Z

include/soci/sqlite3/soci-sqlite3.h

@@ -307,6 +307,21 @@ struct sqlite3_session_backend : details::session_backend
        return "select name as \"TABLE_NAME\""
                " from sqlite_master where type = 'table'";
    }
+    std::string get_column_description_query()
+    {
+        return "select name as 'COLUMN_NAME',"


I think this could be a bit more readable using raw C++11 string literals (R"(...)").

As you want, I missed the override specifier on the function definition.
For the string, the only change will be the escaped quote around notnull, which is fairly contained at the end of the query.

I missed the const too, because it doesn't override at the moment :(

I will fix it and add some tests after.

Sildra · 2023-10-12T20:29:32Z

Appveyor uses sqlite version 3.12 and support for the pragma has been added in 3.14, I will skip the test in this case.

vadz

Thanks for the update and the test!

It looks like find_column()-related changes are independent of the DDL ones, so it would be great to extract them into a separate commit, but if it's too difficult to do, we can merge this as is.

vadz · 2023-10-14T13:35:58Z

tests/sqlite3/test-sqlite3.cpp

+{
+    // Macro assigned to variable to avoid constant expression in if (disables warnings)
+    int sqliteVersion = SQLITE_VERSION_NUMBER;
+    if (sqliteVersion < 3'014'000)


We still need to support C++11.

Suggested change

if (sqliteVersion < 3'014'000)

if (sqliteVersion < 3014000)

vadz · 2023-10-14T13:43:42Z

It looks like SQLite under macOS might be too old?

Sildra · 2023-10-15T17:58:31Z

Unexpectedly, SQLite requires version 3.36 (3.35 in the doc, but it had some bugs in it) for the drop column feature, I am adding it to the prechecks of the test.

Sildra · 2023-10-18T10:27:13Z

@vadz can you merge the PR if everything is good ?
Thanks in advance.

vadz

It looks like the (unrelated, AFAICS) commit refactoring values code broke Oracle tests, could you please check this? I'd be fine with just excluding this commit from this PR too (and would actually prefer it).

TIA!

vadz · 2023-10-18T15:06:52Z

.gitignore

Sorry, I'm not going to merge this. I don't think we should encourage building in the source directory.

vadz · 2023-10-18T15:08:32Z

tests/sqlite3/test-sqlite3.cpp

+#if SQLITE_VERSION_NUMBER < 3036000
+#if SQLITE_VERSION_NUMBER < 3014000


Shouldn't those be run-time, rather than compile-time, tests?

Sildra · 2023-10-18T20:08:44Z

commit removed & compile checks replaced with runtime checks

vadz

Thanks for the update but the build needs to be fixed now.

vadz · 2023-10-18T21:17:06Z

tests/sqlite3/test-sqlite3.cpp

+// Test the DDL and metadata functionality
+TEST_CASE("SQLite DDL with metadata", "[sqlite][ddl]")
+{
+    if (sqlite3_libversion_number() < 3036000) {


I don't know why this doesn't compile in the CI builds, maybe sqlite.h is not included from here?

We could wrap this function at the backend level or just do select sqlite_version(); here perhaps?

I think there is a namespace wrapper in the inclusion of the sqlite header, I will just add the namespace and do your proposed change.

I tested the compil and test on my side, it is now working as intended.

tests/sqlite3/test-sqlite3.cpp

vadz · 2023-10-19T23:13:11Z

Thanks! Will merge in a moment.

Sildra changed the title ~~Improve API to take into account sqlite pragma for column introspection~~ [WIP] Improve API to take into account sqlite pragma for column introspection Sep 19, 2023

vadz reviewed Sep 19, 2023

View reviewed changes

vadz added the SQLite label Sep 19, 2023

Sildra force-pushed the master branch from f8af1fe to 4569bea Compare September 19, 2023 23:05

Sildra force-pushed the master branch from 4569bea to bf7ae24 Compare September 21, 2023 16:39

Sildra changed the title ~~[WIP] Improve API to take into account sqlite pragma for column introspection~~ Add column_info query for SQLite backend Sep 21, 2023

Sildra requested a review from vadz September 27, 2023 08:45

vadz approved these changes Sep 27, 2023

View reviewed changes

Sildra force-pushed the master branch 2 times, most recently from e64c2b1 to 6f8efb6 Compare October 12, 2023 19:30

Sildra force-pushed the master branch from 6f8efb6 to 9267b04 Compare October 12, 2023 20:47

Sildra requested a review from vadz October 13, 2023 09:07

vadz reviewed Oct 14, 2023

View reviewed changes

Sildra force-pushed the master branch 2 times, most recently from 0607ab4 to d830008 Compare October 16, 2023 16:32

Sildra requested a review from vadz October 16, 2023 19:11

vadz reviewed Oct 18, 2023

View reviewed changes

Sildra force-pushed the master branch from d830008 to 4b0883c Compare October 18, 2023 20:07

vadz reviewed Oct 18, 2023

View reviewed changes

Add query for column_info in sqlite3 backend

ca10f48

Sildra force-pushed the master branch from 4b0883c to ca10f48 Compare October 18, 2023 23:15

vadz closed this in 1c37a5f Oct 19, 2023

		#if SQLITE_VERSION_NUMBER < 3036000
		#if SQLITE_VERSION_NUMBER < 3014000

Add column_info query for SQLite backend #1079

Add column_info query for SQLite backend #1079

Conversation

Sildra commented Sep 19, 2023

vadz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Sildra commented Sep 19, 2023

vadz commented Sep 21, 2023

Sildra commented Sep 21, 2023

vadz commented Sep 21, 2023

Sildra commented Sep 21, 2023

vadz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Sildra commented Oct 12, 2023 • edited Loading

vadz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vadz commented Oct 14, 2023

Sildra commented Oct 15, 2023

Sildra commented Oct 18, 2023

vadz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Sildra commented Oct 18, 2023

vadz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Sildra Oct 18, 2023 • edited Loading

Choose a reason for hiding this comment

vadz commented Oct 19, 2023

Sildra commented Oct 12, 2023 •

edited

Loading

Sildra Oct 18, 2023 •

edited

Loading