kuzudb / kuzu

Embeddable property graph database management system built for query speed and scalability. Implements Cypher.
https://kuzudb.com/
MIT License
1.38k stars 97 forks source link

Bitpack internal IDs #2095

Closed benjaminwinger closed 10 months ago

benjaminwinger commented 1 year ago

I was going to benchmark this, as it adds an extra step to the decompression, but I'm not sure it's actually being used. Are internalIDs actually stored anywhere other than in rels?

codecov[bot] commented 1 year ago

Codecov Report

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

Comparison is base (6175606) 91.52% compared to head (3e71420) 90.57%.

:exclamation: Current head 3e71420 differs from pull request most recent head 748465a. Consider uploading reports for the commit 748465a to get more accurate results

Files Patch % Lines
src/storage/store/column.cpp 66.66% 8 Missing :warning:
src/storage/compression/compression.cpp 75.00% 1 Missing :warning:
Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #2095 +/- ## ========================================== - Coverage 91.52% 90.57% -0.96% ========================================== Files 1024 1020 -4 Lines 37844 36532 -1312 ========================================== - Hits 34637 33088 -1549 - Misses 3207 3444 +237 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

ray6080 commented 1 year ago

I was going to benchmark this, as it adds an extra step to the decompression, but I'm not sure it's actually being used. Are internalIDs actually stored anywhere other than in rels?

You're right. Currently it's not materialized in node tables.

ray6080 commented 1 year ago

@benjaminwinger , can we hold this PR a bit longer after the rel node group one #2246 ?

benjaminwinger commented 1 year ago

Yes, I was thinking it would make sense to wait until that's merged so it can be tested.

ray6080 commented 10 months ago

I think this can be closed now. @benjaminwinger

benjaminwinger commented 10 months ago

Yes, let's close in favour of #2579