[release/7.0] Fix to #29638 - GroupBy generates invalid SQL when using custom database function - #31743
Merged
Conversation
Contributor
Author
|
@ajcvickers to decide if we should go ahead with this for patching |
…ase function Problem is that CloningExpressionVisitor doesn't have proper handling for TableValuedFunctionExpression, and therefore goes through default expression visitor pattern (visit all children, check if there are any changes, if there are return new, if not return the same). Since there are no changes, the same instance is returned from cloning, and causes the problem. Fix is to add proper handling of TVFExpression in the CloningExpressionVisitor so that it produces a proper copy. Fixes #29638
roji
approved these changes
Sep 14, 2023
Contributor
|
@maumar Let's discuss next triage whether to service. If we decide to do this in 7, then we should also consider 6. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Port of #30617
Fixes #29638
Description
Problem is that CloningExpressionVisitor doesn't have proper handling for TableValuedFunctionExpression, and therefore goes through default expression visitor pattern (visit all children, check if there are any changes, if there are return new, if not return the same). Since there are no changes, the same instance is returned from cloning, and causes the problem. Fix is to add proper handling of TVFExpression in the CloningExpressionVisitor so that it produces a proper copy.
Customer impact
Incorrect sql generated for queries using group by with custom database function. We generate incorrect aliases so is some cases data corruption is possible, although unlikely.
How found
Customer report on 7.0
Regression
No. The scenario also fails on 6.0
Testing
Added regression tests for affected scenarios.
Risk
Small: Fix potentially affects all scenarios that use clone mechanism on a TVF. The previous cloning behavior for TVF was incorrect, never generating a close but returning the same instance instead. The fix is not only isolated to the group by, so slightly higher risk than other, super isolated cases EF query patch fixes, but the chance of breaking other scenarios is low. We would have to rely on incorrect cloning behavior to begin with. Also added quirk to revert to old behavior if necessary.