Skip to content

Revert "Type MApping: a long gets mapped to integer in Jet" #151

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

Merged
merged 1 commit into from
Oct 18, 2023

Conversation

lauxjpn
Copy link
Member

@lauxjpn lauxjpn commented Oct 12, 2023

This reverts commit 05b10ea.

The long datatype in Jet is 32 bit and therefore equals the Int32 CLR type.
We cannot use it to map the Int64 CLR type to Jet.

The decimal datatype in Jet has a total number of 28 digits, which can be used to represent the largest 64 bit value (signed and unsigned) of 18,446,744,073,709,551,616 (20 digits).

So the original implementation should be correct.

@lauxjpn lauxjpn added the bug label Oct 12, 2023
@lauxjpn lauxjpn added this to the 8.0.0-alpha.2 milestone Oct 12, 2023
@lauxjpn lauxjpn requested a review from ChrisJollyAU October 12, 2023 13:39
@lauxjpn lauxjpn self-assigned this Oct 12, 2023
Copy link
Member

@ChrisJollyAU ChrisJollyAU left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The only limitation then is that it will only be supported in OleDb. Odbc still doesn't handle decimal

@lauxjpn
Copy link
Member Author

lauxjpn commented Oct 12, 2023

Good point.

It should also be mentioned, that recent Access database versions have an actual (opt-in) large number data type.

While we could automatically use decimal for OLE DB and integer for ODBC, it would make switching between those underlying providers difficult.

The way to go is probably to add an option that allows users to select between the store types. This way, the large number type can be added later as well.

@ChrisJollyAU
Copy link
Member

The large number type for recent version (2016+) might not work. When you enable it it can only be opened by 2016 and later. And then its unknown how it would via OleDb/Odbc as it would need a version of the access driver/provider that can understand that format and data type

By the looks of it quickly one has to enable it directly via the desktop app. Not sure if it can be done programmatically.

@lauxjpn
Copy link
Member Author

lauxjpn commented Oct 12, 2023

Not sure if it can be done programmatically.

Should be possible.

The large number type for recent version (2016+) might not work. When you enable it it can only be opened by 2016 and later. And then its unknown how it would via OleDb/Odbc as it would need a version of the access driver/provider that can understand that format and data type

Yeah, that's why we should make it an option for users to select, rather than a default. If someone enables it, it is that persons responsibility.

Anyway, I don't consider it high priority to implement large numbers, as this is a rather new feature and Jet is mostly a legacy database (except for distributed network share scenarios).

@ChrisJollyAU
Copy link
Member

So we go with the option of adding a configuration option to enable using Int64. Call it something like EnableInt64Support

@lauxjpn
Copy link
Member Author

lauxjpn commented Oct 13, 2023

Call it something like EnableInt64Support

It needs to be future-proof in this case here (as discussed above), so it is going to be an enum.

@ChrisJollyAU
Copy link
Member

Some comments from a bit of research

  1. Saving decimal values with Odbc can be done if configured correctly. The OdbcType field of the parameter needs to be set to Numeric as in ((OdbcParameter)parameter).OdbcType = OdbcType.Numeric . The only drawback is that we would need to include System.Data.Odbc in EFCore.Jet (the main package) rather than it only being pulled in with the .Odbc specific package. Tested with Model79_CantSaveDecimalValue and it passes and round-trips correctly
  2. The Large Number data type in Access 2016+ can be used. Obviously if used the database gets stamped and can't be opened by a provider earlier than that (e.g. 2010 database engine).
  3. ADOX and DAO do support it and correctly returns adBigInt. We do have a bug there in that the code to convert that to a string returns integer
  4. The store types to use in the SQL are bigint and datetime2 (Date/Time Extended is also supported). Using these store types automatically stamps the database
  5. Int64 values seem to have a problem being saved (at least in OleDb - havent tested Odbc). It seems x64 gives a conversion error for a parameter (even if the value is just 2). However in x86 it works fine (probably only if the value is within Int32 range, even though parameter has been configured as DbType.Int64). parameter.DbType = System.Data.DbType.Object does the trick. Tested in 365_x64 and successfully round-trip Int64.MaxValue

@lauxjpn
Copy link
Member Author

lauxjpn commented Oct 14, 2023

Nice work!

@ChrisJollyAU
Copy link
Member

Once #158 is in this PR should be able to be merged if we want to map Int64/long to use the decimal type as the base/default. Anything else is pretty much under #154

@ChrisJollyAU ChrisJollyAU merged commit 9babf78 into CirrusRedOrg:master Oct 18, 2023
@lauxjpn
Copy link
Member Author

lauxjpn commented Oct 18, 2023

@ChrisJollyAU Hmm, this wasn't finished yet. Is was marked as work-in-progress. Can we revert the merge again?

@ChrisJollyAU
Copy link
Member

What was still to do? We had split out the bigint work to #154 . And now that we solved the Odbc bug for decimal we didn't have to have 2 options for the user as to how to treat long. We just default to decimal. They can turn the BigInt support on later if they want

@lauxjpn
Copy link
Member Author

lauxjpn commented Oct 18, 2023

Okay, its fine then.

@lauxjpn lauxjpn deleted the revert/bigint branch October 18, 2023 16:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants