Revamp JanusGraphMultiQueryStrategy for better parent step usage [cql-tests] [tp-tests] #3783
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
porunov
force-pushed
the
feature/multi-query-strategy-revamp
branch
from
17b3485
to
464f027
Compare
This was referenced May 20, 2023
porunov
force-pushed
the
feature/multi-query-strategy-revamp
branch
8 times, most recently
from
e73a570
to
052cfa7
Compare
|
Benchmark tests in this comment are now outdated due to CQL storage improvements and new development iterations in this PR. Please, see the new benchmark tests in the following comment. Executed added benchmarks both for this PR and for Master branch: This PR: There were no performance difference for tests:
Conclusion: As this PR adds support to a number of new parent steps, it's expected that new parent steps will gain better performance in multi-query cases. As |
porunov
force-pushed
the
feature/multi-query-strategy-revamp
branch
3 times, most recently
from
d4688a8
to
42fd43d
Compare
porunov
changed the title
porunov
force-pushed
the
feature/multi-query-strategy-revamp
branch
2 times, most recently
from
243f328
to
ccec0b8
Compare
|
I made another development iteration and improved multi-nested repeat steps usage for some cases (added 3 different modes of running multi-nested The new benchmarks are below. Master branch: Current branch: Performance improvements are reflected here:
Please, in case anyone has enough capacity and would like to review this PR (or just have any concerns / comments / suggestions without committing for the full review), leave a commit in this PR until the start of the next week. cc @JanusGraph/committers |
porunov
force-pushed
the
feature/multi-query-strategy-revamp
branch
from
ccec0b8
to
7f21c5a
Compare
There was a problem hiding this comment.
Impressive work, not only on the implementation itself, including the benchmarks, but also on the documentation!
I have only reviewed the documentation as I won't have enough time to review the code changes here. My comments are mostly about stylistic issues.
porunov
force-pushed
the
feature/multi-query-strategy-revamp
branch
from
7f21c5a
to
234bf71
Compare
…-tests] [tp-tests] This commit improves JanusGraphMultiQueryStrategy to better support multi-query compatible parent steps. 1. This commit brings better support for `repeat` step by introducing next itaration registration process. Previously `repeat` children steps was getting traversers registered from the beginning of all outer repeat steps which could result in duplicate or unnecesary retrievals for the first batch. Moreover, next iterations were not considered. This commit changes the approach to bring different `repeat` step modes which can change the batches registration behaviour to aacount only the closest `repeat` step, all `repeat` steps, all only starts of all `repeat` steps. 2. This commit adds support to almost all known TinkerPop Parent steps. The exception is `match` step. We didn't have proper outter start registration for `match` step previously and now as well. Fixes JanusGraph#3733 Fixes JanusGraph#3735 Fixes JanusGraph#2996 Signed-off-by: Oleksandr Porunov <alexandr.porunov@gmail.com>
porunov
force-pushed
the
feature/multi-query-strategy-revamp
branch
from
234bf71
to
89f5611
Compare
Thank you @FlorianHockmann ! I agree with all your comments and applied the changes you suggested. |
FlorianHockmann
approved these changes
Jun 5, 2023
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This commit improves JanusGraphMultiQueryStrategy to better support multi-query compatible parent steps.
This commit brings better support for
repeatstep by introducing next itaration registration process.Previously
repeatchildren steps was getting traversers registered from the beginning of all outer repeat steps whichcould result in duplicate or unnecesary retrievals for the first batch. Moreover, next iterations were not considered.
This commit changes the approach to bring different
repeatstep modes which can change the batches registration behaviourto aacount only the closest
repeatstep, allrepeatsteps, all only starts of allrepeatsteps.This commit adds support to almost all known TinkerPop Parent steps.
The exception is
matchstep. We didn't have proper outter start registration formatchstep previously and now as well.To not introduce a behavior change in multi-nested
repeatsteps the configuration optionquery.batch.repeat-step-modeis added with 3 modes available (closest_repeat_parent- mode which takes the closestrepeatparent into account only,starts_only_of_all_repeat_parents- mode which takes allrepeatparent steps but only starts into account (previous behavior),all_repeat_parents- same asstarts_only_of_all_repeat_parentsbut takesrepeatends into account as well (new default behavior)).The previous approach of inserting and finding multiQuery steps were split on 2 phases:
Current approach is:
See performance improvement in this PR here.
Fixes #3733
Fixes #3735
Fixes #2996
Thank you for contributing to JanusGraph!
In order to streamline the review of the contribution we ask you
to ensure the following steps have been taken:
For all changes:
master)?For code changes:
For documentation related changes: