#3836 - Coalescing string concatenation on nullable Expressions #19239
Conversation
This file contains 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
Merge from Base Repo
…nd coalesces all nullable columns/parameters during a string concatenation
svengeance
changed the title
|
@svengeance - Changes look correct. |
|
Good to hear, I'll work on ensuring all broken tests are strictly the result of coalescing during concatenation, fix/commit, and mark the PR as Ready |
…oncat expressions
|
@smitpatel Looks like I got a CI request timeout? How should I go about requesting a rebuild to validate these checks, or is that out of my hands? |
|
@svengeance - I kicked off run again. |
smitpatel
reviewed
Dec 10, 2019
| { | ||
| if (leftNullable) | ||
| { | ||
| newLeft = newLeft is SqlConstantExpression |
There was a problem hiding this comment.
@maumar - Introduce local function for this.
|
@svengeance thank you for the contribution! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Fixes #3836
I was hoping to get a review of the auto-coalescing implementation before I fixed the broken tests (if there's a better way than creating a PR with knowingly broken tests let me know (or if I should have fixed the tests first)). This feature is a slight breaking change if anyone was dependent on return nullability, and it also breaks around 30 or so existing tests due to the difference in output SQL.
Based on comments from @smitpatel on the issue we should be able to handle 3 expressions (column/const/param) in the NullSemanticsVisitor and appropriately coalesce these so we can better emulate expected C# functionality