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.

Sign up for GitHub

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

Jump to bottom

Refactoring to prepare for the addition of dynamic fast field #1730

Merged
merged 4 commits into from
Dec 22, 2022
Merged

Refactoring to prepare for the addition of dynamic fast field #1730

merged 4 commits into from
Dec 22, 2022

Conversation

fulmicoton
Copy link
Collaborator

  • Exposing insert_key / insert_value
  • Renamed SSTable::{Reader/Writer}-> SSTable::{ValueReader/ValueWriter}
  • Added a generic Dictionary object in the sstable crate
  • Removing the TermDictionary wrapper from tantivy, relying directly on an alias of the generic Dictionary object.
  • dropped the use of byteorder in sstable.
  • Stopped scanning / reading the entire dictionary when streaming a range.

@fulmicoton
Refactoring to prepare for the addition of dynamic fast field
8c307d2 
- Exposing insert_key / insert_value
- Renamed SSTable::{Reader/Writer}-> SSTable::{ValueReader/ValueWriter}
- Added a generic Dictionary object in the sstable crate
- Removing the TermDictionary wrapper from tantivy, relying directly on
  an alias of the generic Dictionary object.
- dropped the use of byteorder in sstable.
- Stopped scanning / reading the entire dictionary when streaming a range.
@fulmicoton fulmicoton marked this pull request as ready for review December 21, 2022 07:44
@codecov-commenter
Copy link

codecov-commenter commented Dec 21, 2022 •
edited
Loading

Codecov Report

Merging #1730 (ca0d361) into main (f39165e) will decrease coverage by 0.00%.
The diff coverage is 92.33%.

❗ Current head ca0d361 differs from pull request most recent head 2232000. Consider uploading reports for the commit 2232000 to get more accurate results

@@            Coverage Diff             @@
##             main    #1730      +/-   ##
==========================================
- Coverage   94.13%   94.13%   -0.01%     
==========================================
  Files         263      266       +3     
  Lines       50520    50723     +203     
==========================================
+ Hits        47556    47746     +190     
- Misses       2964     2977      +13
Impacted Files Coverage Δ
src/core/inverted_index_reader.rs 72.86% <0.00%> (ø)
sstable/src/dictionary.rs 75.67% <75.67%> (ø)
sstable/src/delta.rs 97.58% <94.73%> (+0.21%) ⬆️
sstable/src/lib.rs 98.75% <99.10%> (-0.85%) ⬇️
src/termdict/sstable_termdict/mod.rs 98.70% <100.00%> (-0.13%) ⬇️
src/termdict/tests.rs 100.00% <100.00%> (ø)
sstable/src/block_reader.rs 73.77% <100.00%> (-6.59%) ⬇️
sstable/src/merge/heap_merge.rs 84.44% <100.00%> (ø)
sstable/src/merge/mod.rs 92.52% <100.00%> (ø)
sstable/src/sstable_index.rs 100.00% <100.00%> (ø)
... and 12 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

PSeitz
PSeitz approved these changes Dec 21, 2022
View reviewed changes
Copy link
Contributor

@PSeitz PSeitz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

a lot of rename improvements, nice

I left some minor comments

sstable/src/dictionary.rs Outdated Show resolved Hide resolved
sstable/src/dictionary.rs Show resolved Hide resolved
sstable/src/value/mod.rs Show resolved Hide resolved
sstable/src/value/mod.rs Outdated Show resolved Hide resolved
sstable/src/value/mod.rs Outdated Show resolved Hide resolved
sstable/src/value/range.rs Outdated Show resolved Hide resolved
sstable/src/value/range.rs Outdated Show resolved Hide resolved
@fulmicoton
CR comments.
ca0d361 
Rename deserialize_u64 -> deserialize_vint_u64
@fulmicoton fulmicoton force-pushed the refactoring-sstable2 branch 4 times, most recently from dd27378 to 262d5d2 Compare December 22, 2022 03:15
@fulmicoton fulmicoton merged commit bb48c3e into main Dec 22, 2022
@fulmicoton fulmicoton deleted the refactoring-sstable2 branch December 22, 2022 03:25
This was referenced Jan 13, 2023
Closed
Closed
Hodkinson pushed a commit to Hodkinson/tantivy that referenced this pull request Jan 30, 2023
@fulmicoton
Refactoring to prepare for the addition of dynamic fast field (quickw…
7249ae0 
…it-oss#1730)

* Refactoring to prepare for the addition of dynamic fast field

- Exposing insert_key / insert_value
- Renamed SSTable::{Reader/Writer}-> SSTable::{ValueReader/ValueWriter}
- Added a generic Dictionary object in the sstable crate
- Removing the TermDictionary wrapper from tantivy, relying directly on
  an alias of the generic Dictionary object.
- dropped the use of byteorder in sstable.
- Stopped scanning / reading the entire dictionary when streaming a range.

* Added a benchmark for streaming sstable ranges.

* CR comments.

Rename deserialize_u64 -> deserialize_vint_u64

* Removed needless allocation, split serialize into serialize and clear.
@PSeitz PSeitz mentioned this pull request Jan 31, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@PSeitz PSeitz PSeitz approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@fulmicoton @codecov-commenter @PSeitz