Maintain referential integrity in SelectExpression and prevent multiple visitations #17621
Conversation
…le visitations Fixes #17337
roji
force-pushed
the
ReferentialIntegrity
branch
from
1319a38 to
c4ed66f
Compare
|
@smitpatel this should work better, take a look. If you think there's a better general approach to this (or want to do it yourself) let me know. |
|
Note: SQL baselines need to be updated, will do this after the general approach is validated. |
| /// If you implement an expression visitor which contains specific logic for visiting <see cref="TableExpressionBase"/>, | ||
| /// you should properly populate and check this field (<see cref="SelectExpression.VisitChildren"/> for an example). | ||
| /// </remarks> | ||
| public TableExpressionBase VisitedExpression { get; set; } |
There was a problem hiding this comment.
No public surface like this. This is just plain wrong.
There was a problem hiding this comment.
I considered this - but we need something that will work for completely external visitors, e.g. provider-added postprocessors (such as the one in the test).
Any specific better ideas? Any feedback on the general approach and on the rest?
There was a problem hiding this comment.
PS How was this solved in the old pipeline? Seems like in the old pipeline SelectExpression.VisitChildren could neither mutate the instance nor return a modified copy...
|
@smitpatel @roji I was looking into this a bit more last night, and I do think this is something we should try to take for ask mode in 3.0. It may get rejected due to risk, but I think the way the complexity can get out of hand means we have to try to fix it. |
|
Closing, I think I understand how to do this in the way we usually do it, i.e. visit projections and other SelectExpression components, replacing the old tables with the new/visited tables before running the actual visitor. This would obviate the need for any additional properties on TableExpressionBase etc. Will submit later today. |
|
@roji - I would suggest deferring this work for now. I believe, what we need to do in "really short term" is already in the other PR. We should take a step back and discuss and come up with a good solution for 3.1, but I don't think there is any reason to rush for it. |
Fixes #17337
@smitpatel, here's an attempt at fixing SelectExpression visitation - not sure if this is the kind of solution you had in mind, but it fixes the perf issue in #17455. Some tests are failing, will work on that if the general approach is OK.