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

Draft: Mapping a single property to multiple columns #24909

Closed
wants to merge 1 commit into from

Conversation

ajcvickers
Copy link
Contributor

Part of #13947

The idea here is that a property can contain pseudo-properties that generally don't act like properties in the model, but form the basis for mapping to a column after extracting a value from the outer property.

Why properties? Because they have all the metadata needed to allow the facets of columns to be configured?

Why not relational only? Cosmos won't use this, but in-memory will be able to understand it.

Part of #13947

The idea here is that a property can contain pseudo-properties that generally don't act like properties in the model, but form the basis for mapping to a column after extracting a value from the outer property.

Why properties? Because they have all the metadata needed to allow the facets of columns to be configured?

Why not relational only? Cosmos won't use this, but in-memory will be able to understand it.
@ajcvickers ajcvickers marked this pull request as draft May 14, 2021 22:12
@ajcvickers ajcvickers requested a review from AndriySvyryd May 14, 2021 22:12
@ajcvickers
Copy link
Contributor Author

@AndriySvyryd This is mostly for you to look at and comment on whether this is going in a reasonable direction. Lots still left to do, but EnsureCreated and SaveChanges works for the example in SqlServerEndToEndTest.

Comment on lines +812 to +813
? ((IPseudoProperty)propertyBase).ValueExtractor(
ReadPropertyValue(((IPseudoProperty)propertyBase).OuterProperty))
Copy link
Member

Choose a reason for hiding this comment

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

Why not repurpose the Getter?

Suggested change
? ((IPseudoProperty)propertyBase).ValueExtractor(
ReadPropertyValue(((IPseudoProperty)propertyBase).OuterProperty))
? propertyBase.GetGetter().GetClrValue(ReadPropertyValue(((IPseudoProperty)propertyBase).OuterProperty))

Copy link
Member

@AndriySvyryd AndriySvyryd May 15, 2021

Choose a reason for hiding this comment

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

How will the shadow pseudo properties be handled?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think shadow pseudo properties are a thing. A shadow property has storage in the state manager and no storage in the CLR type. In this case, the property has no storage of it's own because the value doesn't exist in this form in application code at all.

Copy link
Member

@AndriySvyryd AndriySvyryd May 15, 2021

Choose a reason for hiding this comment

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

Yes, the outer property is basically a complex property if the CLR type contains properties that can be used for pseudo-properties. If the outer type is something like a string then the pseudo-properties don't map to a CLR property of their owns but still only keep state in the entity object. We might need a different name for this.

/// Once the model is built, <see cref="IProperty" /> represents a read-only view of the same metadata.
/// </para>
/// </summary>
public interface IReadOnlyPseudoProperty : IReadOnlyProperty, IPseudoProperty
Copy link
Member

Choose a reason for hiding this comment

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

This interface seems unnecessary. A property is a pseudo property if OuterProperty is not null

Copy link
Contributor Author

Choose a reason for hiding this comment

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

At the moment each pseudo-property will have three additional references:

  • The outer property
  • The value extractor delegate
  • The value extractor expression

Do you think it's better to have these three references available but null on every property, rather than introduce a new type? (I'm very much on the fence.)

Copy link
Member

Choose a reason for hiding this comment

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

Yes, though value extractors might not be needed per my previous comment

@AndriySvyryd
Copy link
Member

Why not relational only? Cosmos won't use this, but in-memory will be able to understand it.

Cosmos will use this.

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.

3 participants