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

TASK: Fix and improve find and children flowQuery operation #4712

Open
wants to merge 4 commits into
base: 9.0
Choose a base branch
from

Conversation

mficzel
Copy link
Member

@mficzel mficzel commented Nov 7, 2023

The flowQuery operations find and children are fixed & optimized for the new cr. They now use a combined query with nodetype and property criteria when possible. The number of performed db-queries is the number of contextNodes times the number of filterGroups (comma separated parts)

In addition the find operation now can also handle single property criteria and does not rely on having an instanceof filter first.

The solution adds a NodeFilterCriteria class that encapsulates nodeTypeCriteria and PropertyConstraints. The collection object NodeFilterCriteriaGroup combines multiple of those. The NodeFilterCriteriaGroupFactory allows to take a fizzle-expression and create a NodeFilterCriteriaGroup once only Attribute filters (instanceof and property) are used.

Upgrade instructions

Review instructions

This based on #4699 and should be merged afterwards.
Also we should discuss how to proceed regarding the case insensitive operators feature neos/flow-development-collection#2600 currently those are handled by the legacy code as fallback.

Checklist

  • Code follows the PSR-2 coding style
  • Tests have been created, run and adjusted as needed
  • The PR is created against the lowest maintained branch
  • Reviewer - PR Title is brief but complete and starts with FEATURE|TASK|BUGFIX
  • Reviewer - The first section explains the change briefly for change-logs
  • Reviewer - Breaking Changes are marked with !!! and have upgrade-instructions

@mficzel mficzel force-pushed the 90/improveFindAndChildrenOperation branch 2 times, most recently from 13dc5bd to 209df71 Compare November 7, 2023 21:26
@mficzel
Copy link
Member Author

mficzel commented Nov 7, 2023

@mhsdesign @nezaniel it is vary well possible that i cracked the find and children flowQuery nut :-) ... needs discussion how to name this and which tests and edge cases i missed

!! The linting errors are coming from upstream !!

@mficzel mficzel requested a review from nezaniel November 7, 2023 21:30
@mficzel mficzel force-pushed the 90/improveFindAndChildrenOperation branch 4 times, most recently from 0aaed67 to 43dac6b Compare November 9, 2023 15:43
@mficzel
Copy link
Member Author

mficzel commented Feb 3, 2024

We put this aside as the node filter criteria did not provide the same case-related behavior as the old flowQuery implementation … maybe this has changed in the meantime

Other than that we considered this a bit too dangerous and thought about moving this to a third party package

@mhsdesign
Copy link
Member

Yes once #4725 introduces case insensitive operators we might be able to use satisfy the old flow query api?

i understand that the new cr subgraph api has different features than the feature set of neos 8.3's find or children, but i think the overlap is big enough to use the faster db implementation than to mimic the legacy behaviour by filtering expensively on php.

For legacy use cases we could possibly ship a legacy implementation of find or children in an eel helper LegacyFlowQuery so that people with odd requirements have a change to adjust?

@mhsdesign mhsdesign force-pushed the 90/improveFindAndChildrenOperation branch from 43dac6b to a4d7230 Compare February 3, 2024 18:26
@mficzel mficzel force-pushed the 90/improveFindAndChildrenOperation branch 2 times, most recently from 43dac6b to 4167f9b Compare February 16, 2024 15:56
The flowQuery operations find and children are fixed optimized for the new cr to use a combined query for nodetype and property criteria.
The number of performed db-queries is the number of contextNodes times the number of filterGroups (comma separated parts)

In addition the `find` operation now can also handle single property criteria and does not rely on having an instanceof filter first.
@mficzel mficzel marked this pull request as ready for review February 16, 2024 16:47
@mficzel
Copy link
Member Author

mficzel commented Feb 16, 2024

With the case sensitive operations in place this might be feasible now ...

@mficzel mficzel force-pushed the 90/improveFindAndChildrenOperation branch from 4167f9b to 1f8b2b2 Compare February 16, 2024 16:51
mficzel and others added 2 commits February 16, 2024 17:52
…teriaGroup.php

Co-authored-by: Marc Henry Schultz <85400359+mhsdesign@users.noreply.github.com>
Co-authored-by: Marc Henry Schultz <85400359+mhsdesign@users.noreply.github.com>
@kitsunet kitsunet self-requested a review February 23, 2024 11:47
@mhsdesign mhsdesign self-requested a review May 4, 2024 11:23
@mhsdesign
Copy link
Member

I almost thought this was merged already. Ill will play around with this a bit but then i think (as all the blockers and adjustments were solved) this is ready to be merged? Would be great to find a second reviewer as well :)

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

Successfully merging this pull request may close these issues.

None yet

2 participants