refactor: move IVF_HNSW_SQ & IVF_FLAT to new buliding & search path #2469

BubbleCal · 2024-06-14T09:59:23Z

IVF_HNSW_SQ for new search/build path
IVF_FLAT e2e pass
support to train quantizer with new index builder
fix partition order broken after building
clean IVF related types
index builder method chaining for customizing
impl merging deltas for new IVF_HNSW_SQ & IVF_FLAT

github-actions · 2024-06-14T09:59:44Z

ACTION NEEDED
Lance follows the Conventional Commits specification for release automation.

The PR title and description are used as the merge commit message. Please update your PR title and description to match the specification.

For details on the error please inspect the "PR Title Check" action.

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

…actor

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

…actor

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

…actor

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

codecov-commenter · 2024-06-22T10:25:21Z

Codecov Report

Attention: Patch coverage is 66.92708% with 381 lines in your changes missing coverage. Please review.

Project coverage is 79.63%. Comparing base (7cd11b8) to head (12d7a90).
Report is 6 commits behind head on main.

Files	Patch %	Lines
rust/lance/src/index/vector/builder.rs	70.00%	43 Missing and 56 partials ⚠️
rust/lance/src/index/vector/ivf.rs	61.93%	48 Missing and 11 partials ⚠️
rust/lance-index/src/vector/flat/index.rs	43.47%	26 Missing ⚠️
rust/lance/src/index.rs	62.31%	0 Missing and 26 partials ⚠️
rust/lance-index/src/vector/quantizer.rs	45.23%	20 Missing and 3 partials ⚠️
rust/lance-index/src/vector/hnsw/index.rs	5.00%	19 Missing ⚠️
rust/lance/src/index/vector/ivf/v2.rs	89.59%	9 Missing and 9 partials ⚠️
rust/lance-index/src/vector/ivf/storage.rs	77.92%	14 Missing and 3 partials ⚠️
rust/lance/src/session/index_extension.rs	0.00%	17 Missing ⚠️
rust/lance/src/index/vector/pq.rs	44.44%	15 Missing ⚠️
... and 11 more

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2469      +/-   ##
==========================================
+ Coverage   79.55%   79.63%   +0.07%     
==========================================
  Files         208      207       -1     
  Lines       59351    59187     -164     
  Branches    59351    59187     -164     
==========================================
- Hits        47217    47131      -86     
+ Misses       9371     9268     -103     
- Partials     2763     2788      +25

Flag	Coverage Δ
unittests	`79.63% <66.92%> (+0.07%)`	⬆️

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>

wjones127

This is a big refactor! Thanks for working on this. I have a few minor comments but it looks good overall.

wjones127 · 2024-06-24T17:10:31Z

rust/lance/src/session/index_extension.rs

+            _: &Query,
+            _: Arc<dyn PreFilter>,
+        ) -> Result<RecordBatch> {
+            todo!("panic")


Maybe these should all be unimplemented!()?

wjones127 · 2024-06-24T17:15:56Z

rust/lance/src/index/vector/ivf/v2.rs

+        let row_ids = result
+            .column_by_name(ROW_ID)
+            .unwrap()


FYI, this can be simplified to

Suggested change

let row_ids = result

.column_by_name(ROW_ID)

.unwrap()

let row_ids = result[ROW_ID]

wjones127 · 2024-06-24T17:17:42Z

rust/lance/src/index/vector/ivf.rs

+    existing_indices: &[Arc<dyn Index>],
+    options: &OptimizeOptions,
+) -> Result<(Uuid, usize)> {
+    // Senity check the indices


Suggested change

// Senity check the indices

// Sanity check the indices

wjones127 · 2024-06-24T17:20:34Z

rust/lance/src/index/vector/ivf.rs

+    match index_type {
+        (SubIndexType::Flat, QuantizationType::Flat) => {
+            IvfIndexBuilder::<FlatIndex, FlatQuantizer>::new_incremental(
+                dataset.clone(),


Should this method take an Arc<Dataset> instead if we need to clone?

make sense, will do it next PR cause this requires to modify the whole path

wjones127 · 2024-06-24T17:28:26Z

rust/lance/src/index/vector/builder.rs

+        let partition_build_order = (0..ivf.num_partitions())
+            .map(|partition_id| reader.partiton_size(partition_id))
+            .collect::<Result<Vec<_>>>()?
+            // sort by partition size in descending order
+            .into_iter()
+            .enumerate()
+            .map(|(idx, x)| (x, idx))
+            .sorted()
+            .rev()
+            .map(|(_, idx)| idx)
+            .collect::<Vec<_>>();


How expensive is reader.partition_size()? I feel like this could simply be:

Suggested change

let partition_build_order = (0..ivf.num_partitions())

.map(|partition_id| reader.partiton_size(partition_id))

.collect::<Result<Vec<_>>>()?

// sort by partition size in descending order

.into_iter()

.enumerate()

.map(|(idx, x)| (x, idx))

.sorted()

.rev()

.map(|(_, idx)| idx)

.collect::<Vec<_>>();

let mut partition_build_order = (0..ivf.num_partitions()).collect::<Vec<_>>();

partition_build_order.sort_by_key(|part_id| -reader.partition_size(part_id));

this can't be done because:

partition_size returns Result<usize>

can'y apply "-" on usize
but i simplify this a little

wjones127 · 2024-06-24T17:30:57Z

rust/lance/src/index/vector/builder.rs

+                );
+            }
+
+            match reader.partiton_size(partition)? {


Spelling

Suggested change

match reader.partiton_size(partition)? {

match reader.partition_size(partition)? {

westonpace

This PR is large so I think the best path forward is just to go ahead and merge it and we can work through the details as we test it.

How do I use the new path today? Is it always used for HNSW indices (and the old path still always used for IVF_PQ indices)?

Do you anticipate using the new path for IVF_PQ indices? Is there some way I can try that out today? Or do I need to hack in some code changes to test that?

BubbleCal · 2024-06-25T02:57:10Z

This PR is large so I think the best path forward is just to go ahead and merge it and we can work through the details as we test it.

How do I use the new path today? Is it always used for HNSW indices (and the old path still always used for IVF_PQ indices)?

Do you anticipate using the new path for IVF_PQ indices? Is there some way I can try that out today? Or do I need to hack in some code changes to test that?

This new path can be used for IVF_FLAT and IVF_HNSW_SQ only now. The current PQ implementation is a little complicated so it needs more work to make it work with the new path.

So far there's no simple way to get IVF_PQ work on the new path

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

…to hnsw-refactor

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

…ancedb#2469) - IVF_HNSW_SQ for new search/build path - IVF_FLAT e2e pass - support to train quantizer with new index builder - fix partition order broken after building - clean IVF related types - index builder method chaining for customizing - impl merging deltas for new IVF_HNSW_SQ & IVF_FLAT --------- Signed-off-by: BubbleCal <bubble-cal@outlook.com>

BubbleCal requested review from westonpace and chebbyChefNEQ June 14, 2024 09:59

BubbleCal force-pushed the hnsw-refactor branch from 72d40a3 to 04dc9c9 Compare June 14, 2024 10:10

BubbleCal changed the title ~~refactor: IVF_HNSW_SQ & IVF_FLAT~~ refactor: support IVF_HNSW_SQ & IVF_FLAT e2e Jun 14, 2024

BubbleCal force-pushed the hnsw-refactor branch from 04dc9c9 to 6ae764c Compare June 14, 2024 10:17

github-actions bot added the python label Jun 14, 2024

BubbleCal force-pushed the hnsw-refactor branch from 6ae764c to b52010f Compare June 14, 2024 11:10

refactor: clean IVF_HNSW_SQ

50c9785

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

BubbleCal force-pushed the hnsw-refactor branch from b52010f to 50c9785 Compare June 14, 2024 11:10

BubbleCal added 3 commits June 14, 2024 19:59

fix incompability caused by metadata key changing

26e538b

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

remove legacy code

9197a13

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

more

f83b4c3

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

BubbleCal changed the title ~~refactor: support IVF_HNSW_SQ & IVF_FLAT e2e~~ refactor: move IVF_HNSW_SQ & IVF_FLAT to new buliding & search path Jun 18, 2024

BubbleCal added 16 commits June 18, 2024 16:08

resolve conflicts

7645782

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

finish optimize

894fb04

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

test optimize

3adcc44

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

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

adf5b4e

…actor

fix

99b696c

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

fix

b1eb655

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

fmt

60ec0bf

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

fix recall

9f8d360

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

norm dot

3977e54

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

fix

660c6d9

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

fix

36e466f

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

fmt

46d10de

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

fix offset

d71cbb6

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

fix ut

7c90826

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

fix merging

8bb0474

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

fix num_partitions

9ec7067

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

BubbleCal added 5 commits June 21, 2024 11:25

Merge branch 'main' into hnsw-refactor

071b640

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

c409255

…actor

fix

1b603a5

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

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

b2db3e1

…actor

add logs

c9866a7

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

BubbleCal added 2 commits June 22, 2024 22:25

fix vector cache

9132d5a

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

Merge branch 'main' into hnsw-refactor

02cdc0a

wjones127 approved these changes Jun 24, 2024

View reviewed changes

westonpace approved these changes Jun 24, 2024

View reviewed changes

BubbleCal added 3 commits June 25, 2024 12:12

fix comments

8df19ae

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

Merge branch 'hnsw-refactor' of https://github.com/BubbleCal/lance in…

9e900b5

…to hnsw-refactor

fix temp dir cleaned before building finished

12d7a90

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

BubbleCal merged commit f51c5f0 into lancedb:main Jun 25, 2024
21 of 22 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: move IVF_HNSW_SQ & IVF_FLAT to new buliding & search path #2469

refactor: move IVF_HNSW_SQ & IVF_FLAT to new buliding & search path #2469

BubbleCal commented Jun 14, 2024 •

edited

Loading

github-actions bot commented Jun 14, 2024

codecov-commenter commented Jun 22, 2024 •

edited

Loading

wjones127 left a comment

wjones127 Jun 24, 2024

wjones127 Jun 24, 2024

wjones127 Jun 24, 2024

wjones127 Jun 24, 2024

BubbleCal Jun 25, 2024

wjones127 Jun 24, 2024

BubbleCal Jun 25, 2024

wjones127 Jun 24, 2024

westonpace left a comment

BubbleCal commented Jun 25, 2024

	match reader.partiton_size(partition)? {
	match reader.partition_size(partition)? {

refactor: move IVF_HNSW_SQ & IVF_FLAT to new buliding & search path #2469

refactor: move IVF_HNSW_SQ & IVF_FLAT to new buliding & search path #2469

Conversation

BubbleCal commented Jun 14, 2024 • edited Loading

github-actions bot commented Jun 14, 2024

codecov-commenter commented Jun 22, 2024 • edited Loading

Codecov Report

wjones127 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

westonpace left a comment

Choose a reason for hiding this comment

BubbleCal commented Jun 25, 2024

BubbleCal commented Jun 14, 2024 •

edited

Loading

codecov-commenter commented Jun 22, 2024 •

edited

Loading