-
-
Notifications
You must be signed in to change notification settings - Fork 235
TASK: Refactor content subgraph queries and add reference resolution index #5626
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
Conversation
|
Amazing numbers, thanks for taking care! It's obviously quite hard to reason about this, but I don't see anything weird at first glance and I would say: Tests should(tm) cover that the behavior didn't change |
|
|
||
| return $table | ||
| ->setPrimaryKey(['name', 'position', 'nodeanchorpoint']); | ||
| ->setPrimaryKey(['name', 'position', 'nodeanchorpoint']) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is the first change after the release of Neos 9.0 to bring a schema adjustment which as you said must be executed via ./flow cr:setup
we must explicitly denote that in the release notes as new users will not be aware of this additional set of migrations (aside from doctrine migrate)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yep; Technically it does not have to be executed, but the speedup does not happen without it.
| ]; | ||
| $subtreeTagConstraints = ''; | ||
| $i = 0; | ||
| foreach ($this->visibilityConstraints->excludedSubtreeTags as $excludedTag) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
isnt there the method addSubtreeTagConstraints which we still use below?
but for a proper review i would need to dig into this deeply
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yep; needed to copy it though, since the query builder has no support (that I know of) for subselects.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same as @bwaidelich , makes sense :). I did not check the queries in 110% detail, but trust our tests.
This significantly improves (back)reference query performance
(in a 25k node project from >200ms to <2ms for a query fetching 8 nodes by reference)
Upgrade instructions
./flow cr:setup
Review instructions
mainly the query tweaks are interesting
Checklist
FEATURE|TASK|BUGFIX!!!and have upgrade-instructions