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

Query: Add TableReferenceExpression as a bridge to ColumnExpression for referential integrity #24458

Merged
merged 1 commit into from
Mar 23, 2021
Merged

Query: Add TableReferenceExpression as a bridge to ColumnExpression for referential integrity #24458

merged 1 commit into from
Mar 23, 2021

Conversation

smitpatel
Copy link
Contributor

@smitpatel smitpatel commented Mar 19, 2021 •
edited
Loading

Part of #17337

  • Add TableReferenceExpression
  • SelectExpression contains tableReferenceExpression for each table added
  • TableReferenceExpression can identify the table by querying the selectExpression
  • During pushdown and set operation we update the table references as the selectExpression where columns belong is changing
  • Whenever adding something to selectExpression, we assign unique alises if there is nested selectExpression
  • Improve identifier detection for Distinct/GroupBy cases
  • Pushdown internally gives a visitor to update the expression being added in terms of new column expressions
  • Copy proper identifiers when applying set operation
  • Make dictionary being passed around in query IReadOnlyDictionary
  • Move entityProjectionCache from QueryExpression to ProjectionBindingExpressionVisitor as it is not an internal state for QueryExpression. It existing only in the context of applying particular projection mapping once.
  • Remove TableAliasUniquifyingExpressionVisitor, added debug only (currently disabled) TableAliasVerifyingExpressionVisitor which verifies that all alises are unique and used in order.

@smitpatel smitpatel marked this pull request as draft March 19, 2021 23:11
@smitpatel smitpatel force-pushed the smit/ABriefRevisitToGraphTheory branch from 48b9bc3 to f801da6 Compare March 20, 2021 07:42
@smitpatel smitpatel changed the title Query: Add TableReferenceExpression as a bridge to ColumnExpression f… Query: Add TableReferenceExpression as a bridge to ColumnExpression for referential integrity Mar 20, 2021
@smitpatel smitpatel force-pushed the smit/ABriefRevisitToGraphTheory branch from f801da6 to 1d2cd44 Compare March 20, 2021 07:58
@smitpatel smitpatel force-pushed the smit/ABriefRevisitToGraphTheory branch 3 times, most recently from 7ba85c7 to fb3c5b8 Compare March 22, 2021 20:53
@smitpatel smitpatel marked this pull request as ready for review March 23, 2021 00:22
maumar
maumar reviewed Mar 23, 2021
View reviewed changes
src/EFCore.Relational/Properties/RelationalStrings.resx
<data name="UnableToTranslateSubqueryWithDistinct" xml:space="preserve">
<value>Subquery with 'Distinct' can only be translated if projection consists only of entities and their properties, or it contains keys of all entities required to generate results on the client side. Either add '{column}' to the projection, remove complex elements of the projection, or rewrite the query to not use the 'Distinct' operation.</value>
</data>
<data name="UnableToTranslateSubqueryWithGroupBy" xml:space="preserve">
Copy link
Contributor

Choose a reason for hiding this comment

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

since these are gone, we should probably also update message for InsufficientInformationToIdentifyOuterElementOfCollectionJoin

@smitpatel smitpatel force-pushed the smit/ABriefRevisitToGraphTheory branch from dd7277f to 6e46d5f Compare March 23, 2021 16:06
Base automatically changed from smit/movingaround to main March 23, 2021 16:54
@smitpatel
Query: Add TableReferenceExpression as a bridge to ColumnExpression f…
247ce69 
…or referential integrity

- Add TableReferenceExpression
- SelectExpression contains tableReferenceExpression for each table added
- TableReferenceExpression can identify the table by querying the selectExpression
- During pushdown and set operation we update the table references as the selectExpression where columns belong is changing
- Whenever adding something to selectExpression, we assign unique alises if there is nested selectExpression
- Improve identifier detection for Distinct/GroupBy cases
- Pushdown internally gives a visitor to update the expression being added in terms of new column expressions
- Copy proper identifiers when applying set operation
- Make dictionary being passed around in query IReadOnlyDictionary
- Move entityProjectionCache from QueryExpression to ProjectionBindingExpressionVisitor as it is not an internal state for QueryExpression. It existing only in the context of applying particular projection mapping once.
- Remove TableAliasUniquifyingExpressionVisitor, added debug only (currently disabled) TableAliasVerifyingExpressionVisitor which verifies that all alises are unique and used in order.

Resolves #17337
@smitpatel smitpatel force-pushed the smit/ABriefRevisitToGraphTheory branch from 6e46d5f to 247ce69 Compare March 23, 2021 16:56
@smitpatel smitpatel merged commit 26c0d2a into main Mar 23, 2021
@smitpatel smitpatel deleted the smit/ABriefRevisitToGraphTheory branch March 23, 2021 17:36
@smitpatel smitpatel mentioned this pull request Mar 23, 2021
Closed
@ajcvickers ajcvickers mentioned this pull request Mar 25, 2021
Closed
@sfsharapov
Copy link

Why not to make TableReferenceExpression and ColumnExpression classes public? After these changes EfCore.SqlServer2008Query package could not translate queries as it was worked in fifth version.

@smitpatel
Copy link
Contributor Author

ColumnExpression is still a public class.
As for TableReferenceExpression it is not public it is a transparent wrapper which no one is supposed to access or use other than SelectExpression internally as an implementation detail. If something stops working after this change then that code is probably written incorrectly using wrong set of APIs and worked by chance.

@sfsharapov
Copy link

sfsharapov commented Nov 16, 2021 •
edited
Loading

ColumnExpression is still a public class.

Sorry, I meant ConcreteColumnExpression. ColumnExpression, used before introducing ConcreteColumnExpression, is public.

@smitpatel
Copy link
Contributor Author

Notes same as TableReferenceExpression applies to ConcreteColumnExpression too. It is implementation details and no one needs to know about it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@maumar maumar maumar approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@smitpatel @sfsharapov @maumar