feat: implement disk-based inverted index #2643

BubbleCal · 2024-07-26T05:02:58Z

read posting list from reader of inverted file
cache posting list
switch to v2 format
flatten the inverted file to reduce the number of IO operations
use binary search to speed up wand
reorg files, divide the inverted.rs into builder.rs, index.rs and wand.rs

Signed-off-by: BubbleCal <bubble-cal@outlook.com>

BubbleCal · 2024-07-26T05:07:06Z

rust/lance-index/src/scalar/inverted/builder.rs

these are all moved from inverted.rs

BubbleCal · 2024-07-26T05:07:47Z

rust/lance-index/src/scalar/inverted/index.rs

Most of them are moved from inverted.rs

Signed-off-by: BubbleCal <bubble-cal@outlook.com>

BubbleCal · 2024-07-26T05:19:07Z

rust/lance-index/src/scalar/inverted/index.rs

+        let frequency_col =
+            Float32Array::from_iter_values(indices.iter().map(|&i| self.frequencies[i]));
+
+        let block_head_indices = block_head_indices(self.len());


read first element of each block, and store them right before the posting list

codecov-commenter · 2024-07-26T05:37:59Z

Codecov Report

Attention: Patch coverage is 63.28438% with 275 lines in your changes missing coverage. Please review.

Project coverage is 79.68%. Comparing base (72ff096) to head (221a264).

Files	Patch %	Lines
rust/lance-index/src/scalar/inverted/index.rs	64.86%	114 Missing and 16 partials ⚠️
rust/lance-index/src/scalar/inverted/builder.rs	66.66%	63 Missing and 14 partials ⚠️
rust/lance-index/src/scalar/lance_format.rs	47.77%	38 Missing and 9 partials ⚠️
rust/lance-index/src/scalar/inverted/wand.rs	73.52%	8 Missing and 1 partial ⚠️
rust/lance-index/src/scalar/inverted.rs	0.00%	6 Missing ⚠️
rust/lance-index/src/scalar.rs	20.00%	4 Missing ⚠️
rust/lance-arrow/src/lib.rs	83.33%	1 Missing ⚠️
rust/lance/src/io/exec/fts.rs	0.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2643      +/-   ##
==========================================
- Coverage   79.81%   79.68%   -0.14%     
==========================================
  Files         224      226       +2     
  Lines       65871    65981     +110     
  Branches    65871    65981     +110     
==========================================
+ Hits        52572    52574       +2     
- Misses      10231    10325      +94     
- Partials     3068     3082      +14

Flag	Coverage Δ
unittests	`79.68% <63.28%> (-0.14%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Signed-off-by: BubbleCal <bubble-cal@outlook.com>

…ed-inverted

BubbleCal · 2024-07-29T10:17:42Z

rust/lance-index/src/scalar/bitmap.rs

@@ -347,14 +346,3 @@ pub async fn train_bitmap_index(

    do_train_bitmap_index(batches_source, dictionary, index_store).await
 }
-
-pub async fn train_inverted_index(


Move to inverted.rs

Signed-off-by: BubbleCal <bubble-cal@outlook.com>

…ed-inverted

Signed-off-by: BubbleCal <bubble-cal@outlook.com>

wjones127 · 2024-07-29T22:28:49Z

rust/lance-index/src/scalar/inverted/builder.rs

+        let batch = RecordBatch::try_new(
+            arrow_schema::Schema::new(vec![
+                arrow_schema::Field::new("doc", doc_col.data_type().to_owned(), false),
+                arrow_schema::Field::new(super::ROW_ID, arrow_schema::DataType::UInt64, false),


FYI there is a ROW_ID_FIELD constant we can use for this instead.

wjones127 · 2024-07-29T22:33:41Z

rust/lance-index/src/scalar/inverted/builder.rs

+    let mut tokenizer = TOKENIZER.clone();
+    let mut stream = new_data;
+    while let Some(batch) = stream.try_next().await? {
+        let doc_col = batch.column(0).as_string::<Offset>();


IMO it doesn't seem worth making this whole method generic over the offsets. Could we instead write a function that does:

fn iter_str_array(array: &dyn Array) -> Box<dyn Iterator<Item = Option<&str>> + '_>

And iterate over that boxed iterator?

wjones127 · 2024-07-29T22:41:56Z

rust/lance-index/src/scalar/inverted/builder.rs

+                let row_ids = row_ids.as_primitive::<UInt64Type>().values().to_vec();
+                let frequencies = frequencies.as_primitive::<Float32Type>().values().to_vec();


It seems like you are doing O(n_tokens) mem copies, which seems very inefficient. Have you considered making the PostingList storing the arrays directly? Or a ScalarBuffer?

Here I don't store them as array because i saw it's much slower than Vec, can try scalar buffer and switch to it if there's no obvious perf impact

Signed-off-by: BubbleCal <bubble-cal@outlook.com>

westonpace

A few suggestions, no big concerns.

westonpace · 2024-07-30T01:26:03Z

rust/lance-index/src/scalar.rs

@@ -64,14 +68,21 @@ pub trait IndexStore: std::fmt::Debug + Send + Sync + DeepSizeOf {
    /// Create a new file and return a writer to store data in the file
    async fn new_index_file(&self, name: &str, schema: Arc<Schema>)
        -> Result<Box<dyn IndexWriter>>;
+    async fn new_index_file_v2(


Let's make a GH issue to move all scalar indices to V2 so we can get rid of these methods eventually.

here it is, will work on that once this gets merged

No rush to work on it. It will probably be a bit tricky because we will need to make sure old indices continue to function.

westonpace · 2024-07-30T01:27:58Z

rust/lance-index/src/scalar/inverted/wand.rs

+
+// WAND parameters
+lazy_static! {
+    static ref FACTOR: f32 = std::env::var("WAND_FACTOR")


Why an env variable? Is it because we don't have a good way of specifying scalar config or because we doubt users will ever need to change this?

If this is changed does it mean retrain is needed?

Going to move it into query parameter, changing this doesn't require retrain cause it only impacts the recall/performance for query

westonpace · 2024-07-30T04:27:11Z

rust/lance-index/src/scalar/lance_format.rs

+
+#[async_trait]
+impl IndexReader for v2::reader::FileReader {
+    async fn read_record_batch(&self, offset: u32) -> Result<RecordBatch> {


Might be better to just panic with unimplemented!. Reading a batch of size u32::MAX into memory isn't going to end well.

westonpace · 2024-07-30T04:28:07Z

rust/lance-index/src/scalar/lance_format.rs

+        let path = self.index_dir.child(name);
+
+        let other_store = dest_store.as_any().downcast_ref::<Self>();
+        if let Some(dest_lance_store) = other_store {
+            // If both this store and the destination are lance stores we can use object_store's copy
+            // This does blindly assume that both stores are using the same underlying object_store
+            // but there is no easy way to verify this and it happens to always be true at the moment
+            let dest_path = dest_lance_store.index_dir.child(name);
+            self.object_store.copy(&path, &dest_path).await
+        } else {
+            let reader = self.open_index_file_v2(name).await?;
+            let num_batches = reader.num_batches().await;
+            if num_batches == 0 {
+                return Err(Error::Internal {
+                    message:
+                        "Cannot copy an empty index file because the schema cannot be determined"
+                            .into(),
+                    location: location!(),
+                });
+            }
+            let first_batch = reader.read_record_batch(0).await?;
+            let schema = first_batch.schema();
+            let mut writer = dest_store.new_index_file_v2(name, schema).await?;
+            writer.write_record_batch(first_batch).await?;
+            for batch_index in 1..num_batches {
+                writer
+                    .write_record_batch(reader.read_record_batch(batch_index).await?)
+                    .await?;
+            }
+            writer.finish().await?;
+            Ok(())
+        }
+    }


Can we make a free function and avoid the repetition? This method is identical to the v1 counterpart right?

it's not the same after make that read_record_batch() unimplemented, and this requires to expose the index_dir and object_store.

westonpace · 2024-07-30T04:29:24Z

rust/lance-index/src/scalar/inverted/builder.rs

+        let token_set_batch = self.tokens.to_batch()?;
+        let mut token_set_writer = dest_store
+            .new_index_file_v2(TOKENS_FILE, token_set_batch.schema())
+            .await?;
+        token_set_writer.write_record_batch(token_set_batch).await?;
+        token_set_writer.finish().await?;
+
+        let invert_list_batch = self.invert_list.to_batch()?;
+        let mut invert_list_writer = dest_store
+            .new_index_file_v2(INVERT_LIST_FILE, invert_list_batch.schema())
+            .await?;
+        invert_list_writer
+            .write_record_batch(invert_list_batch)
+            .await?;
+        invert_list_writer.finish().await?;
+
+        let docs_batch = self.docs.to_batch()?;
+        let mut docs_writer = dest_store
+            .new_index_file_v2(DOCS_FILE, docs_batch.schema())
+            .await?;
+        docs_writer.write_record_batch(docs_batch).await?;
+        docs_writer.finish().await?;


Can we put this in a helper function? It looks very similar to the logic in remap (and maybe the train function?)

Signed-off-by: BubbleCal <bubble-cal@outlook.com>

wjones127

Thanks. Nice work here.

…ed-inverted

- read posting list from reader of inverted file - cache posting list - switch to v2 format - flatten the inverted file to reduce the number of IO operations - use binary search to speed up wand - reorg files, divide the `inverted.rs` into `builder.rs`, `index.rs` and `wand.rs` --------- Signed-off-by: BubbleCal <bubble-cal@outlook.com>

feat: implement disk-based inverted index

2cded09

Signed-off-by: BubbleCal <bubble-cal@outlook.com>

github-actions bot added the enhancement New feature or request label Jul 26, 2024

BubbleCal commented Jul 26, 2024

View reviewed changes

rust/lance-index/src/scalar/inverted/builder.rs

Copy link

Contributor Author

BubbleCal Jul 26, 2024

Choose a reason for hiding this comment

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

these are all moved from inverted.rs

BubbleCal commented Jul 26, 2024

View reviewed changes

rust/lance-index/src/scalar/inverted/index.rs

Copy link

Contributor Author

BubbleCal Jul 26, 2024

Choose a reason for hiding this comment

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

Most of them are moved from inverted.rs

rename

921c421

Signed-off-by: BubbleCal <bubble-cal@outlook.com>

BubbleCal commented Jul 26, 2024

View reviewed changes

BubbleCal added 2 commits July 26, 2024 14:30

improve perf

2cdf94e

Signed-off-by: BubbleCal <bubble-cal@outlook.com>

more comments

7293506

Signed-off-by: BubbleCal <bubble-cal@outlook.com>

BubbleCal mentioned this pull request Jul 26, 2024

Full text search (FTS) indices #1195

Open

7 tasks

BubbleCal added 11 commits July 26, 2024 18:04

switch to v2 format

b24ea77

Signed-off-by: BubbleCal <bubble-cal@outlook.com>

optimize IO

f515822

Signed-off-by: BubbleCal <bubble-cal@outlook.com>

configurable IO SIZE

11057ed

Signed-off-by: BubbleCal <bubble-cal@outlook.com>

increase the min IO size

f6ebf7e

Signed-off-by: BubbleCal <bubble-cal@outlook.com>

optimize

1ff9e9c

Signed-off-by: BubbleCal <bubble-cal@outlook.com>

remove arrow

e2cbeee

Signed-off-by: BubbleCal <bubble-cal@outlook.com>

improve

c04aab1

Signed-off-by: BubbleCal <bubble-cal@outlook.com>

read entire list

88222cf

Signed-off-by: BubbleCal <bubble-cal@outlook.com>

clean

e594c83

Signed-off-by: BubbleCal <bubble-cal@outlook.com>

fix example

bd59c58

Signed-off-by: BubbleCal <bubble-cal@outlook.com>

Merge branch 'main' of https://github.com/lancedb/lance into disk-bas…

a4450f8

…ed-inverted

BubbleCal requested review from eddyxu, westonpace and wjones127 July 29, 2024 10:12

BubbleCal commented Jul 29, 2024

View reviewed changes

BubbleCal added 3 commits July 29, 2024 18:17

remove comments

ad96779

Signed-off-by: BubbleCal <bubble-cal@outlook.com>

Merge branch 'main' of https://github.com/lancedb/lance into disk-bas…

22a6216

…ed-inverted

fmt

9cf8f03

Signed-off-by: BubbleCal <bubble-cal@outlook.com>

wjones127 reviewed Jul 29, 2024

View reviewed changes

fix

937c61b

Signed-off-by: BubbleCal <bubble-cal@outlook.com>

westonpace reviewed Jul 30, 2024

View reviewed changes

BubbleCal added 3 commits July 30, 2024 17:42

fix comments

04c0803

Signed-off-by: BubbleCal <bubble-cal@outlook.com>

fix comments

fade6a5

Signed-off-by: BubbleCal <bubble-cal@outlook.com>

add wand_factor parameter for FTS

9bdffc3

Signed-off-by: BubbleCal <bubble-cal@outlook.com>

BubbleCal requested review from westonpace and wjones127 July 30, 2024 11:20

BubbleCal added 2 commits July 30, 2024 19:27

fmt

01ec483

Signed-off-by: BubbleCal <bubble-cal@outlook.com>

ignore null doc

fede010

Signed-off-by: BubbleCal <bubble-cal@outlook.com>

westonpace approved these changes Jul 31, 2024

View reviewed changes

wjones127 approved these changes Jul 31, 2024

View reviewed changes

Merge branch 'main' of https://github.com/lancedb/lance into disk-bas…

221a264

…ed-inverted

BubbleCal merged commit 9e7d97f into lancedb:main Aug 1, 2024
21 of 22 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: implement disk-based inverted index #2643

feat: implement disk-based inverted index #2643

BubbleCal commented Jul 26, 2024 •

edited

Loading

BubbleCal Jul 26, 2024

BubbleCal Jul 26, 2024

BubbleCal Jul 26, 2024

codecov-commenter commented Jul 26, 2024 •

edited

Loading

BubbleCal Jul 29, 2024

wjones127 Jul 29, 2024

wjones127 Jul 29, 2024

wjones127 Jul 29, 2024

BubbleCal Jul 30, 2024

westonpace left a comment

westonpace Jul 30, 2024

BubbleCal Jul 30, 2024

westonpace Jul 31, 2024

westonpace Jul 30, 2024

BubbleCal Jul 30, 2024

westonpace Jul 30, 2024

westonpace Jul 30, 2024

BubbleCal Jul 30, 2024

westonpace Jul 30, 2024

wjones127 left a comment

		let row_ids = row_ids.as_primitive::<UInt64Type>().values().to_vec();
		let frequencies = frequencies.as_primitive::<Float32Type>().values().to_vec();

feat: implement disk-based inverted index #2643

feat: implement disk-based inverted index #2643

Conversation

BubbleCal commented Jul 26, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-commenter commented Jul 26, 2024 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

westonpace left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wjones127 left a comment

Choose a reason for hiding this comment

BubbleCal commented Jul 26, 2024 •

edited

Loading

codecov-commenter commented Jul 26, 2024 •

edited

Loading