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

DDC-2076 Remove useless join over target table of ManyToMany relationship #8438

Merged
merged 1 commit into from
Feb 28, 2021

Conversation

plfort
Copy link
Contributor

@plfort plfort commented Jan 25, 2021

Quite an old issue : #2758

I noticed this additional "join" while trying to optimize a query. As mentioned in the issue, I don't see a case when this join is useful.
In my case (query optimization), this PR provides a significant reduction of the query cost given by Postgresql

SenseException
SenseException previously approved these changes Jan 27, 2021
Copy link
Member

@SenseException SenseException left a comment

Choose a reason for hiding this comment

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

Looks good to me. Maybe SqlWalker need some integration tests, but I'm not sure if this should be done in the scope of this PR.

@beberlei
Copy link
Member

beberlei commented Feb 5, 2021

@SenseException The SelectSqlGenerationTest is the integration test for SqlWalker, so this is fine.

I think the patch is good, thanks.

unrelated however I realized this must provide wrong results on composite keys or not? It seems weird to iterate the columns and add multiple in conditions, maybe I am wrong.

beberlei
beberlei previously approved these changes Feb 5, 2021
@beberlei beberlei added this to the 2.8.2 milestone Feb 5, 2021
@beberlei beberlei dismissed their stale review February 5, 2021 23:18

found a bug

beberlei
beberlei previously approved these changes Feb 5, 2021
@beberlei beberlei modified the milestones: 2.8.2, 2.9.0 Feb 5, 2021
@beberlei
Copy link
Member

beberlei commented Feb 5, 2021

@plfort i am a bit hesitant to merge a cosmetic PR into a branch in bugfix mode, the old code works, so this is an improvement and changing the SQL is a bit more risky, so I would like you to rebase this on 2.9 and we can merge it there.

@plfort plfort dismissed stale reviews from beberlei and SenseException via c75ce1e February 7, 2021 18:01
@plfort plfort changed the base branch from 2.8.x to 2.9.x February 7, 2021 18:02
@plfort
Copy link
Contributor Author

plfort commented Feb 7, 2021

I agree with you about composite keys, it requires further tests. I thought about it but for this change I wanted to keep the behaviour.

@plfort
Copy link
Contributor Author

plfort commented Feb 23, 2021

Rebase on 2.9 done BTW

@beberlei beberlei merged commit 38ccbd8 into doctrine:2.9.x Feb 28, 2021
@rvanlaak
Copy link
Contributor

if this PR did fix #2758, that issue can get closed now

@plfort plfort deleted the member_of_manytomany_optim branch May 24, 2021 16:51
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.

4 participants