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

Remove invalid optimization of CASE-WHEN expressions #33754

Merged
merged 3 commits into from
May 21, 2024

Conversation

ranma42
Copy link
Contributor

@ranma42 ranma42 commented May 19, 2024

This PR fixes #33751 by removing the invalid optimization.
Additionally, the CompareTo-specific optimization is extended to handle the CompareTo(x, y) == {-1, 0, 1} case (which was previously handled by the generic optimization) in order to prevent regressions.

@roji roji requested a review from maumar May 19, 2024 07:27
@ranma42
Copy link
Contributor Author

ranma42 commented May 19, 2024

I need some guidance regarding the tests: are they adequate?

  • I am unsure if I added them to the correct place
  • I added tests for the "complex" case (results can be TRUE, FALSE or NULL), under the assumption that they are mainly checking the values rather than the SQL (which I would assume would be more relevant if the PR added/updated the optimization); the issue was originally detected on a WHERE filter, but the optimization can also run in projection contexts, so it should preserve the difference between FALSE and NULL

@maumar
Copy link
Contributor

maumar commented May 20, 2024

@ranma42 thanks for doing the work! For the tests, I would suggest to use NullSemantics model (NullSemanticsQueryFixtureBase, NullSemanticsQueryTestBase) rather than Northwind. Northwind is somewhat special in our tests, given it has particular well-known structure and data, and we try to keep the configuration clean. We do the customization using ITestModelCustomizer, but it's probably overkill for this.

For the tests themselves, I would maybe do one test in select and one in where, so that we test both "simple" and "complex" cases.

@ranma42
Copy link
Contributor Author

ranma42 commented May 21, 2024

I moved the tests to the NullSemanticsQueryTestBase and added tests for the simple (WHERE/filter) case.
Note that the tests do not really involve NULLs or NULL-related semantics/optimizations.

@maumar maumar merged commit e969995 into dotnet:main May 21, 2024
7 checks passed
@maumar
Copy link
Contributor

maumar commented May 21, 2024

@ranma42 thank you for the contribution!

@ranma42 ranma42 deleted the fix-case-when-9 branch May 24, 2024 18: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.

Invalid optimization of CASE-WHEN expressions
2 participants