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

[release/8.0] Fix to #31961 - Projection filtering on second-level reference could not be translated #31996

Merged
merged 1 commit into from
Oct 19, 2023

Conversation

maumar
Copy link
Contributor

@maumar maumar commented Oct 9, 2023

Problem was that optimizing visitor was looking for a very specific pattern (member access over conditional), but in this scenario conditional is wrapped around Convert node. Fix is to recognize this case also, strip convert around the conditional and apply it around non-null portion instead, before applying the member access over it.

Fixes #31961

Risk

Low - change is isolated to a narrow optimization (member access over conditional) that also uses type conversion (either from derived to base type or from non-nullable to nullable), and is a simple change (strip convert around convert node and apply it on the non-null part of it before applying the member access.

@maumar maumar changed the title Fix to #31961 - Projection filtering on second-level reference could not be translated [release/8.0] Fix to #31961 - Projection filtering on second-level reference could not be translated Oct 9, 2023
@maumar
Copy link
Contributor Author

maumar commented Oct 11, 2023

@roji could you take a look?

@ajcvickers
Copy link
Member

@roji Can you take a look at this?

@ajcvickers ajcvickers requested a review from roji October 17, 2023 09:02
Copy link
Member

@roji roji left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, see comments below on the Convert node preservation.

Looking at this more generally, the optimization seems to make quite an effort to apply only when the conditional test is a comparison to null; but I'm not sure why that is - couldn't this be generalized?

i.e. instead of only applying to:

(a != null ? new { Member = b, ... } : null).Member

... couldn't this apply to any test expression:

(<arbitrary expression> ? new { Member = b, ... } : null).Member

... transforming it to:

<arbitrary expression> ? b : null

(this would obviously be outside the scope of this PR)

…not be translated

Problem was that optimizing visitor was looking for a very specific pattern (member access over conditional), but in this scenario conditional is wrapped around Convert node.
Fix is to recognize this case also, strip convert around the conditional and apply it around non-null portion instead, before applying the member access over it.

Fixes #31961
@maumar
Copy link
Contributor Author

maumar commented Oct 18, 2023

@roji we could try the general purpose optimization, although idk if we wouldn't be losing some context by doing it. This optimization is not mathematically correct on the Core level - we should be pushing down the member to both sides of the conditional expression. However, in case of x != null ? x.Member : null we recognize this as common patten to guard against null refs, so we take some liberty here. Either way, it's currently out of scope for sure.

@maumar maumar merged commit 50ee18e into release/8.0 Oct 19, 2023
7 checks passed
@maumar maumar deleted the fix31961 branch October 19, 2023 00:04
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.

Projection filtering on second-level reference could not be translated
4 participants