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: Support translation of ToString #19473

Merged
merged 2 commits into from
Oct 31, 2020

Conversation

ralmsdeveloper
Copy link
Contributor

@smitpatel
Copy link
Member

Enable tests

  • Like_with_non_string_column_using_ToString
  • Convert_ToString

@bricelam
Copy link
Contributor

bricelam commented Jan 3, 2020

Also, DateTime, DateTimeOffset, Decimal, Guid, and TimeSpan will be a different format than calling ToString on the client.

@bricelam
Copy link
Contributor

bricelam commented Jan 3, 2020

(Well, Guid will be the same format, just upper-case)

@ralmsdeveloper
Copy link
Contributor Author

Além disso, DateTime, DateTimeOffset, Decimal, Guid e TimeSpan será um formato diferente do que chamar ToString no cliente.

@bricelam

TimeStamp has no difference.
As far as I can see we will have the same information (eval client vs server)

For the others, the translation really is not compatible with client.
_**DateTime, DateTimeOffset, Decimal**_

@ralmsdeveloper
Copy link
Contributor Author

ralmsdeveloper commented Jan 4, 2020

Habilitar os testes Enable tests

  • Like_with_non_string_column_using_ToString
  • Convert_ToString

@smitpatel & @bricelam
We don't support Convert.ToString, should we implement?

@bricelam
Copy link
Contributor

bricelam commented Jan 6, 2020

We discussed this as a team, and we decided that in EF Core, the semantics of ToString() can be very loose. It can return any reasonable string representation of the value. The format may be different from the string returned by .NET. This is already the case on SQL Server when the client and server are using different cultures.

Given that, everything but ulong should translate using a simple Convert(..., typeof(string)) call on SQLite.

@smitpatel
Copy link
Member

We don't support Convert.ToString, should we implement?

Can skip. It can be done in separate PR at later stage.

{
private static readonly HashSet<Type> _typeMapping = new HashSet<Type>
{
typeof(int),
Copy link
Member

Choose a reason for hiding this comment

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

Can we add test for all these types in BuiltInDataTypesTestBase?

It can follow similar structure to QueryBuiltInDataTypesTest,
A client side value converted to ToString compared to ToString on the column.

@smitpatel smitpatel assigned maumar and unassigned smitpatel Jan 8, 2020
@smitpatel
Copy link
Member

re-assigning this to @maumar

@ralmsdeveloper
Copy link
Contributor Author

@bricelam sorry for the delay, I had to solve some things here in the company.

I've been doing some testing, so I removed support for:

Guid
Float
Byte[]

For some reason when trying to convert the value (-1.234 float) I'm getting -1.23399996757507

@bricelam
Copy link
Contributor

Add back Guid, float, and byte[].

float is expected to by lossy between .NET and SQLite

@ralmsdeveloper
Copy link
Contributor Author

@bricelam I'm sorry for the delay, I had a lot of things to solve here at the company, I'll add that until Saturday.

@bricelam
Copy link
Contributor

No rush; we don't RTM until November 😉

@smitpatel smitpatel requested a review from maumar June 15, 2020 18:40
@bricelam bricelam removed their assignment Jun 15, 2020
@ajcvickers
Copy link
Member

This PR still needs work and is lower priority than other items we need to complete for 5.0, so unfortunately we are going to hold off on this for now.

@microsoft-github-updates microsoft-github-updates bot changed the base branch from master to main July 23, 2020 04:35
@smitpatel
Copy link
Member

@ralmsdeveloper - Are you planning to complete this? Else I can take over and make needed changes to merge it to main.

@smitpatel smitpatel assigned smitpatel and unassigned maumar Oct 30, 2020
smitpatel added a commit to ralmsdeveloper/EntityFrameworkCore that referenced this pull request Oct 30, 2020
@smitpatel smitpatel requested review from bricelam and maumar and removed request for bricelam and maumar October 30, 2020 22:44
@runfoapp runfoapp bot mentioned this pull request Oct 30, 2020
@smitpatel smitpatel merged commit 1b3d637 into dotnet:main Oct 31, 2020
@smitpatel
Copy link
Member

@ralmsdeveloper - Thank you.

@ralmsdeveloper
Copy link
Contributor Author

@smitpatel sorry for the delay, I took a few days to help my father with his health!

Thanks!

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.

Sqlite Support translation of ToString
5 participants