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

SQLite affinity fallback is wrong: fallback to blob instead of numeric #31915

Closed
buybackoff opened this issue Sep 29, 2023 · 5 comments
Closed
Labels
closed-no-further-action The issue is closed and no further action is planned. customer-reported

Comments

@buybackoff
Copy link

This line creates a problem when trying to get a .NET type from SQLite storage type.

if (clrType == null || clrType == typeof(byte[]))
{
return Blob;

From this SQLite docs item 3.1.5: Otherwise, the affinity is NUMERIC. That is, when _typeRules cannot resolve a type then the type should be numeric. However, the current implementation falls back to Blob. Item 3.1.3 says that the blob should only be used as a fallback when the storage type is absent: if no type is specified then the column has affinity BLOB

It's funny that numeric is resolved to blob there. 🤦 Also problematic are: TIMESTAMP, DECIMAL(5,2).

Found this by manually scaffolding SQLite Sakila DB (e.g. Actor, Payment tables). Specifically, using IRelationalTypeMappingSource.FindMapping(DatabaseColumn.StoreType).

@ajcvickers
Copy link
Contributor

/cc @bricelam

@buybackoff
Copy link
Author

I looked a bit deeper. Numeric is internal to SQLite and not representable in C#, it means either int or double and one need to choose either something universal (double) or apply some heuristics bases on column name or storage type name. All options are not nice, but falling back to double is simpler and still better than to byte[].

In my code I just implemented the affinity resolution according to SQLite specs before calling this EF API, where I map non-standard storage types to real and then get double from the API. Yet still I had to introduce a strict mode to require manual override of any ambiguous case.

@bricelam
Copy link
Contributor

bricelam commented Oct 2, 2023

This was a conscious decision made in 3.0 to fallback to BLOB instead of INTEGER/REAL (see #13841 and #13253).

...falling back to double is...better than to byte[]

We disagree. The chances of an unknown type being long or double seem a lot lower than it being something like string or byte[]. Since byte[] is the only format that's guaranteed to be lossless, we went with that.

...or apply some heuristics bases on column name or storage type name

This is exactly what we've done in 8.0 when reverse engineering a model from an existing database. See #8824

@buybackoff
Copy link
Author

I do not understand the reasoning behind this phrase:

The chances of an unknown type being long or double seem a lot lower than it being something like string or byte[]

According to the SQLite docs a present (any non-empty string) type that is not resolved to any affinity is treated as numeric. In the Sakila sample DB all wrong cases are numeric.

Thanks a lot for the links! I would not have found them easily. Now I understand what's going and why in EF code.

I have a workaround for my needs so this could be closed.

@bricelam
Copy link
Contributor

bricelam commented Oct 3, 2023

An affinity of numeric is not the same as a static type of integer or real. ADO.NET is statically typed. Saying that a column with an unknown type is always numeric is different than saying it has a numeric affinity. Even though SQLite has type affinity, at the end of the day, it's dynamically typed and will work with any type inside the column regardless of affinity.

@ajcvickers ajcvickers closed this as not planned Won't fix, can't repro, duplicate, stale Oct 4, 2023
@ajcvickers ajcvickers added the closed-no-further-action The issue is closed and no further action is planned. label Oct 4, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
closed-no-further-action The issue is closed and no further action is planned. customer-reported
Projects
None yet
Development

No branches or pull requests

3 participants