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

Fix to #18916 - Query: regression in EFCore 3.1 Preview 2 and Preview3 for query with conditional expression whose element is a comparison containing null-value parameter #18937

Merged
merged 1 commit into from
Nov 26, 2019

Conversation

maumar
Copy link
Contributor

@maumar maumar commented Nov 15, 2019

Problem was that now we are peeking into parameter values and simplifying the queries. As a result comparisons of non-nullable expressions with null-value parameter are optimized out to a simple constant false. Sql server expects search condition in some places (e.g. WHERE, HAVING or WHEN parts of CASE statements).
Before we look into parameter values we run SearchConditionConvertingExpressionVisitor which fixes those cases. However, this visitor is currently sql server specific, and the part of the pipeline that looks into parameter values is not provider specific, so we can't re-run it.

Fix is to manually convert constants encountered in those areas into conditions. We used to do it already for WHERE and HAVING but we missed WHEN.

Resolves #18916

@maumar maumar requested a review from smitpatel November 15, 2019 21:27
@maumar maumar force-pushed the fix18916 branch 3 times, most recently from 736df95 to 38fe34d Compare November 15, 2019 21:44
@smitpatel
Copy link
Contributor

This has build check failures. Can you investigate?

@maumar
Copy link
Contributor Author

maumar commented Nov 19, 2019

Customer Impact
Query failure (invalid sql) when using conditional expression when result expression is a comparison with null-valued parameter.

How found
Customer-reported (#18916)

Test coverage
We did not have tests when all necessary conditions were met.

Regression?
Yes, from 3.0.

Risk
Risk is very small - fix is very targeted and we already apply the same logic in two other places. Just missed the 3rd case where it was needed.

Workaround:
Workaround is to rewrite the query so that comparison with null-value parameter is not happening in the result of case expression. Workaround is relatively simple to implement but not intuitive. Alternatively, 2 separate queries can be written, one for prm2 being null and one when its not null.

original query:

var prm1 = true;
var prm2 = default(string);
var query = customers.Where(c => c.IsVip == true ? true : c.Name == prm2);

workaround:

var prm1 = true;
var prm2 = default(string);
var query = customers.Where(c => c.IsVip == true ? true : prm2 == null ? c.Name == null : c.Name == prm2);

@maumar
Copy link
Contributor Author

maumar commented Nov 19, 2019

@Pilchie @ajcvickers

… 3 for query with conditional expression whose element is a comparison containing null-value parameter

Problem was that now we are peeking into parameter values and simplifying the queries. As a result comparisons of non-nullable expressions with null-value parameter are optimized out to a simple constant false. Sql server expects search condition in some places (e.g. WHERE, HAVING or WHEN parts of CASE statements).
Before we look into parameter values we run SearchConditionConvertingExpressionVisitor which fixes those cases. However, this visitor is currently sql server specific, and the part of the pipeline that looks into parameter values is not provider specific, so we can't re-run it.

Fix is to manually convert constants encountered in those areas into conditions. We used to do it already for WHERE and HAVING but we missed WHEN.

Resolves #18916
@ajcvickers
Copy link
Contributor

@Pilchie @wtgodbe @dougbu Can this now be merged for 3.1.1?

@wtgodbe wtgodbe merged commit 951aca3 into release/3.1 Nov 26, 2019
@wtgodbe wtgodbe deleted the fix18916 branch November 26, 2019 19:12
@ajcvickers ajcvickers removed this from the 3.1.x milestone Nov 26, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants