Description
openedon 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.