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

Rewrite SearchConditionConverter #34905

Merged
merged 2 commits into from
Oct 16, 2024
Merged

Rewrite SearchConditionConverter #34905

merged 2 commits into from
Oct 16, 2024

Conversation

roji
Copy link
Member

@roji roji commented Oct 14, 2024

Our current SearchConditionConvertingExpressionVisitor has the following design issues:

  • It relies on visitor state to track whether the current node is within search condition context (e.g. WHERE) or not.
  • Because of this, each and every expression type has to be handled explicitly, e.g. there's a VisitAtTimeZone() which duplicates the generate visitation logic from AtTimeZoneExpression.VisitChildren() - only so that _isSearchCondition can be set to false before visiting (and set back at the end).
  • This means that every time an expression type is added, SearchConditionConvertingExpressionVisitor must be updated to support it, and duplicates substantial logic (all the expression's VisitChildren()) which must be properly synced/maintained.

This rewrite changes the design as follows:

  • No more visitor state; the context (are we in a search condition?) is simply passed via method parameters, and handled generically in Visit().
  • Expression types where handling needs to care about the context has methods (e.g. VisitSqlBinary) which accept the context as parameter, passing it down as needed. For all other expression types, regular visitation via VisitChildren is used; this cuts down all the duplicate logic, requiring code only for the very specific expression types we need to care about.
  • Various other code cleanups and simplifications.

This is similar to #34697, which changed SqlNullabillityProcessor into a real visitor, allowing VisitChildren to be used (for non-SqlExpression expressions).

/cc @ranma42

@roji roji requested review from maumar and cincuranet October 14, 2024 17:44
@roji roji marked this pull request as ready for review October 14, 2024 18:23
@roji roji merged commit 213d37b into dotnet:main Oct 16, 2024
7 checks passed
@roji roji deleted the SearchCondition branch October 16, 2024 20:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants