Implement single-directional rel table storage (not including updates) #4688
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
royi-luo
force-pushed
the
royi/fwd-rel-table
branch
from
67e47e6 to
8967b7f
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #4688 +/- ##
=======================================
Coverage 86.17% 86.18%
=======================================
Files 1375 1375
Lines 58491 58565 +74
Branches 7221 7239 +18
=======================================
+ Hits 50407 50474 +67
- Misses 7920 7927 +7
Partials 164 164 ☔ View full report in Codecov by Sentry. |
|
Quick sanity check for FTS index size: Query:
|
Benchmark ResultMaster commit hash:
|
royi-luo
commented
Jan 8, 2025
| @@ -1,6 +1,7 @@ | |||
| #include "binder/binder.h" | |||
There was a problem hiding this comment.
I added the storage direction to table info for now but not sure this is the best place since table info has one row per rel table property. I don't think there is an existing table function that works better though so it would probably be best to either have it here or have a new table function for storage direction
There was a problem hiding this comment.
I think this is fine for now.
royi-luo
commented
Jan 8, 2025
royi-luo
commented
Jan 8, 2025
royi-luo
commented
Jan 8, 2025
royi-luo
commented
Jan 8, 2025
royi-luo
commented
Jan 8, 2025
royi-luo
commented
Jan 8, 2025
royi-luo
commented
Jan 8, 2025
Benchmark ResultMaster commit hash:
|
royi-luo
force-pushed
the
royi/fwd-rel-table
branch
from
7809e95 to
49d8257
Compare
Benchmark ResultMaster commit hash:
|
Benchmark ResultMaster commit hash:
|
andyfengHKU
reviewed
Jan 9, 2025
| @@ -325,7 +325,7 @@ kU_CreateNodeTable | |||
| : CREATE SP NODE SP TABLE SP (kU_IfNotExists SP)? oC_SchemaName SP? '(' SP? kU_PropertyDefinitions SP? ( ',' SP? kU_CreateNodeConstraint )? SP? ')' ; | |||
|
|
|||
| kU_CreateRelTable | |||
| : CREATE SP REL SP TABLE SP (kU_IfNotExists SP)? oC_SchemaName SP? '(' SP? kU_RelTableConnection SP? ( ',' SP? kU_PropertyDefinitions SP? )? ( ',' SP? oC_SymbolicName SP? )? ')' ; | |||
| : CREATE SP REL SP TABLE SP (kU_IfNotExists SP)? oC_SchemaName SP? '(' SP? kU_RelTableConnection SP? ( ',' SP? kU_PropertyDefinitions SP? )? ( ',' SP? oC_SymbolicName SP? )? ')' (SP WITH SP? kU_ParsingOptions)?; | |||
There was a problem hiding this comment.
Can u use a separate PR to change all ParsingOptions to options. I no longer think ParsingOptions is a good name
There was a problem hiding this comment.
We already have kU_Options that matches kU_ParsingOptions but without the surrounding (. Do you have a different suggestion for the rule name or did you mean to rewrite all instances of kU_ParsingOptions using the current kU_Options?
| @@ -29,6 +34,10 @@ class RelTableCatalogEntry final : public TableCatalogEntry { | |||
| common::table_id_t getDstTableID() const { return dstTableID; } | |||
| bool isSingleMultiplicity(common::RelDataDirection direction) const; | |||
| common::RelMultiplicity getMultiplicity(common::RelDataDirection direction) const; | |||
|
|
|||
| std::span<const common::RelDataDirection> getStorageDirections() const; | |||
There was a problem hiding this comment.
Maybe call these two
std::span<const common::RelDataDirection> getRelDataDirections
common::RelStorageDirection getStorageDirection
There was a problem hiding this comment.
By the way I was thinking of replacing RelStorageDirection with ExtendDirection since I noticed we have that. Do you think that's a reasonable idea or should we keep them separate?
royi-luo
force-pushed
the
royi/fwd-rel-table
branch
2 times, most recently
from
f7300f0 to
8fc9faa
Compare
Benchmark ResultMaster commit hash:
|
ray6080
approved these changes
Jan 10, 2025
CMakeLists.txt
Outdated
| @@ -1,6 +1,6 @@ | |||
| cmake_minimum_required(VERSION 3.15) | |||
|
|
|||
| project(Kuzu VERSION 0.7.1.1 LANGUAGES CXX C) | |||
| project(Kuzu VERSION 0.7.1.2 LANGUAGES CXX C) | |||
There was a problem hiding this comment.
You might have to bump this again before merge. Please double check against master branch when you're ready to merge the PR.
Benchmark ResultMaster commit hash:
|
Rel table changes Partitioner changes Update grammar Create FTS index with single direction storage Fix FTS integration + rename 'fwd' option to 'fwd_only' Run clang-format Fix test failures Bump project version Add storage direction to table info Update table info tests Self-review rel table catalog stores vector of directions instead of storage direction More code cleanup Run clang-format More code cleanup Run clang-format Address review comments
royi-luo
force-pushed
the
royi/fwd-rel-table
branch
from
38fb01d to
bf68a1c
Compare
Benchmark ResultMaster commit hash:
|
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.
Description
Adds option for rel tables to only store data in the forward direction. Implemented parts include:
RelBatchInsertonly creates pipelines for the required directionsPartitioneronly create partitions for the required directionsRelTableWITH (storage_direction = 'fwd_only')Future TODOs
The following are still required to complete the feature:
Partially addresses #4320
Contributor agreement