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

Stop referencing the query from ProjectionBindingExpression in the shaper #32980

Open
Tracked by #34524
roji opened this issue Jan 31, 2024 · 0 comments
Open
Tracked by #34524

Stop referencing the query from ProjectionBindingExpression in the shaper #32980

roji opened this issue Jan 31, 2024 · 0 comments

Comments

@roji
Copy link
Member

roji commented Jan 31, 2024

Our ShapedQueryExpression contains a QueryExpression - representing the server side of the query (e.g. a SelectExpression) - and a ShaperExpression - representing the client side. Currently, the shaper contains references to the QueryExpression (via ProjectionBindingExpression), meaning that our query tree is not a tree, but a graph. When the QueryExpression is replaced via ShapedQueryExpression.UpdateQueryExpression(), this does a recursive visit into the ShaperExpression to update all references to point to the new, replacing query expression.

This situation isn't good: we can have subtle referential integrity issues (like in SqlExpressionSimplifyingExpressionVisitor, see #32979), and making things fully immutable is difficult as references constantly need to be updated. We should stop doing this, and have ProjectionBindingExpression implicitly refer to the QueryExpression of the ShapedQueryExpression in which its located

  • hopefully this really is the way things really work. This may be a bit tricky, e.g. it seems that for final GroupBy we diverge the actual QueryExpression and the SelectExpression referenced by its ShaperExpression for some reason.

Note: this situation is similar to how ColumnExpressions were referencing their TableExpressionBase directly via .NET references (changed in #32812). This also meant that the tree was in fact a graph, that we needed to recursively replace columns in many cases when their table changed, and that we had various bugs due to lost referential integrity.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants