Complete support for C++ integer types #954

zann1x · 2022-04-07T21:33:31Z

As often discussed (e.g. #90, #920, #926), SOCI doesn't have a complete support for all C++ integer types. This PR aims to fix this.

To remove ambiguity and confusion in cross-platform cases I switched from usage of short, int, long etc. to the fixed-width integer types int16_t, int32_t etc. which are part of the type support library as of C++11. Each of those types got its corresponding value in the exchange_type and data_type enum. This also meant renaming the enumerators to x_[u]intN and dt_[u]intN (e.g. x_uint32 and dt_uint32).
The mapping of the newly introduced types int8_t, uint8_t, int16_t, uint16_t and uint32_t to database column types was done to the best of my knowledge and can surely be judged better by someone working intensely with the specific database backends.

I also added additional test cases in common-tests.h to test the fixed-width types in general as well as their min and max boundaries. All existing tests should stay pretty much untouched and should still pass when this PR is finished.

The commits are divided into the needed changes per backend and tests. All commit messages not starting with "WIP: ..." are more or less done and ready for review.

vadz · 2022-04-08T17:17:25Z

Thanks a lot for working on this! It will take me some time to review this (and the CI failures will need to be fixed, of course), but I'll try to get to it a.s.a.p.

zann1x · 2022-04-08T18:30:04Z

A big part is still work in progress anyways, so take your time :)

Despite previously assumed and tested, most of the databases support storing the various unsigned integer values. The checks for it in the tests are therefore obsolete. Only Firebird and SQLite seem to have a problem with storing UINT64_MAX. The retrieval of it in a sorted result set with multiple other values shows that the value is stored as a signed integer in the database.

Firebird and SQLite store UINT64_MAX incorrectly, leading to an incorrect value ordering when retrieving sorted table contents.

The currently used preprocessor defines used to distinguish between platforms and defining the corresponding type_conversion/exchange_traits was found in multiple files. The logic of it is now put in a single place with custom defines to be used instead.

zann1x · 2023-08-13T15:47:33Z

A quick update from my side so you know that I haven't forgotten about this PR:

I've rebase the branch on the current master, jotted down something that introduces a new type db_type and reverted data_type back to its original state. Every user-facing method/variable that previously dealt with data_type got its respective overload or addition for db_type. The silent breakage should be gone now. I haven't updated the docs yet, but if you're okay with the changes, I'll update them accordingly.

Note that users can still encounter std::bad_cast exceptions because of the stricter type mapping. This can result in the weird circumstance that the old API e.g. returns dt_long_long, which, despite the name, you now have to retrieve as uint32_t instead of long long. So far, this worked because of the type_conversions in unsigned-types.h. Examples for that can be found in test-mysql.cpp (1, 2, 3).
We could possible relax the type check in row::get(size_t) in order to avoid some (or even all?) of the std::bad_cast exceptions if you're unhappy with these. The implicit type casts from #918 may be re-integrated here.

Next to that I noticed that I haven't added the bounds check in mysql::standard_into_type that we once talked about. This is now fixed as well.

Also, how do we want to proceed with the x_* types? If they are visible to users (which I suppose is possible somewhere?), they probably need a rework as well.

I'm pretty sure that these ones are supposed to be only used internally. And considering that they are in soci::details namespace, I feel like we would be justified in changing them incompatibly. And a simple code search for them doesn't find anything which is encouraging.

Another comment on this one: if they are supposed to be only used internally, can't we remove the old names and spare us type aliases like x_integer = x_int32?

vadz · 2023-08-13T18:15:43Z

Thanks a lot for the update and I'll try to get back to this a.s.a.p. but I'm on vacation right now, so it won't happen immediately -- sorry for yet another delay.

In the meanwhile, any testing and, in particular, reports of any compatibility problems would be welcome!

include/soci/sqlite3/soci-sqlite3.h

include/soci/column-info.h

vadz

Thanks a lot once again for all your work on this!

AFAICS there indeed shouldn't be any compatibility problems remaining, and the few minor questions below can be dealt with later, so I think we can merge this -- but you mentioned that you wanted to update the docs further and this would, of course, be very welcome.

Please let me know if you plan to do this in the near future or if I should merge it and maybe try to update them myself.

Thanks again!

vadz · 2023-10-15T21:00:43Z

include/soci/odbc/soci-odbc.h

+inline bool odbc_standard_type_backend_base::supports_negative_tinyint() const
+{
+    // MSSQL ODBC driver only supports a range of [0..255] for tinyint.
+    return statement_.session_.get_database_product()
+            != odbc_session_backend::prod_mssql;
+}
+
+inline bool odbc_standard_type_backend_base::can_convert_to_unsigned_sql_type() const
+{
+    // MSSQL ODBC driver seemingly can't handle the conversion of unsigned C
+    // types to their respective unsigned SQL type because they are out of
+    // range for their supported signed types. This results in the error
+    // "Numeric value out of range (SQL state 22003)".
+    // The only place it works is with tinyint values as their range is
+    // [0..255], i.e. they have enough space for unsigned values anyway.
+    return statement_.session_.get_database_product()
+            != odbc_session_backend::prod_mssql;
+}
+


I wouldn't make these functions inline, they don't seem to be performance critical but I suspect we might need to change/adjust them in the future to account for more ODBC quirks (e.g. test the exact version or something else).

All of these helper functions are declared and defined in the header, so I just continued this convention.

I suspect we might need to change/adjust them in the future to account for more ODBC quirks (e.g. test the exact version or something else).

Because of this I would just leave them as is if that's okay with you.

include/soci/soci-backend.h

vadz · 2023-10-15T21:05:25Z

include/soci/soci-platform.h

+#define SOCI_OS_LINUX       0x0001
+#define SOCI_OS_FREE_BSD    0x0002
+#define SOCI_OS_APPLE       0x0003
+#define SOCI_OS_WINDOWS     0x0004


I wonder if we could use enum class for those (and maybe make SOCI_OS a constexpr value of this type) rather than using the preprocessor?

SOCI_OS is (indirectly) used further down the line in preprocessor checks for enabling specific exchange_traits and type_conversions. I don't know how this would play out with enum class and/or constexpr.

vadz · 2023-10-15T21:18:44Z

tests/postgresql/test-postgresql.cpp

