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

CPP: Prevent forced bad join order which is saved by context. #18828

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

alexet
Copy link
Contributor

@alexet alexet commented Feb 20, 2025

It is impossible to avoid a CP in this predicate on it's own.

Picking getClassAndName first is always a CP as it doesn't use nameWithoutArgs. This disjunct cannot be picked as not method instanceof ConversionOperator requires method to be bound first.

In practice the argument result also happens to be bound at the call site, so getClassAndName is not a CP at the call site. However the call site has no idea that we need method to be bound and we join order assuming that it isn't so it. We also assume it isn't bound internally. This means that the join order is quite sensitive.

An alternative would be to add result to the binding set to get a similar effect to what we do now (I wasn't sure whether to make this do what seems to be intended or whether to preserve the existing join order).

@alexet alexet requested a review from MathiasVP February 20, 2025 16:42
@Copilot Copilot bot review requested due to automatic review settings February 20, 2025 16:42
@alexet alexet requested a review from a team as a code owner February 20, 2025 16:42

Choose a reason for hiding this comment

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

Copilot reviewed 1 out of 1 changed files in this pull request and generated no comments.

Tip: Copilot only keeps its highest confidence comments to reduce noise and keep you focused. Learn more

@github-actions github-actions bot added the C++ label Feb 20, 2025
@alexet alexet added the no-change-note-required This PR does not need a change note label Feb 20, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C++ no-change-note-required This PR does not need a change note
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants