Skip to content

Add Half type support for SQLite (#30931)#37481

Open
kimjaejung96 wants to merge 3 commits intodotnet:mainfrom
kimjaejung96:users/kimjaejung96/half-type-sqlite-30931
Open

Add Half type support for SQLite (#30931)#37481
kimjaejung96 wants to merge 3 commits intodotnet:mainfrom
kimjaejung96:users/kimjaejung96/half-type-sqlite-30931

Conversation

@kimjaejung96
Copy link

@kimjaejung96 kimjaejung96 commented Jan 11, 2026

  • 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:
  • Tests for the changes have been added (for bug fixes / features)
  • Code follows the same patterns and style as existing code in this repo

Summary

Add Half (16-bit floating point) type support for Microsoft.Data.Sqlite and EF Core SQLite provider.

Fixes #30931

Changes

Microsoft.Data.Sqlite (ADO.NET layer)

File Change
SqliteValueReader.cs Add GetFieldValue<Half> support
SqliteValueBinder.cs Add Half binding + type mapping

EF Core SQLite Provider

File Change
SqliteHalfTypeMapping.cs New - SQLite-specific type mapping
SqliteTypeMappingSource.cs Register Half mapping

Tests

File Change
SqliteDataReaderTest.cs Half read/write tests
SqliteTypeMappingSourceTest.cs CLR/Store type mapping tests (including UNREALISTIC)
SqliteTypeMappingTest.cs SQL literal generation + clone test

Design Notes

Based on #35328 feedback:

  1. SQLite-specific only - No changes to EFCore.Relational (no HalfTypeMapping in common layer)
  2. No DbType - System.Data.DbType has no Half value, so DbType is null
  3. Float pattern - Follows existing float implementation (read via GetFloat(), bind via BindDouble())
  4. Conditional compilation - Uses #if NET6_0_OR_GREATER for netstandard2.0 compatibility

@kimjaejung96
Add Half type support for SQLite
37c25c6 
- Add Half support in SqliteValueReader.GetFieldValue<T>
- Add Half binding in SqliteValueBinder
- Add SqliteHalfTypeMapping for EF Core SQLite provider
- SQLite-specific implementation only (per maintainer feedback)
- Use REAL store type, DbType is null (no DbType.Half exists)

Fixes dotnet#30931
@kimjaejung96 kimjaejung96 requested a review from a team as a code owner January 11, 2026 06:04
bricelam
bricelam reviewed Jan 12, 2026
View reviewed changes
Copy link
Contributor

@bricelam bricelam left a comment

Choose a reason for hiding this comment

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

Looking good to me.

You should also add logic for Half to SqliteDatabaseModelFactory.

src/Microsoft.Data.Sqlite.Core/SqliteValueReader.cs Outdated
#if NET6_0_OR_GREATER
if (typeof(T) == typeof(Half))
{
return (T)(object)(Half)GetFloat(ordinal);
Copy link
Contributor

@bricelam bricelam Jan 12, 2026

Choose a reason for hiding this comment

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

Either add GetHalf, or use GetDouble here to avoid extra conversions.

Copy link
Contributor

@bricelam bricelam Jan 12, 2026

Choose a reason for hiding this comment

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

(below too)

@AndriySvyryd
Copy link
Member

AndriySvyryd commented Jan 12, 2026

For reference, the previous attempt was #35328

@kimjaejung96
Address PR review feedback
10d5a20 
- Use GetDouble instead of GetFloat for Half reading (avoids extra conversion)
- Add Half scaffolding support to SqliteDatabaseModelFactory
  - Add HALF, FLOAT16 type hints for Half CLR type mapping
  - Add Half range validation in REAL value type handling
  - Add Half default value parsing
@kimjaejung96
Copy link
Author

kimjaejung96 commented Jan 12, 2026

Thanks again @bricelam — I pushed updates addressing the feedback:

  • SqliteValueReader: Half now uses GetDouble
  • Added Half handling to SqliteDatabaseModelFactory for scaffolding

(Also, thanks to AndriySvyryd for pointing me to the previous attempt.)

bricelam
bricelam approved these changes Jan 13, 2026
View reviewed changes
Copy link
Contributor

@bricelam bricelam left a comment

Choose a reason for hiding this comment

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

LGTM

src/EFCore.Sqlite.Core/Scaffolding/Internal/SqliteDatabaseModelFactory.cs Outdated

private static readonly HashSet<string> _floatTypes = new(StringComparer.OrdinalIgnoreCase) { "SINGLE" };

private static readonly HashSet<string> _halfTypes = new(StringComparer.OrdinalIgnoreCase) { "HALF", "FLOAT16" };
Copy link
Contributor

@bricelam bricelam Jan 13, 2026

Choose a reason for hiding this comment

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

FLOAT16 seems inconsistent with the other floating-point types. I'd go with just HALF.

Copy link
Contributor

@bricelam bricelam Jan 13, 2026 •
edited
Loading

Choose a reason for hiding this comment

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

BTW, the philosophy behind these names was to use names specified in the SQL standard, if available, or at least one generic/.NET name to allow some way of preserving the .NET type in the schema. But, the default EF Core mapping will always adhere to the STRICT rules.

Copy link
Author

@kimjaejung96 kimjaejung96 Jan 14, 2026

Choose a reason for hiding this comment

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

Thx !
Updated SqliteDatabaseModelFactory to drop FLOAT16 and keep only HALF as the explicit type hint (per your suggestion).

Copy link
Author

@kimjaejung96 kimjaejung96 Jan 17, 2026

Choose a reason for hiding this comment

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

@roji Hi! I addressed the last feedback (removed FLOAT16 and kept only HALF). Could you please take another look and merge when you have a chance? Thanks!

Thanks again @bricelam for the review!

Copy link
Author

@kimjaejung96 kimjaejung96 Jan 22, 2026

Choose a reason for hiding this comment

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

Hi. @roji All checks are green and the PR has an approving review. Could you check when you have a moment? Thanks!

@roji roji force-pushed the main branch 2 times, most recently from 249ae47 to 6b86657 Compare January 13, 2026 17:46
@kimjaejung96
Remove FLOAT16 from Half type hints
f6d7298 
Per reviewer feedback, keep only HALF for consistency with other
floating-point type hints (SINGLE for float).
@kimjaejung96 kimjaejung96 requested a review from bricelam January 17, 2026 01:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@bricelam bricelam bricelam approved these changes

Reviewers whose approvals may not affect merge requirements

At least 1 approving review is required to merge this pull request.

Assignees

@roji roji

Labels

community-contribution

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Microsoft.Data.Sqlite: Support Half

4 participants

@kimjaejung96 @AndriySvyryd @bricelam @roji