Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

C++ data types support v2 #1116

Merged
merged 47 commits into from
Jan 11, 2024
Merged

C++ data types support v2 #1116

merged 47 commits into from
Jan 11, 2024

Conversation

vadz
Copy link
Member

@vadz vadz commented Jan 2, 2024

Unfortunately I don't have permission to push to @zann1x fork, so I had to create the new PR instead of updating #954. I've added a few small changes on top of yours and I think this should be merged now, but please let me know if you see anything wrong with what I did.

Also, I think it would probably be better to squash merge this instead of preserving the existing commits but, again, please let me know if you'd rather keep them. Thanks in advance!

To stay backwards-compatible as much as possible, we should keep all
methods from the soci-simple API and just forward them to their new
equivalents.
Types that refered to non-fixed-size types have been replaced in favor
of their fixed-size types (e.g. dt_integer -> dt_int32, x_short ->
x_int16). To be backwards compatible, we need to keep all of the
previous types though.
bool values occupy 1 byte of storage in Postgres, so we might as well
extract it as 1 byte instead of 4.
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.
zann1x and others added 14 commits August 13, 2023 17:25
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.
Make sure to list db_type first, before the now deprecated data_type.
Also define deprecated x_typename synonyms at the end of exchange_type
enum.

No real changes.
Forward the former to the latter instead of repeating the same code for
both.
Preserve the functions taking data_type for compatibility.
This is unnecessary as it can be recovered from db_type: even if this is
not lossless, we don't really care about it if all we need is data_type,
so simplify the API and the implementation by only having one "type"
parameter instead of two.

No real changes.
This should have been part of fdfcc12 (Remove data_type argument from
describe_column() backend function, 2024-01-03) but was forgotten there.
It wasn't quite the same for all of them, notably MySQL and ODBC
disagreed about the mapping of db_uint32 and ODBC backend also mapped
db_uint64 to signed dt_long_long and not dt_unsigned_long_long as the
other backends, so make it possible to preserve its existing behaviour
by overriding to_data_type() in it.

There is still a minor incompatibility in MySQL backend: the type of
unsigned INT24 fields is now dt_long_long and not dt_integer as before,
but we can hopefully live with this.
@zann1x
Copy link
Contributor

zann1x commented Jan 3, 2024

Thanks for your updates on this issue!
I'm fine with almost all of the changes you've made. As you've noticed yourself, we'd yet again introduce a breakage with fdfcc12. Because of the effort we put into making the PR backwards compatible, I'd argue to not make an exception here either. I.e., I would keep the extra data_type argument in describe_column in order to be able to represent the old behavior in all cases.

@vadz
Copy link
Member Author

vadz commented Jan 3, 2024

I admit I hadn't realized there would be this problem when starting to make this change, and I probably wouldn't have done it if I did, but now that it's done I think we can live with the compatibility breakage just for unsigned int24 values in MySQL -- this is such a narrow corner case that it would be a pity to continue dealing with data_type in all backends for all types just to preserve compatibility with it.

If we could find some way to deal with this at MySQL level, I'd be for it, but, again, it's very annoying to have this extra parameter in all the other backends that don't need it at all. Unfortunately the only other solution I see is to add db_uint24 which is not great neither. But if we're serious about MySQL MEDIUMINT support, maybe we should do this?

@zann1x
Copy link
Contributor

zann1x commented Jan 3, 2024

I wouldn't want to add db_uint24 just for the sake of making the MySQL backend happy.

Another solution might be to update some variable in mysql_statement_backend indicating when this special case was encountered in mysql_statement_backend::describe_column. An override of mysql_statement_backend::to_data_type could check for that variable's state and handle the case accordingly.

Add a hack to still return dt_integer for them instead of dt_long_long
returned for the other columns of db_uint32 type.
@vadz
Copy link
Member Author

vadz commented Jan 3, 2024

I wouldn't want to add db_uint24 just for the sake of making the MySQL backend happy.

I wouldn't neither, but logically speaking this would be consistent with using sized integer types for all the rest...

Another solution might be to update some variable in mysql_statement_backend indicating when this special case was encountered in mysql_statement_backend::describe_column. An override of mysql_statement_backend::to_data_type could check for that variable's state and handle the case accordingly.

This would make the function non-reentrant, which is not great neither, but it would at least be limited to MySQL backend... So let's do it like this (pushed now), thanks!

@vadz vadz merged commit 78ee6ef into SOCI:master Jan 11, 2024
16 checks passed
@vadz vadz deleted the data_types branch January 11, 2024 14:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants