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

Metadata and type mapping support for primitive collections #30730

Closed
Tracked by #30731
roji opened this issue Apr 19, 2023 · 3 comments
Closed
Tracked by #30731

Metadata and type mapping support for primitive collections #30730

roji opened this issue Apr 19, 2023 · 3 comments
Labels
area-model-building area-primitive-collections area-type-mapping closed-fixed The issue has been fixed and is/will be included in the release indicated by the issue milestone. type-enhancement
Milestone

Comments

@roji
Copy link
Member

roji commented Apr 19, 2023

Primitive collection support has been implemented in query, along with basic type mapping support. However, we still need to complete the metadata and type mapping story for this:

  • We're lacking metadata APIs to allow the user to configure the element type in the collection. This includes stuff like setting the store type, the requiredness, etc.
  • Currently, every provider implements its own FindCollectionMapping in its TypeMappingSource, checking whether the CLR type is an IEnumerable, and building and cloning a type mapping for the collection. We may be able to move some common logic to core/relational.
    • In fact, we should consider mapping collections to JSON by default, without the provider needing to actually do anything. Note that this means storage only - no querying (which requires provider-specific logic).
  • When doing type inference for constant and parameter query roots, we detect the type mapping of their elements based on later usage, and need to resolve the collection type mapping for that element mapping. This is currently done by instantiating a new type mapping instance, in the provider's type inference code - not great. We should be able to find collection type mapping via TypeMappingSource by providing the collection CLR type and element type mapping.
    • Once this is done, make the type converter comparison a simple reference comparison again rather than an Equals comparison, see QuerySqlGenerator.VisitSqlParameter.

See related #30677, which is about add APIs for how JSON serialization and value extraction should happen.

@AndriySvyryd
Copy link
Member

Related - #17471

@roji
Copy link
Member Author

roji commented Apr 24, 2023

Think specifically about precedence of primitive collections and value converters: if the provider's FindMapping returns primitive collections (as it currently does in #30738), these will override any value converters. If we have any built-in value converters from IEnumerable types, these would be overridden by the new support; but I'm not sure we have any.

Note that byte[] specifically isn't affected, since relational providers already map it as a regular type (so before we even attempt to map it as a collection). The same happens obviously with string which is also IEnumerable.

/cc @ajcvickers

roji added a commit to roji/efcore that referenced this issue Apr 26, 2023
Mostly completes dotnet#30724, except for full type inference is blocking on dotnet#30730.
roji added a commit to roji/efcore that referenced this issue Apr 26, 2023
Mostly completes dotnet#30724, except for full type inference is blocking on dotnet#30730.
roji added a commit to roji/efcore that referenced this issue Apr 26, 2023
Mostly completes dotnet#30724, except for full type inference is blocking on dotnet#30730.
roji added a commit to roji/efcore that referenced this issue Apr 26, 2023
Mostly completes dotnet#30724, except for full type inference is blocking on dotnet#30730.
roji added a commit to roji/efcore that referenced this issue Apr 26, 2023
Mostly completes dotnet#30724, except for full type inference is blocking on dotnet#30730.
roji added a commit to roji/efcore that referenced this issue Apr 27, 2023
Mostly completes dotnet#30724, except for full type inference is blocking on dotnet#30730.
ghost pushed a commit that referenced this issue Apr 27, 2023
Mostly completes #30724, except for full type inference is blocking on #30730.
@roji
Copy link
Member Author

roji commented May 16, 2023

Test value converters on the element type mapping, e.g. allowing mapping List<MyPoco> to a JSON string array if a converter is configured from MyPoco to string.

ajcvickers added a commit that referenced this issue Jun 2, 2023
Part of #30730

- Type mappings now have a `JsonValueReaderWriter` that handles reading and writing JSON for the type.
- This can be overridden by a `JsonValueReaderWriter` on a property, for example, to allow GeoJson to be written instead of WKT.
- These are typically singleton instances, and can be optimized out for well-known cases.
- If the type mapping has a converter, then the converter must be applied to the value before being written to JSON, and the converter must be applied after reading the JSON value.
- If the property itself has a `JsonValueReaderWriter`, then the unconverted property value is passed to this reader/writer.
ajcvickers added a commit that referenced this issue Jun 12, 2023
Part of #30730

- Type mappings now have a `JsonValueReaderWriter` that handles reading and writing JSON for the type.
- This can be overridden by a `JsonValueReaderWriter` on a property, for example, to allow GeoJson to be written instead of WKT.
- These are typically singleton instances, and can be optimized out for well-known cases.
- If the type mapping has a converter, then the converter must be applied to the value before being written to JSON, and the converter must be applied after reading the JSON value.
- If the property itself has a `JsonValueReaderWriter`, then the unconverted property value is passed to this reader/writer.
ajcvickers added a commit that referenced this issue Jul 25, 2023
Part of #30730

Some stuff remaining, but wanted to get this out there for initial review.

Missing:
- ElementType in compiled model
- Negative cases
- More tests
ajcvickers added a commit that referenced this issue Jul 26, 2023
Part of #30730

Some stuff remaining, but wanted to get this out there for initial review.

Missing:
- ElementType in compiled model
- Negative cases
- More tests
ajcvickers added a commit that referenced this issue Jul 30, 2023
Part of #30730

Some stuff remaining, but wanted to get this out there for initial review.

Missing:
- ElementType in compiled model
- Negative cases
- More tests
ajcvickers added a commit that referenced this issue Aug 1, 2023
Part of #30730

Some stuff remaining, but wanted to get this out there for initial review.

Missing:
- ElementType in compiled model
- Negative cases
- More tests
ajcvickers added a commit that referenced this issue Aug 6, 2023
Part of #30730

Some stuff remaining, but wanted to get this out there for initial review.

Missing:
- ElementType in compiled model
- Negative cases
- More tests
ajcvickers added a commit that referenced this issue Aug 9, 2023
Part of #30730

Some stuff remaining, but wanted to get this out there for initial review.

Missing:
- ElementType in compiled model
- Negative cases
- More tests

Review updates

Change builder shape based on API review

Updated based on review.

More tests and refactoring of how ElementTypes are created.
ajcvickers added a commit that referenced this issue Aug 10, 2023
Part of #30730

Some stuff remaining, but wanted to get this out there for initial review.

Missing:
- ElementType in compiled model
- Negative cases
- More tests

Review updates

Change builder shape based on API review

Updated based on review.

More tests and refactoring of how ElementTypes are created.
@ajcvickers ajcvickers added closed-fixed The issue has been fixed and is/will be included in the release indicated by the issue milestone. and removed needs-design labels Aug 19, 2023
@ajcvickers ajcvickers modified the milestones: 8.0.0, 8.0.0-rc1 Aug 19, 2023
@ajcvickers ajcvickers modified the milestones: 8.0.0-rc1, 8.0.0 Nov 14, 2023
@ajcvickers ajcvickers removed their assignment Aug 31, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-model-building area-primitive-collections area-type-mapping closed-fixed The issue has been fixed and is/will be included in the release indicated by the issue milestone. type-enhancement
Projects
None yet
Development

No branches or pull requests

3 participants