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

[WIP] Concating null with not null string resulting in null #14865

Closed
wants to merge 3 commits into from

Conversation

Muppets
Copy link
Contributor

@Muppets Muppets commented Feb 27, 2019

DO NOT COMMIT THIS - work in progress.

Looking for some feedback on WIP for issue #3836.

My attack plan was to include a Coalesce function call with all string concatenations. This does produce somewhat ugly SQL for a ton of common scenarios. I changed the check to only wrap references to nullable columns in Coalesce but this will miss a ton of different possible outcomes.

Looking for some guidance on what you fancy doing here, uglify everything or only cover some really specific examples?

I've not updated all the unit tests to match this change yet so there are broken tests.

@smitpatel
Copy link
Contributor

  • This work has conflict with New Query Pipeline #14455 . Precisely a lot of this code is going away.
  • string.Concat comes as ExpressionType.Add, in new pipeline we just decided to move it to method call translation.

@smitpatel
Copy link
Contributor

As for the fix, the code would change altogether due to SQL tree being different in new pipeline. We should be able to apply this optimization more smartly once we have null expansion metadata, then for the very specific case of string concat when argument could be nullable, we could apply coalesce.

@Muppets
Copy link
Contributor Author

Muppets commented Mar 2, 2019

Sweet, sounds like the new query pipeline will make this a ton easier. I'll close this out and we can revisit the issue once that is merged.

@Muppets Muppets closed this Mar 2, 2019
@Muppets Muppets deleted the issue3836 branch February 2, 2020 11:59
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.

2 participants