[DDC-1256] Fix applying ON/WITH conditions to first join in Class Table Inheritance #886

Merged
merged 6 commits into from Feb 17, 2014

Projects

None yet

6 participants

@Strate
Contributor
Strate commented Dec 22, 2013

Some related issues:
http://www.doctrine-project.org/jira/browse/DDC-1256
http://www.doctrine-project.org/jira/browse/DDC-2131

I think that applying custom user joins conditions to last join in CTI is ok because only in last join DB will know about all joined table aliases. And I can't see any other probles with this.

Tests are not passed, because, I think, problem in tests.

To be discussed, of course

@doctrinebot

Hello,

thank you for creating this pull request. I have automatically opened an issue
on our Jira Bug Tracker for you. See the issue link:

http://www.doctrine-project.org/jira/browse/DDC-2869

We use Jira to track the state of pull requests and the versions they got
included in.

@Strate
Contributor
Strate commented Dec 24, 2013

Another solution.

We can use explicit join clause for joining all tables from cti hirerarchy as one table. Let me show example:
We have a hierarchy of 3 classes. A is root, B & C are two childs. Tables are: tA, tB, tC.
Also, we have an entity D (table tD) with reference to A from field refA.
And now we want to execute DQL SELECT d FROM D LEFT JOIN D.refA _a WITH _a.id = 1
In current realization it will be translated to SQL:

SELECT
   ***
FROM tD
LEFT JOIN tA ON <D.refA auto join condition>
LEFT JOIN tB ON <B CTI auto join condition>
LEFT JOIN tC ON <C CTI auto join condition> AND ( tA.id = 1)

It is wrong, because user condition will be applied only to last join, but it should affect all CTI joins. I suggest to use explicit join clauses (http://www.postgresql.org/docs/9.3/static/explicit-joins.html for postgresql, I think that most of all databases have similar technic).

With explicit join clauses resulting sql will be this:

SELECT 
   ***
FROM tD
LEFT JOIN (
    tA
    LEFT JOIN tB ON <B CTI auto join condition>
    LEFT JOIN tC ON <C CTI auto join condition>
) ON <D.refA auto join condition> AND (tA.id = 1)

In this sql last user join condition (AND (tA.id = 1)) will be applied to whole join of D.refA relation.

What do you think?

@Strate
Contributor
Strate commented Jan 19, 2014

I have implemeted nested joins generation for class table inheritance and ON/WITH clause.
Failed 1 test is also failed in master branch.
Review, please.

@Ocramius Ocramius and 1 other commented on an outdated diff Jan 20, 2014
lib/Doctrine/ORM/Query/SqlWalker.php
break;
}
// Handle WITH clause
- if ($condExpr !== null) {
- // Phase 2 AST optimization: Skip processing of ConditionalExpression
- // if only one ConditionalTerm is defined
- $sql .= ' AND (' . $this->walkConditionalExpression($condExpr) . ')';
- }
+ $withCondition = (null !== $condExpr) ? ('(' . $this->walkConditionalExpression($condExpr) . ')') : '';
@Ocramius
Ocramius Jan 20, 2014 Member

Can you swap the conditional? null === $condExpr ? '' : '(' . $this->walkConditionalExpression($condExpr) . ')'?

@Strate
Strate Jan 20, 2014 Contributor

Of course I can, if this really needed.

