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.

Sign up for GitHub

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

BUGFIX: Improve performance on find*Aggregates #5268

Merged
merged 1 commit into from
Oct 7, 2024
Merged

BUGFIX: Improve performance on find*Aggregates #5268

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 3 additions & 4 deletions Neos.ContentGraph.DoctrineDbalAdapter/src/Domain/Repository/ContentGraph.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -175,10 +175,9 @@ public function findNodeAggregateById(
public function findParentNodeAggregates(
NodeAggregateId $childNodeAggregateId
): NodeAggregates {
$queryBuilder = $this->nodeQueryBuilder->buildBasicNodeAggregateQuery()
->innerJoin('n', $this->nodeQueryBuilder->tableNames->hierarchyRelation(), 'ch', 'ch.parentnodeanchor = n.relationanchorpoint')
->innerJoin('ch', $this->nodeQueryBuilder->tableNames->node(), 'cn', 'cn.relationanchorpoint = ch.childnodeanchor')
->andWhere('ch.contentstreamid = :contentStreamId')
$queryBuilder = $this->nodeQueryBuilder->buildParentNodeAggregateQuery()
->innerJoin('h', $this->nodeQueryBuilder->tableNames->node(), 'cn', 'cn.relationanchorpoint = h.childnodeanchor')
->andWhere('h.contentstreamid = :contentStreamId')
->andWhere('cn.nodeaggregateid = :nodeAggregateId')
->setParameters([
'nodeAggregateId' => $childNodeAggregateId->value,
Expand Down
12 changes: 10 additions & 2 deletions Neos.ContentGraph.DoctrineDbalAdapter/src/NodeQueryBuilder.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,12 +56,10 @@ public function buildChildNodeAggregateQuery(NodeAggregateId $parentNodeAggregat
return $this->createQueryBuilder()
->select('cn.*, ch.contentstreamid, ch.subtreetags, cdsp.dimensionspacepoint AS covereddimensionspacepoint')
->from($this->tableNames->node(), 'pn')
->innerJoin('pn', $this->tableNames->hierarchyRelation(), 'ph', 'ph.childnodeanchor = pn.relationanchorpoint')
->innerJoin('pn', $this->tableNames->hierarchyRelation(), 'ch', 'ch.parentnodeanchor = pn.relationanchorpoint')
->innerJoin('ch', $this->tableNames->dimensionSpacePoints(), 'cdsp', 'cdsp.hash = ch.dimensionspacepointhash')
->innerJoin('ch', $this->tableNames->node(), 'cn', 'cn.relationanchorpoint = ch.childnodeanchor')
->where('pn.nodeaggregateid = :parentNodeAggregateId')
->andWhere('ph.contentstreamid = :contentStreamId')
->andWhere('ch.contentstreamid = :contentStreamId')
->orderBy('ch.position')
->setParameters([
Expand All @@ -70,6 +68,16 @@ public function buildChildNodeAggregateQuery(NodeAggregateId $parentNodeAggregat
]);
}

public function buildParentNodeAggregateQuery(): QueryBuilder
{
return $this->createQueryBuilder()
->select('n.*, h.contentstreamid, h.subtreetags, dsp.dimensionspacepoint AS covereddimensionspacepoint')
->from($this->tableNames->node(), 'n')
->innerJoin('n', $this->tableNames->hierarchyRelation(), 'h', 'h.parentnodeanchor = n.relationanchorpoint')
Copy link
Member

Choose a reason for hiding this comment

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

fyi the only difference to the previously used buildBasicNodeAggregateQuery is that we join on h.parentnodeanchor instead of h.childnodeanchor

I had the feeling introducing this might deduplicate code another place and it could:

In findParentNodeAggregateByChildOriginDimensionSpacePoint we use the same logic besides one additional where. So i think it could be nicely deduplicated like:

public function findParentNodeAggregateByChildOriginDimensionSpacePoint(NodeAggregateId $childNodeAggregateId, OriginDimensionSpacePoint $childOriginDimensionSpacePoint): ?NodeAggregate
    {
        $subQueryBuilder = ...;
    
        $queryBuilder = $this->nodeQueryBuilder->buildParentNodeAggregateQuery()
            ->andWhere('n.nodeaggregateid = (' . $subQueryBuilder->getSQL() . ')')
            ->setParameters([
                'contentStreamId' => $this->contentStreamId->value,
                'childNodeAggregateId' => $childNodeAggregateId->value,
                'childOriginDimensionSpacePointHash' => $childOriginDimensionSpacePoint->hash,
            ]);

Also i dont exactly understand how this utility is supposed to work (when do we pass arguments or when to use setParameters) but because we dont return a final query i think its buildBasicParentNodeAggregateQuery

Copy link
Contributor Author

@dlubitz dlubitz Oct 7, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Is your comment only for clarification? Or do you complain about the partially duplication of the queries and want to get that changed?

Copy link
Member

@mhsdesign mhsdesign Oct 7, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

i was just thoroughly reviewing this and partly investigating and trying to understand. So i guess 50/50 ill create a followup pr :D

_> #5276

->innerJoin('h', $this->tableNames->dimensionSpacePoints(), 'dsp', 'dsp.hash = h.dimensionspacepointhash')
->where('h.contentstreamid = :contentStreamId');
}

public function buildFindRootNodeAggregatesQuery(ContentStreamId $contentStreamId, FindRootNodeAggregatesFilter $filter): QueryBuilder
{
$queryBuilder = $this->buildBasicNodeAggregateQuery()
Expand Down
Loading