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

fix issue in composite file #1456

Merged
merged 1 commit into from
Aug 22, 2022
Merged

fix issue in composite file #1456

merged 1 commit into from
Aug 22, 2022

Conversation

PSeitz
Copy link
Contributor

@PSeitz PSeitz commented Aug 22, 2022

The file offsets were recorded incorrectly in some cases, e.g. when the recording looked like this [(Field 1, Index 0, Offset 0), (Field 1, Index 1, Offset 14), (Field 0, Index 0, Offset 14)]. The last file is offset 14 to end of file for field 0. But the data was converted to a vec and sorted, which changes the last file to Field 1.

The file offsets were recorded incorrectly in some cases, e.g. when the recording looked like this [(Field 1, Index 0, Offset 0), (Field 1, Index 1, Offset 14), (Field 0, Index 0, Offset 14)]. The last file is offset 14 to end of file for field 0. But the data was converted to a vec and sorted, which changes the last file to Field 1.
@codecov-commenter
Copy link

codecov-commenter commented Aug 22, 2022

Codecov Report

Merging #1456 (a49c4fe) into main (4a31690) will decrease coverage by 0.01%.
The diff coverage is 89.79%.

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

@@            Coverage Diff             @@
##             main    #1456      +/-   ##
==========================================
- Coverage   94.19%   94.18%   -0.02%     
==========================================
  Files         237      237              
  Lines       44406    44443      +37     
==========================================
+ Hits        41830    41860      +30     
- Misses       2576     2583       +7     
Impacted Files Coverage Δ
src/directory/composite_file.rs 93.22% <89.79%> (-1.78%) ⬇️
src/fastfield/multivalued/mod.rs 94.11% <0.00%> (-2.95%) ⬇️
src/fastfield/reader.rs 87.80% <0.00%> (-0.61%) ⬇️
src/query/boolean_query/block_wand.rs 97.05% <0.00%> (+0.21%) ⬆️

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

@PSeitz PSeitz changed the title fix issue in composite fix issue in composite file Aug 22, 2022
@PSeitz PSeitz requested a review from fulmicoton August 22, 2022 07:14
@@ -106,6 +98,14 @@ pub struct CompositeFile {
offsets_index: HashMap<FileAddr, Range<usize>>,
}

impl std::fmt::Debug for CompositeFile {
fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
fn fmt(&self, f: &mut std::fmt::Formatter) -> std::fmt::Result {

@fulmicoton
Copy link
Collaborator

What an horrible bug. Thanks for spotting!

@fulmicoton fulmicoton merged commit 494e92c into main Aug 22, 2022
@fulmicoton fulmicoton deleted the fix_composite_file branch August 22, 2022 08:52
@@ -60,8 +60,8 @@ impl<W: TerminatingWrite + Write> CompositeWrite<W> {
pub fn for_field_with_idx(&mut self, field: Field, idx: usize) -> &mut CountingWriter<W> {
let offset = self.write.written_bytes();
let file_addr = FileAddr::new(field, idx);
assert!(!self.offsets.contains_key(&file_addr));
self.offsets.insert(file_addr, offset);
assert!(!self.offsets.iter().any(|el| el.0 == file_addr));
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should this really be an assert instead of a debug_assert, i.e. is it necessary for memory safety? This does have linear instead of amortized constant runtime in the number of offsets now, so avoiding the check in release builds would be nice if possible IMHO.

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.

4 participants