@Ocramius Ocramius and 1 other commented on an outdated diff Jan 20, 2014
lib/Doctrine/ORM/Query/SqlWalker.php
if ($targetClass->isInheritanceTypeJoined()) {
- $sql .= $this->_generateClassTableInheritanceJoins($targetClass, $joinedDqlAlias);
+ $ctiJoins = $this->_generateClassTableInheritanceJoins($targetClass, $joinedDqlAlias);
+ // If we have WITH condition, we need to build nested joins for target class table and cti joins
+ if ($withCondition) {
+ $sql .= '(' . $targetTableJoin['table'] . $ctiJoins . ') ON ' . $targetTableJoin['condition'] . ' AND ' . $withCondition;
@Ocramius
Ocramius Jan 20, 2014 Member

You are not checking if $targetTableJoin keys are set

@Strate
Strate Jan 20, 2014 Contributor

I think it is not necessary, because $targetTableJoin always filled with both keys if one of switch case fired. If no switch case fired it is abnormal situation (and, I think, we need to add default section with throwing exception). In previous version int this case invalid sql generated.

@Ocramius
Ocramius Jan 20, 2014 Member

@Strate an exception would also be fine as long as you can't reach this location without $targetTableJoin being set

@Strate
Strate Jan 20, 2014 Contributor

@Ocramius I will add an exception.

@Ocramius Ocramius commented on the diff Jan 20, 2014
lib/Doctrine/ORM/Query/SqlWalker.php
if ($targetClass->isInheritanceTypeJoined()) {
- $sql .= $this->_generateClassTableInheritanceJoins($targetClass, $joinedDqlAlias);
+ $ctiJoins = $this->_generateClassTableInheritanceJoins($targetClass, $joinedDqlAlias);
+ // If we have WITH condition, we need to build nested joins for target class table and cti joins
+ if ($withCondition) {
@Ocramius
Ocramius Jan 20, 2014 Member

Can you group the if ($withCondition) { at line #1109 and this one together?

@Strate
Strate Jan 20, 2014 Contributor

Do you mean something like this?

if ($targetClass->isInheritanceTypeJoined()) {
    $ctiJoins = $this->_generateClassTableInheritanceJoins($targetClass, $joinedDqlAlias);
    if ( $withCondition ) {
        $sql .= '(' . $targetTableJoin['table'] . $ctiJoins . ') ON ' . $targetTableJoin['condition'];
    } else {
        $sql .= $targetTableJoin['table'] . ' ON ' . $targetTableJoin['condition'] . $ctiJoins;
    }
} else {
    $sql .= $targetTableJoin['table'] . ' ON ' . $targetTableJoin['condition'];
}

if ($withCondition ) {
    $sql .= ' AND ' . $withCondition;
}

And, yet one question. What do you think about string interpolation instead of concatenation?
$sql .= "({$targetTableJoin['table']}{$ctiJoins}) ON {$targetTableJoin['condition']}";
looks better and will work bit faster than
$sql .= '(' . $targetTableJoin['table'] . $ctiJoins . ') ON ' . $targetTableJoin['condition'];

@Ocramius
Ocramius Feb 4, 2014 Member

Yes, the first example is better. As of interpolation vs concatenation, we never use interpolation.

@Strate
Strate Feb 5, 2014 Contributor

Is everything ok now?

@Strate
Contributor
Strate commented Feb 4, 2014

When you merge or decline my PR?

@beberlei
Member
beberlei commented Feb 8, 2014

@guilhermeblanco can you review?

@guilhermeblanco
Member

@beberlei added to todo list... will do today I suspect.
Unit tests are failing still. =\

@Strate
Contributor
Strate commented Feb 8, 2014

Unit tests are failing because thay were failing in master branch when I started work on this. Should I rebase or merge with current master?

@guilhermeblanco
Member

@Strate yes, please... =)

@Strate
Contributor
Strate commented Feb 10, 2014

@guilhermeblanco merged with current master branch, so, now travis build is seems to be ok.

@Strate
Contributor
Strate commented Feb 17, 2014

1 month after fix was commited - still no resolution.

@guilhermeblanco guilhermeblanco merged commit 0dbd742 into doctrine:master Feb 17, 2014

1 check passed

default The Travis CI build passed
Details
@guilhermeblanco
Member

Sorry @Strate, patch is good, I just wanted to remove the else, but completely forgot about it. =
Merged for now... I can deal with the else another day. =)

@Strate Strate deleted the Strate:bugfix/join-with-condition-placement-fix branch Feb 18, 2014
@Strate Strate restored the Strate:bugfix/join-with-condition-placement-fix branch Feb 18, 2014
@Strate Strate deleted the Strate:bugfix/join-with-condition-placement-fix branch Feb 18, 2014
@Strate
Contributor
Strate commented Feb 18, 2014

@guilhermeblanco good, thank you very much!

@olee
olee commented Jan 29, 2015

It seems this issue was not fully resolved yet.
I have following tables:

  • Order => ManyToOne => Product
  • Product (Joined inheritance table with subtyle ProductModule)

And doing this query does not work:

SELECT o.id, p.id FROM Order o INNER JOIN Product p WITH p.id = o.product WHERE p.user = 1

It produces the following SQL (simplified):

SELECT ... FROM order o INNER JOIN product p LEFT JOIN product_module m ON p.id = m.id AND (p.id = o.product_id) WHERE p.user_id = 1

But correct would be

SELECT ... FROM order o INNER JOIN (product p LEFT JOIN product_module m ON p.id = m.id) ON p.id = o.product_id WHERE p.user_id = 1

Tested with doctrine 2.4.x-dev

@Strate
Contributor
Strate commented Jan 29, 2015

@olee could you provide a unit test for your case?

@olee
olee commented Feb 1, 2015

For now I separated the whole thing into 2 queries.
The first selects all products matching the user-condition and the seconds gets the order with the condition "p.id IN (...)" with all the product-IDs.

I will try to prepare a test-code when I have time.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment