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

Make our SelectExpression cloning mechanism compatible with external roots #30982

Closed
roji opened this issue May 27, 2023 · 1 comment · Fixed by #32665
Closed

Make our SelectExpression cloning mechanism compatible with external roots #30982

roji opened this issue May 27, 2023 · 1 comment · Fixed by #32665
Assignees
Labels
area-query closed-fixed The issue has been fixed and is/will be included in the release indicated by the issue milestone. type-bug
Milestone

Comments

@roji
Copy link
Member

roji commented May 27, 2023

We have a CloningExpressionVisitor which is used to deep-clone a SelectExpression; it knows about some specific expression types (SelectExpression, TpcTablesExpression, TableValuedFunctionExpression), and also recognizes IClonableTableExpressionBase, which table expressions can implement in order to have their own cloning logic.

However, since IClonableTableExpressionBase.Clone() doesn't accept the cloning visitor itself, it's impossible for such custom cloning implementations to recurse into their contained sub-expressions. For example, ValuesExpression performs a shallow copy of itself, meaning that the copy references the same elements as the original. Also, SqlServerOpenJsonExpression gets cloned as a TableValuedExpression (which it extends), but the extra data it adds doesn't get cloned; it could implement IClonableTableExpressionBase, but would then again have to implement shallow copying.

We can pass the cloning visitor as a parameter to Clone, so that implementations can deep-copy.

@roji
Copy link
Member Author

roji commented May 27, 2023

Note that this is important only if a SelectExpression is contained within a custom TableExpression, since regular expressions are immutable and aren't cloned anyway. This is probably a rare edge case situation, at least at the moment.

@ajcvickers ajcvickers added this to the Backlog milestone Jun 7, 2023
roji added a commit to roji/efcore that referenced this issue Dec 24, 2023
@roji roji self-assigned this Dec 24, 2023
@roji roji modified the milestones: Backlog, 9.0.0 Dec 24, 2023
@roji roji added the closed-fixed The issue has been fixed and is/will be included in the release indicated by the issue milestone. label Dec 24, 2023
roji added a commit to roji/efcore that referenced this issue Dec 24, 2023
roji added a commit to roji/efcore that referenced this issue Jan 1, 2024
roji added a commit that referenced this issue Jan 2, 2024
@ajcvickers ajcvickers modified the milestones: 9.0.0, 9.0.0-preview1 Jan 31, 2024
@roji roji modified the milestones: 9.0.0-preview1, 9.0.0 Oct 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-query closed-fixed The issue has been fixed and is/will be included in the release indicated by the issue milestone. type-bug
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants