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

Rework local storage for node table and fix #2376 #2394

Merged
merged 1 commit into from
Nov 14, 2023
Merged

Conversation

ray6080
Copy link
Contributor

@ray6080 ray6080 commented Nov 12, 2023

  • Rework local storage to be more space efficient. Instead of always allocating a Vector of 2048 whenever an update/insert resides in its offset range. For example, update at offset 0 and offset 2049 will allocate two Vectors. In this PR, updates/inserts always append to the end of a collection of Vectors, which allocates as needed and one value at a time. The mapping between node offsets and row idx is maintained in extra maps.
  • Fix issue 2376. The issue is due to incorrectly updating hash index when updates on node table involves the primary key column.

Note that this PR also forces flattening on updates/deletions/insertions, which is to simplify the implementation of following fixes on rel updates/deletions/insertions. This can bring performance down, though it's not profiled by how much, and should be solved at once as we optimize the update performance after all fixes.
Also, there are quite few places not covered by tests, which are mostly due to that lots of tests on rel updates are disabled. I will revisit the test coverage after my fixes on rel updates.

TODOs

  • Revisit and improve storage test coverage.
  • Remove the rigid flatten all rules on updates/insertions/deletions.

Copy link

codecov bot commented Nov 12, 2023

Codecov Report

Attention: 32 lines in your changes are missing coverage. Please review.

Comparison is base (684511c) 91.24% compared to head (5b73d7b) 91.21%.

Files Patch % Lines
src/storage/store/column_chunk.cpp 30.76% 18 Missing ⚠️
src/storage/store/string_column_chunk.cpp 0.00% 5 Missing ⚠️
src/storage/store/column.cpp 94.73% 3 Missing ⚠️
src/storage/store/rel_table.cpp 40.00% 3 Missing ⚠️
src/storage/local_table.cpp 98.24% 2 Missing ⚠️
src/storage/store/rel_table_data.cpp 0.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #2394      +/-   ##
==========================================
- Coverage   91.24%   91.21%   -0.03%     
==========================================
  Files        1022     1020       -2     
  Lines       36323    36336      +13     
==========================================
+ Hits        33144    33145       +1     
- Misses       3179     3191      +12     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@ray6080 ray6080 changed the title Rework local storage for node table Rework local storage for node table and fix #2376 Nov 13, 2023
@ray6080 ray6080 force-pushed the rel-updates branch 2 times, most recently from 16199c4 to 56aa822 Compare November 14, 2023 06:18
src/common/vector/value_vector.cpp Outdated Show resolved Hide resolved
src/include/storage/local_storage.h Outdated Show resolved Hide resolved
src/include/storage/local_table.h Outdated Show resolved Hide resolved
src/include/storage/local_table.h Show resolved Hide resolved
src/storage/store/node_table_data.cpp Outdated Show resolved Hide resolved
src/storage/store/rel_table_data.cpp Outdated Show resolved Hide resolved
src/storage/store/struct_column.cpp Show resolved Hide resolved
src/storage/store/struct_column_chunk.cpp Outdated Show resolved Hide resolved
@ray6080 ray6080 force-pushed the rel-updates branch 2 times, most recently from c965f86 to 8136f74 Compare November 14, 2023 20:29
@ray6080 ray6080 merged commit ba5b927 into master Nov 14, 2023
12 checks passed
@ray6080 ray6080 deleted the rel-updates branch November 14, 2023 22:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants