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.

Sign up for GitHub

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

Jump to bottom

Adding Half type support for SqlLite #35193

Closed
wants to merge 5 commits into from
Closed

Adding Half type support for SqlLite #35193

wants to merge 5 commits into from

Conversation

newmasterSG
Copy link

@newmasterSG newmasterSG commented Nov 24, 2024 •
edited
Loading

  • I've read the guidelines for contributing and seen the walkthrough
  • I've posted a comment on an issue with a detailed description of how I am planning to contribute and got approval from a member of the team
  • The code builds and tests pass locally (also verified by our automated build checks)
  • Commit messages follow this format:
        Summary of the changes
        - add supporting half type
        - add new implementation RelationalTypeMapping for Half type

Fixes #30931

  • Tests for the changes have been added (for bug fixes / features)
  • Code follows the same patterns and style as existing code in this repo

@newmasterSG newmasterSG requested a review from a team as a code owner November 24, 2024 22:14
@newmasterSG
Copy link
Author

newmasterSG commented Nov 24, 2024 •
edited
Loading

Contributor

@dotnet-policy-service agree

@cincuranet cincuranet requested review from cincuranet and removed request for a team November 27, 2024 10:49
cincuranet
cincuranet requested changes Dec 3, 2024
View reviewed changes
Copy link
Contributor

@cincuranet cincuranet left a comment

Choose a reason for hiding this comment

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

Thanks for your contribution. This looks good, just some small details to iron out.

src/EFCore.Sqlite.Core/Storage/Internal/SqliteHalfTypeMapping.cs Outdated Show resolved Hide resolved
src/EFCore.Sqlite.Core/Storage/Internal/SqliteHalfTypeMapping.cs Outdated
=> "'" + HalfFormatConst + "'";

/// <inheritdoc/>
protected override string GenerateNonNullSqlLiteral(object value) => Convert.ToDouble((Half)value).ToString("R");
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this shouldn't be here, given we have proper implementation for SqlLiteralFormatString.

Copy link
Author

Choose a reason for hiding this comment

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

Just remove this code?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes.

src/Microsoft.Data.Sqlite.Core/SqliteValueReader.cs Show resolved Hide resolved
src/Microsoft.Data.Sqlite.Core/SqliteValueReader.cs Show resolved Hide resolved
bricelam
bricelam suggested changes Dec 4, 2024
View reviewed changes
src/EFCore.Sqlite.Core/Storage/Internal/SqliteTypeMappingSource.cs Outdated
@@ -82,6 +82,9 @@ private static readonly HashSet<string> SpatialiteTypes
{ typeof(decimal), SqliteDecimalTypeMapping.Default },
{ typeof(double), Real },
{ typeof(float), new FloatTypeMapping(RealTypeName) },
#if NET5_0_OR_GREATER
{ typeof(Half), new FloatTypeMapping(RealTypeName) },
Copy link
Contributor

@bricelam bricelam Dec 4, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

This should be SqliteHalfMapping, not FloatTypeMapping.

Copy link
Contributor

Choose a reason for hiding this comment

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

Good catch. I missed that.

bricelam
bricelam reviewed Dec 4, 2024
View reviewed changes
src/EFCore.Sqlite.Core/Storage/Internal/SqliteHalfTypeMapping.cs Outdated
Comment on lines 43 to 44
protected override string SqlLiteralFormatString
=> "'" + HalfFormatConst + "'";
Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm, this doesn't look right. It will store them as TEXT values, not REAL. I think this whole implementation should look more like FloatTypeMapping and DoubleTypeMapping.

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, should this be added to Relational instead?

Copy link
Author

Choose a reason for hiding this comment

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

What do you mean as Relation?

Copy link
Contributor

Choose a reason for hiding this comment

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

In EFCore.Relaional (next to the ones for float and double). This would allow other providers to reuse them.

Copy link
Author

Choose a reason for hiding this comment

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

Yeah, thanks. I re-wrote from sqlhalftypemapping to simple half type mapping

@cincuranet cincuranet self-assigned this Dec 4, 2024
@newmasterSG newmasterSG requested a review from bricelam December 5, 2024 06:48
@cincuranet
Copy link
Contributor

Looking at it with fresh eyes now and also after talking with the team... This should be split into two PRs. First would handle only the Microsoft.Data.Sqlite part (what #30931 is basically about). And for EF you should open issue first to discuss it first - because it definitely needs a discussion - and after it is approved you can submit another PR. I'll close this now @newmasterSG and feel free to open new PR for the Microsoft.Data.Sqlite part.

@cincuranet cincuranet closed this Dec 5, 2024
@newmasterSG
Copy link
Author

newmasterSG commented Dec 5, 2024 •
edited
Loading

@cincuranet , so I need to create two issues. First only for sqllite and second for full ef?

@bricelam
Copy link
Contributor

bricelam commented Dec 5, 2024

Yeah, the Microsoft.Data.Sqlite changes here look ready to go to me (FWIW), but there's more to consider on the EF side like value converters, compiled models, migrations, etc.

@newmasterSG newmasterSG mentioned this pull request Dec 9, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cincuranet cincuranet Awaiting requested review from cincuranet

@bricelam bricelam Awaiting requested review from bricelam

Assignees

@cincuranet cincuranet

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Microsoft.Data.Sqlite: Support Half
3 participants
@newmasterSG @cincuranet @bricelam