@@ -1199,7 +1214,7 @@ struct test_enum_with_explicit_custom_type_int_rowset : table_creator_base

        try
        {
-            sql << "CREATE TABLE soci_test( Type smallint)";
+            sql << "CREATE TABLE soci_test( Type integer)";


Sorry, do you remember why did this have to be changed? Does it indicate a potential incompatibility?

smallint is mapped to integer in the current code and int16 in the new one.

@zann1x Does this mean that using into(int_var) from a column of smallint type doesn't work any longer? If so, this would be problematic as it still would be a silent break.

There is a discussion of allowing (at least broadening) implicit conversions in #1088 but can we do something like this for into() itself?

That is basically what I meant here:

Note that users can still encounter std::bad_cast exceptions because of the stricter type mapping. This can result in the weird circumstance that the old API e.g. returns dt_long_long, which, despite the name, you now have to retrieve as uint32_t instead of long long. So far, this worked because of the type_conversions in unsigned-types.h. Examples for that can be found in test-mysql.cpp (1, 2, 3).
We could possible relax the type check in row::get(size_t) in order to avoid some (or even all?) of the std::bad_cast exceptions if you're unhappy with these. The implicit type casts from #918 may be re-integrated here.

#918 should be reworked as it declares template metaprog functions that are already standard in c++11.

So I guess we do need something along the lines of #1088 to preserve compatibility for row API uses (BTW, just to fix the definitions: "silent breakage" is code which continues to compile, without warnings, but changes behaviour, so this would be one).

@Sildra Would you agree with reworking your PR to only allow lossless implicit type conversions, i.e. those from smaller to bigger int types (of the same sign?), in the row API? If so, I still think we should merge this one and then merge your PR. TIA!

I think I can allow this by comparing the sizeof of source and target as precheks and separating double. Will work on it tommorow.

Better idea, I can change the static cast to perform runtime narrowing checks.

Sildra@0eb519b

Let me know what you think of this implem.

I'm not convinced that we need narrowing conventions, it risks being very confusing if your code works perfectly fine at first and then suddenly starts falling with an exception just because somebody added a value allowed by the column type but out of the C++ type range to the database.

But I'd also prefer to discuss this in its own PR, this one is becoming very difficult to navigate.

include/soci/column-info.h

src/backends/mysql/statement.cpp

zann1x · 2023-10-16T09:35:46Z

AFAICS there indeed shouldn't be any compatibility problems remaining, and the few minor questions below can be dealt with later, so I think we can merge this

Nice, that's great to hear!

but you mentioned that you wanted to update the docs further and this would, of course, be very welcome.

Please let me know if you plan to do this in the near future or if I should merge it and maybe try to update them myself.

I'll update the docs and address your comments sometime later this week.

include/soci/soci-backend.h

Sildra · 2023-10-20T10:49:33Z

As more and more databases are adding a json type, is it possible to add a db_json in the enum ?

vadz · 2024-01-02T23:45:17Z

Sorry for missing my own deadline of merging this in 2023, but I think it should be ready to merge now with just some minor changes from #1116 -- please let me know if you see anything wrong with them, otherwise I'll (squash) merge that one soon.

Thanks again for all your work here!

Provide complete support for all C++ integer types and map them to the corresponding database types whenever possible. See #954, #1116.

vadz added this to the 4.1.0 milestone Apr 8, 2022

zann1x force-pushed the data_types branch 2 times, most recently from 797fe86 to 90219de Compare April 14, 2022 22:11

zann1x force-pushed the data_types branch 3 times, most recently from 74a0e67 to 3ef6a9e Compare May 5, 2022 08:59

zann1x force-pushed the data_types branch from 3ef6a9e to 9437df4 Compare May 8, 2022 16:51

zann1x force-pushed the data_types branch 9 times, most recently from 02888cc to d69c2fd Compare June 3, 2022 09:56

zann1x force-pushed the data_types branch 12 times, most recently from ad41518 to 0333372 Compare June 9, 2022 21:52

zann1x added 8 commits August 13, 2023 17:25

Fix uint64 vector unit tests

88e2cb4

Firebird and SQLite store UINT64_MAX incorrectly, leading to an incorrect value ordering when retrieving sorted table contents.

Fix MSSQL ODBC unit tests

e8e4fc9

Fix Postgres unit test

4de379f

Fix MySQL ODBC unit tests

1c3e069

Handle potential overflow in mysql into-conversion

ff7e29c

Expose new db_type for dynamic type mapping

826fa77

zann1x force-pushed the data_types branch from b8eaeca to 826fa77 Compare August 13, 2023 15:40

zann1x mentioned this pull request Aug 18, 2023

Query-Logging with prepared statements #1068

Open

Sildra reviewed Oct 11, 2023

View reviewed changes

include/soci/sqlite3/soci-sqlite3.h Outdated Show resolved Hide resolved

Sildra mentioned this pull request Oct 11, 2023

SQLite incorrect DDL handling #1085

Closed

vadz mentioned this pull request Oct 11, 2023

SOCI doesn't support automatic type conversion #1088

Closed

Sildra reviewed Oct 11, 2023

View reviewed changes

include/soci/column-info.h Show resolved Hide resolved

vadz approved these changes Oct 15, 2023

View reviewed changes

Sildra reviewed Oct 15, 2023

View reviewed changes

src/backends/mysql/statement.cpp Show resolved Hide resolved

zann1x added 2 commits October 18, 2023 22:28

Add db_type description to docs

037a332

Forward create_column_type() to db_type overload

06b68c2

Sildra reviewed Oct 19, 2023

View reviewed changes

include/soci/soci-backend.h Show resolved Hide resolved

vadz mentioned this pull request Oct 20, 2023

Add a db_json in the enum #1095

Open

Sildra mentioned this pull request Oct 22, 2023

Add automatic widening conversions in dynamic bindings #1097

Closed

vadz mentioned this pull request Jan 2, 2024

C++ data types support v2 #1116

Merged

vadz added a commit that referenced this pull request Jan 11, 2024

Merge branch 'data_types'

78ee6ef

Provide complete support for all C++ integer types and map them to the corresponding database types whenever possible. See #954, #1116.

vadz merged commit 06b68c2 into SOCI:master Jan 11, 2024
16 checks passed

zann1x deleted the data_types branch January 18, 2024 14:38

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Complete support for C++ integer types #954

Complete support for C++ integer types #954

zann1x commented Apr 7, 2022

vadz commented Apr 8, 2022

zann1x commented Apr 8, 2022

zann1x commented Aug 13, 2023 •

edited

Loading

vadz commented Aug 13, 2023

vadz left a comment

vadz Oct 15, 2023

zann1x Oct 18, 2023

vadz Oct 15, 2023

zann1x Oct 18, 2023

vadz Oct 15, 2023

Sildra Oct 15, 2023

vadz Oct 16, 2023

zann1x Oct 16, 2023

Sildra Oct 16, 2023

vadz Oct 18, 2023

Sildra Oct 19, 2023

Sildra Oct 19, 2023

Sildra Oct 21, 2023

vadz Oct 21, 2023

zann1x commented Oct 16, 2023

Sildra commented Oct 20, 2023

vadz commented Jan 2, 2024

Complete support for C++ integer types #954

Complete support for C++ integer types #954

Conversation

zann1x commented Apr 7, 2022

vadz commented Apr 8, 2022

zann1x commented Apr 8, 2022

zann1x commented Aug 13, 2023 • edited Loading

vadz commented Aug 13, 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

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

zann1x commented Oct 16, 2023

Sildra commented Oct 20, 2023

vadz commented Jan 2, 2024

zann1x commented Aug 13, 2023 •

edited

Loading