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

Fix to #32911 - ComplexProperty with AsSplitQuery #33020

Merged
merged 1 commit into from
Feb 13, 2024

Conversation

maumar
Copy link
Contributor

@maumar maumar commented Feb 7, 2024

Problem was that when we lift structural type projection during pushdown, if that projection contains complex types with the same column names (e.g. cross join of same entities - it's perfectly fine to do in a vacuum, we just alias the columns whose names are repeated) we would lift the projection incorrectly. What we do is go through all the properties, apply the corresponding columns to the selectExpression if needed and generate StructuralTypeProjection object if the projection needs to be applied one level up. For complex types we would generate a shaper expression and then run it through the same process, BUT the nested complex properties would be added to a flat structure along with the primitive properties, rather than in separate cache dedicated for complex property shapers. This was wrong and not what we expected to see, when processing this structure one level up (i.e. when applying projection to the outer select)

SELECT [applying_this_projection_was_wrong]
FROM
(
    SELECT c.Id, c.Name, c.ComplexProp, o.ComplexProp as ComplexProp0
    FROM Customers as c
    JOIN Orders as o ON ...
) as s

i.e. applying projection once worked fine, but doing it second time did not. The reason why is that we expected to see information about the complex type shape in the complex property shaper cache, rather than flat structure for primitives, but it wasn't there. So we assumed this is the first time we the projection is being applied, so we conjure up the complex type shaper based on table alias and IColumn metadata. This results in a situation, where complex property that was aliased is never picked. So we end up with:

SELECT s.Id, s.Name, s.ComplexProp -- we would also try to add s.ComplexProp again, instead of s.ComplexProp0 but of course we don't add same thing twice FROM
(
    SELECT c.Id, c.Name, c.ComplexProp, o.ComplexProp as ComplexProp0
    FROM Customers as c
    JOIN Orders as o ON ...
) as s

This leads to bad data - two different objects with distinct data in them are mapped to the same column in the database.

Fix is to property build a complex type shaper structure when applying projection instead, so the structure we generate matches expectations.

Also modified VisitChildren and MakeNullable methods on StructuralTypeProjectionExpression to process/preserve complex type cache information, which was previously gobbled up/ignored.

Fixes #32911

@maumar maumar requested a review from roji February 7, 2024 11:34

var tableMap = projection1.TableMap.ToDictionary(kvp => kvp.Key, _ => setOperationAlias);
var resultComplexShaper = new RelationalStructuralTypeShaperExpression(
Copy link
Contributor Author

Choose a reason for hiding this comment

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

or should we just do complexPropertyShaper1.Update(resultComplexProjection) ?

@maumar maumar marked this pull request as draft February 8, 2024 01:52
@maumar maumar force-pushed the fix32911_finally_got_it_maybe branch from d12d53f to 3c556ea Compare February 8, 2024 04:46
@@ -124,6 +151,18 @@ protected override Expression VisitChildren(ExpressionVisitor visitor)
propertyExpressionMap[property] = newExpression;
}

var complexPropertyCache = default(Dictionary<IComplexProperty, StructuralTypeShaperExpression>);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this is a bit scary @roji

@maumar maumar marked this pull request as ready for review February 8, 2024 05:01
Problem was that when we lift structural type projection during pushdown, if that projection contains complex types with the same column names (e.g. cross join of same entities - it's perfectly fine to do in a vacuum, we just alias the columns whose names are repeated) we would lift the projection incorrectly.
What we do is go through all the properties, apply the corresponding columns to the selectExpression if needed and generate StructuralTypeProjection object if the projection needs to be applied one level up. For complex types we would generate a shaper expression and then run it through the same process, BUT the nested complex properties would be added to a flat structure along with the primitive properties, rather than in separate cache dedicated for complex property shapers.
This was wrong and not what we expected to see, when processing this structure one level up (i.e. when applying projection to the outer select)

SELECT [applying_this_projection_was_wrong]
FROM
(
    SELECT c.Id, c.Name, c.ComplexProp, o.ComplexProp as ComplexProp0
    FROM Customers as c
    JOIN Orders as o ON ...
) as s

i.e. applying projection once worked fine, but doing it second time did not. The reason why is that we expected to see information about the complex type shape in the complex property shaper cache, rather than flat structure for primitives, but it wasn't there. So we assumed this is the first time we the projection is being applied, so we conjure up the complex type shaper based on table alias and IColumn metadata. This results in a situation, where complex property that was aliased is never picked. So we end up with:

SELECT s.Id, s.Name, s.ComplexProp -- we would also try to add s.ComplexProp again, instead of s.ComplexProp0 but of course we don't add same thing twice
FROM
(
    SELECT c.Id, c.Name, c.ComplexProp, o.ComplexProp as ComplexProp0
    FROM Customers as c
    JOIN Orders as o ON ...
) as s

This leads to bad data - two different objects with distinct data in them are mapped to the same column in the database.

Fix is to property build a complex type shaper structure when applying projection instead, so the structure we generate matches expectations. Also modified VisitChildren and MakeNullable methods on StructuralTypeProjectionExpression to process/preserve complex type cache information, which was previously gobbled up/ignored.

Fixes #32911
@maumar maumar force-pushed the fix32911_finally_got_it_maybe branch from 3c556ea to 9b31af9 Compare February 8, 2024 06:54
@roji
Copy link
Member

roji commented Feb 8, 2024

@maumar wrote some thoughts offline, let's discuss.

Copy link
Member

@roji roji left a comment

Choose a reason for hiding this comment

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

As discussed offline, this seems like an OK fix. The one possible issue is the very eager recursive generation of the entire complex type tree, which isn't great perf-wise; ideally we'd only generate shapers more lazily, as they are being bound in the query (as we do for (non-JSON) owned entity types).

But it seems like this adds considerable complexity, so let's go with this for now - we can revisit this when we work on complex type JSON mapping. I also have some more extreme query architecture thoughts about doing things simpler (fully separate query from shaper side, have the query always have its projection populated, and reference columns in that projection by position from the shaper). If that happens we'll redo most of this anyway.

Thanks for working on this @maumar - this was my design fail for complex types in 8.0...

@maumar maumar merged commit 42e6cfb into main Feb 13, 2024
7 checks passed
@maumar maumar deleted the fix32911_finally_got_it_maybe branch February 13, 2024 17:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Incorrect apply projection for complex properties
2 participants