-
-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Inlining vertex properties into a CompositeIndex structure #4692
Conversation
f9709af
to
80717be
Compare
4bfbbfb
to
cd15a3f
Compare
e02cacf
to
eab2ba3
Compare
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.
Thank you @ntisseyre for this great contribution!
I left some general comments below. However, I have some questions / thoughts.
- What happens if the user uses a composite index with inlined properties but requests properties which are not covered by the inline composite index? Will we fetch only vertex id from the composite index or will we fetch those inline properties from the index as well?
- It would be really good to add documentation about this new functionality. I think somewhere between
Label Constraint
andComposite versus Mixed Indexes
topics should be a good place for the doc, but I'm good with other places as well: https://github.com/JanusGraph/janusgraph/blob/master/docs/schema/index-management/index-performance.md#composite-versus-mixed-indexes - A benchmark test would be really beneficial to actually see some performance differences between inline indices and normal composite indices.
janusgraph-core/src/main/java/org/janusgraph/graphdb/database/index/IndexUpdate.java
Outdated
Show resolved
Hide resolved
janusgraph-core/src/main/java/org/janusgraph/graphdb/database/index/IndexUpdateContainer.java
Outdated
Show resolved
Hide resolved
...ph-core/src/main/java/org/janusgraph/graphdb/database/management/JanusGraphIndexWrapper.java
Outdated
Show resolved
Hide resolved
janusgraph-core/src/main/java/org/janusgraph/graphdb/database/management/ManagementSystem.java
Show resolved
Hide resolved
Signed-off-by: ntisseyre <[email protected]>
eab2ba3
to
9b8b885
Compare
Thank you, @porunov, for the feedback!
|
Signed-off-by: ntisseyre <[email protected]>
Benchmark test executed locally:
Results: Inline indices properties fetching is ~22 times faster for this test executed locally compared to normal properties fetching logic. P.S. I didn't expect such a huge difference to be fair, but looks amazing 👀 |
Benchmark test: LGTM. |
When I run this test locally, it also returns unstable results: sometimes it succeeds, and sometimes it doesn't see values being inlined in the index structure. I suspect that the reIndexing/data propagation is not finished or that there is some cache, but I couldn't find where. |
janusgraph-backend-testutils/src/main/java/org/janusgraph/graphdb/JanusGraphIndexTest.java
Show resolved
Hide resolved
Signed-off-by: ntisseyre <[email protected]>
b76766a
to
ce8e897
Compare
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.
LGTM. Thank you for this amazing feature @ntisseyre !
21f1d9f
to
1a52520
Compare
@porunov, I have updated the documentation in one of the commits. Please take a look at the |
We actually describe all upgrade instructions and breaking changes in docs/changelog.md in the release the feature lands into. As your contributed feature will land into
In that case you can remove |
Signed-off-by: ntisseyre <[email protected]>
1a52520
to
5aa68f5
Compare
@ntisseyre do you want to merge it or not ready yet? Just asking because I wanted to rebase #4695 after you merge this PR to add inline index functionality to the JSON schema importer. |
I was waiting to see if anyone had any more comments, but I'm good to merge it. |
Sorry I just saw this. This is awesome!!!! I didn't take a look at implementation detail, but we are essentially implementing covering index, right? |
That's right, it is like MySQL covering index |
Overview
Inlining vertex properties into a search index structure (CompositeIndex) can offer significant performance and efficiency benefits.
Performance Improvements
Faster Querying: Inlining vertex properties directly within the index allows the search engine to retrieve all relevant data from the index itself. This means queries don’t need to make additional calls to data stores to fetch full vertex information, significantly reducing lookup time.
Data Locality
In distributed storages, having inlined properties ensures that more complete data exists within individual partitions or shards. This reduces cross-node network calls and improves the overall query performance by ensuring data is more local to the request being processed.
Cost of Indexing vs. Storage Trade-off
While inlining properties increases the size of the index (potentially leading to more extensive index storage requirements), it is often a worthwhile trade-off for performance, mainly when query speed is critical. This is a typical pattern in systems optimized for read-heavy workloads.
Usage
In order to take advantage of the inlined properties feature, JanusGraph Transaction should be set to use
.propertyPrefetching(false)
Example: