[CLN] Reorganize delta module into seperate module and split out impls and [PERF] Refactor bf get_size to avoid nested loops #2674
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
Reviewer ChecklistPlease leverage this checklist to ensure your code review is thorough before approving Testing, Bugs, Errors, Logs, Documentation
System Compatibility
Quality
|
This stack of pull requests is managed by Graphite. Learn more about stacking. |
1 task
HammadB
force-pushed
the
hammad/reorg_delta_module
branch
from
23b8513
to
0d77acf
Compare
sanketkedia
approved these changes
Aug 21, 2024
| @@ -52,14 +52,6 @@ impl BlockDelta { | |||
| V::delete(prefix, key.into(), self) | |||
| } | |||
|
|
|||
| /// Gets the minimum key in the block delta. | |||
| pub fn get_min_key(&self) -> Option<CompositeKey> { | |||
There was a problem hiding this comment.
was this not being used anywhere?
There was a problem hiding this comment.
nope, but i added a flavor back in stacked pr
| @@ -377,11 +377,6 @@ impl SparseIndexManager { | |||
| } | |||
| } | |||
|
|
|||
| pub fn create(&self, id: &Uuid) -> SparseIndex { | |||
There was a problem hiding this comment.
not being used anywhere?
There was a problem hiding this comment.
will clean up separately, as it was not added in this PR and I don't want to retrigger tests
## Description of changes *Summarize the changes made by this PR.* - This PRs main intent is to make get_size() and sizing related operations on delta O(1) instead of O(n). The O(n) behavior was leading to a slow insert time since every call resulted in a get_size(). - Refactored uint32, string, int32 deltas into a single_column_storage delta that is generic over value types and uses a size tracker helper struct. - Refactored DataRecord storage to use a single map instead of one per column, and also one lock - Refactored both deltas to track their sizes, handling overwrites and deletes - Moved the add logic OUT of ArrowWriteableImplementations, and made them dumb proxies for type inversion - Changed several value types to be written with ownership, they are copied under the hood anyways, and our general pattern is to put the onus of cloning on the caller. Int32Arrays are Arc’ed, RoaringBitmap needed to be copied etc. - Handled a stray TODO on DataRecord metadata sizing - Pushed splitting into the delta storage, simplifying the delta layer and getting better use of generics ## Test plan *How are these changes tested?* This change needs more tests, I plan to stack some more tests on this PR. Exisitng tests cover add path but but not post-delete sizing. - [x] Tests pass locally with `pytest` for python, `yarn test` for js, `cargo test` for rust ## Documentation Changes None
HammadB
changed the title
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 of changes
Summarize the changes made by this PR.
stacked with #2684
Test plan
How are these changes tested?
Existing tests cover this change
pytestfor python,yarn testfor js,cargo testfor rustDocumentation Changes
None