-
-
Notifications
You must be signed in to change notification settings - Fork 952
Make doc store binary format more compact (#903) #3009
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -44,7 +44,7 @@ pub use self::writer::StoreWriter; | |
| mod store_compressor; | ||
|
|
||
| /// Doc store version in footer to handle format changes. | ||
| pub(crate) const DOC_STORE_VERSION: DocStoreVersion = DocStoreVersion::V2; | ||
| pub(crate) const DOC_STORE_VERSION: DocStoreVersion = DocStoreVersion::V3; | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
When a pre-V3 segment is merged, the merge path can copy raw document bytes into the new store ( Useful? React with 👍 / 👎. |
||
|
|
||
| #[cfg(feature = "lz4-compression")] | ||
| mod compression_lz4_block; | ||
|
|
@@ -216,6 +216,72 @@ pub(crate) mod tests { | |
| ) | ||
| } | ||
|
|
||
| // Pathological schema with many numeric fields, as suggested in | ||
| // https://github.com/quickwit-oss/tantivy/issues/903 . This exercises the | ||
| // variable-length field-id and integer encoding heavily and verifies that | ||
| // documents still round-trip through the store correctly. | ||
| #[test] | ||
| fn test_store_many_numeric_fields_roundtrip() -> crate::Result<()> { | ||
| use common::DateTime; | ||
|
|
||
| use crate::schema::{OwnedValue, Value}; | ||
|
|
||
| const NUM_U64_FIELDS: usize = 100; | ||
|
|
||
| let mut schema_builder = Schema::builder(); | ||
| let u64_fields: Vec<_> = (0..NUM_U64_FIELDS) | ||
| .map(|i| schema_builder.add_u64_field(&format!("u64_{i}"), STORED)) | ||
| .collect(); | ||
| let i64_field = schema_builder.add_i64_field("i64", STORED); | ||
| let date_field = schema_builder.add_date_field("date", STORED); | ||
| let schema = schema_builder.build(); | ||
|
|
||
| let path = Path::new("store"); | ||
| let directory = RamDirectory::create(); | ||
| let store_wrt = directory.open_write(path)?; | ||
| { | ||
| let mut store_writer = | ||
| StoreWriter::new(store_wrt, Compressor::default(), BLOCK_SIZE, true)?; | ||
| for i in 0..NUM_DOCS as u64 { | ||
| let mut doc = TantivyDocument::default(); | ||
| for (j, field) in u64_fields.iter().enumerate() { | ||
| doc.add_u64(*field, i.wrapping_mul(j as u64 + 1)); | ||
| } | ||
| // Include both signs to exercise zig-zag encoding. | ||
| doc.add_i64(i64_field, (i as i64) - (NUM_DOCS as i64) / 2); | ||
| doc.add_date(date_field, DateTime::from_timestamp_nanos(i as i64 * 1_000)); | ||
| store_writer.store(&doc, &schema)?; | ||
| } | ||
| store_writer.close()?; | ||
| } | ||
|
|
||
| let store_file = directory.open_read(path)?; | ||
| let store = StoreReader::open(store_file, 10)?; | ||
| for i in 0..NUM_DOCS as u64 { | ||
| let doc = store.get::<TantivyDocument>(i as u32)?; | ||
| for (j, field) in u64_fields.iter().enumerate() { | ||
| assert_eq!( | ||
| doc.get_first(*field).unwrap().as_value().as_u64().unwrap(), | ||
| i.wrapping_mul(j as u64 + 1) | ||
| ); | ||
| } | ||
| assert_eq!( | ||
| doc.get_first(i64_field) | ||
| .unwrap() | ||
| .as_value() | ||
| .as_i64() | ||
| .unwrap(), | ||
| (i as i64) - (NUM_DOCS as i64) / 2 | ||
| ); | ||
| let expected_date = DateTime::from_timestamp_nanos(i as i64 * 1_000); | ||
| match doc.get_first(date_field).unwrap().as_value().into() { | ||
| OwnedValue::Date(dt) => assert_eq!(dt, expected_date), | ||
| other => panic!("expected date, got {other:?}"), | ||
| } | ||
| } | ||
| Ok(()) | ||
| } | ||
|
|
||
| #[test] | ||
| fn test_store_with_delete() -> crate::Result<()> { | ||
| let mut schema_builder = schema::Schema::builder(); | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -31,12 +31,17 @@ type Block = OwnedBytes; | |
| pub(crate) enum DocStoreVersion { | ||
| V1 = 1, | ||
| V2 = 2, | ||
| /// Same as V2 but field ids and integers are stored using variable-length | ||
| /// encoding (`VInt`, with zig-zag for signed integers) to make blocks more | ||
| /// compact before compression. | ||
| V3 = 3, | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Introducing V3 here changes the on-disk store bytes, but Useful? React with 👍 / 👎. |
||
| } | ||
| impl Display for DocStoreVersion { | ||
| fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { | ||
| match self { | ||
| DocStoreVersion::V1 => write!(f, "V1"), | ||
| DocStoreVersion::V2 => write!(f, "V2"), | ||
| DocStoreVersion::V3 => write!(f, "V3"), | ||
| } | ||
| } | ||
| } | ||
|
|
@@ -49,6 +54,7 @@ impl BinarySerializable for DocStoreVersion { | |
| Ok(match u32::deserialize(reader)? { | ||
| 1 => DocStoreVersion::V1, | ||
| 2 => DocStoreVersion::V2, | ||
| 3 => DocStoreVersion::V3, | ||
| v => { | ||
| return Err(io::Error::new( | ||
| io::ErrorKind::InvalidData, | ||
|
|
@@ -499,7 +505,7 @@ mod tests { | |
| assert_eq!(store.cache_stats().cache_hits, 1); | ||
| assert_eq!(store.cache_stats().cache_misses, 2); | ||
|
|
||
| assert_eq!(store.cache.peek_lru(), Some(232206)); | ||
| assert_eq!(store.cache.peek_lru(), Some(229266)); | ||
|
|
||
| Ok(()) | ||
| } | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For a V3 store whose field-id vint is corrupted or produced by a non-conforming writer with a value above
u32::MAX, this cast silently truncates it to anotherFieldinstead of reporting data corruption. That can make stored values appear under the wrong field when the wrapped id happens to be valid, so this should validate the vint with a checkedu32conversion before constructing theField.Useful? React with 👍 / 👎.