feat: bitpack with miniblock #3067

broccoliSpicy · 2024-10-30T16:55:47Z

This PR tries to add bit-packing encoding in the mini-block encoding path.
In this PR, each chunk(1024 values) has it's own bit-width parameter and it's stored in each chunk

I found that the current implementation to get the bit_width of every 1024 values is very slow and hurts the write speed significantly, more investigation needed, I will deal with it by filling a different issue and PR.

#3052

codecov-commenter · 2024-10-30T17:20:22Z

Codecov Report

Attention: Patch coverage is 81.42857% with 13 lines in your changes missing coverage. Please review.

Project coverage is 77.48%. Comparing base (270cab3) to head (dcabd0f).
Report is 4 commits behind head on main.

Files with missing lines	Patch %	Lines
...coding/src/encodings/physical/bitpack_fastlanes.rs	80.39%	10 Missing ⚠️
rust/lance-encoding/src/encoder.rs	77.77%	2 Missing ⚠️
rust/lance-encoding/src/encodings/physical.rs	0.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3067      +/-   ##
==========================================
- Coverage   77.60%   77.48%   -0.12%     
==========================================
  Files         240      240              
  Lines       78683    78752      +69     
  Branches    78683    78752      +69     
==========================================
- Hits        61059    61024      -35     
- Misses      14496    14590      +94     
- Partials     3128     3138      +10

Flag	Coverage Δ
unittests	`77.48% <81.42%> (-0.12%)`	⬇️

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.

github-actions · 2024-10-30T19:21:49Z

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.

westonpace

This is really cool! A few changes are needed but I'm excited to make progress on this.

westonpace · 2024-10-30T20:02:29Z

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

@@ -561,7 +561,7 @@ impl IndexWorker {
    }
 }

-pub(crate) struct PostingReader {


These changes don't seem relevant? Can we remove them?

oh, sure, might be some automatic lint fix

westonpace · 2024-10-30T21:04:01Z

protos/encodings.proto

+
+  // The items in the list
+  Buffer buffer = 3;


We probably don't need buffer. That can be a concern for structural encodings (e.g. PageLayout).

fixed. Thanks!

westonpace · 2024-10-30T21:04:53Z

protos/encodings.proto

@@ -263,6 +271,7 @@ message ArrayEncoding {
        FixedSizeBinary fixed_size_binary = 11;
        BitpackedForNonNeg bitpacked_for_non_neg = 12;
        Constant constant = 13;
+        Bitpack2 bitpack2 = 14;


We don't need it for this PR but I'm thinking of putting the new 2.1 encodings in message Compression { } instead of ArrayEncoding. On the plus side, we can probably name it bitpack and not bitpack2 once we do that. Let's save for future work though.

gotcha, good idea.

westonpace · 2024-10-30T21:05:39Z

python/play.py

@@ -0,0 +1,96 @@
+import pyarrow as pa


Does this need to be included in this PR? If so, we should rename it and move it in a benchmark somwhere. Although I'd rather just keep it out.

oh, sorry, will remove it. I keep git rm this file, then I get some lint issues then I git add . it back haha

fixed, thanks.

westonpace · 2024-10-30T21:06:49Z

rust/lance-encoding/src/encoder.rs

    ) -> Result<Box<dyn MiniBlockCompressor>> {
        assert!(field.data_type().byte_width() > 0);
+        if let DataBlock::FixedWidth(ref fixed_width_data) = data {
+            if fixed_width_data.bits_per_value <= 64 {


Should we also check that it is a multiple of 8? Or maybe that it is one of [8, 16, 32, 64]? Would your logic work (for example) with fixed-size-list<int8, 3> which has a width of 24?

yeah, good idea. I should make sure it is one of 8, 16, 32, 64.
one interesting issue may need to be considered:
when input is fixed-size-list<int8, 4>, we can bitpack it and get back the decoding result back correctly, but the compression ratio is probably bad, we may also need to make sure don't use bitpack when hit this case

westonpace · 2024-10-30T21:08:31Z

rust/lance-encoding/src/encodings/physical/bitpack_fastlanes.rs

+// and the bit-width parameter has the same bit-width as the uncompressed DataBlock
+// for example, if the input DataBlock has `bits_per_value` of `16`, there will be 2 bytes(16 bits)
+// in front of each chunk storing the `bit-width` parameter.


Why not just always use 1 byte?

yeah, this is related to the type of output vector, it's type is the same as input data type(to satisfy signature of Bitpack::unchecked_pack), so when we are filling in bit-width into output, it's type is same as input data type

I think for fastlanes bitpack compression to work, its start output buffer position must be aligned to the same input data type alignment.
I will do same experiments to test it.

I think it will panic to squeeze in a u8 into a vector of u64 and then treat the rest of vector as u64, because the alignment requirement doesn't hold anymore.

westonpace · 2024-10-30T21:08:47Z

rust/lance-encoding/src/encodings/physical/bitpack_fastlanes.rs

+        let bit_widths = $data
+            .get_stat(Stat::BitWidth)
+            .expect("FixedWidthDataBlock should have valid bit width statistics");
+        println!("bit_widths statistics got");


Suggested change

println!("bit_widths statistics got");

fixed, thanks.

westonpace · 2024-10-30T21:12:08Z

rust/lance-encoding/src/encodings/physical/bitpack_fastlanes.rs

+            .downcast_ref::<PrimitiveArray<UInt64Type>>()
+            .unwrap();
+
+        let (packed_chunk_sizes, total_size) = bit_widths_array


Are chunk_size and total_size a "number of values" measurement or a "number of bytes" measurement?

packed_chunk_sizes and total_size are "number of values" measurement, it needs to be this way because the function signature Bitpack::unchecked_pack requires the output slice type to be the same as the input slice, so when we use total_size to

let mut output: Vec<$data_type> = Vec::with_capacity(total_size);

the output size is total_size with number of bytes of total_size * sizeof<each element>

westonpace · 2024-10-30T21:15:51Z

rust/lance-encoding/src/encodings/physical/bitpack_fastlanes.rs

+            }
+            chunks.push(MiniBlockChunk {
+                num_bytes: ((1 + packed_chunk_sizes[i]) * std::mem::size_of::<$data_type>()) as u16,
+                log_num_values: 10,


Since ELEMS_PER_CHUNK is a constant it would be good for this to be a constant too. Maybe just LOG_ELEMENTS_PER_CHUNK.

fixed, thanks!

westonpace · 2024-10-30T21:17:40Z

rust/lance-encoding/src/encodings/physical/bitpack_fastlanes.rs

+                // Copy for memory alignment
+                let chunk_in_u8: Vec<u8> = data.to_vec();


I see the memory alignment concern now. Let's not worry about it now. In the future to fix this I think we need...

Each page must be aligned (preferably to something largish like 64 bytes)

The miniblock layout is responsible for ensuring that the mini-block buffer (that we get from the compressor) is either at the start of the page or is padded to sufficient alignment (again, can just do 64)

Each miniblock chunk should be aligned. The compressor can return a preferred alignment (we want to be more conservative here and use the smallest alignment we can) and the layout can be responsible for doing the actual padding.

here, I need to cast the raw data fetched from memory into slice of u8, u16, u32, or u64 based on their uncompressed_bit_width, to make this cast successful, I need to make sure the data being cast has the alignment requirement that a slice of u8, u16, u32, u64 needs(multiple of 8 bytes).
the bit-packed chunk itself guarantees that(it's a multiple of 1024 bits(128 bytes), if mini-chunk layout can guarantee that, then this copy may be eliminated

bitpack with miniblock

964fee3

github-actions bot added enhancement New feature or request python labels Oct 30, 2024

broccoliSpicy added 2 commits October 30, 2024 19:03

add tests for miniblock bitpack

30d7f51

lint

cc6e468

broccoliSpicy changed the title ~~feat: bitpack with miniblock~~ feat: bitpack with miniblock, each chunk has it's own bit-width parameter and it's stored in each chunk Oct 30, 2024

broccoliSpicy requested a review from westonpace October 30, 2024 19:22

broccoliSpicy changed the title ~~feat: bitpack with miniblock, each chunk has it's own bit-width parameter and it's stored in each chunk~~ feat: bitpack with miniblock Oct 30, 2024

broccoliSpicy added 5 commits October 30, 2024 19:44

lint 2

1f15274

lint 3

22159e9

remove a test script

9f5dc6d

delete a irrelevant comment

1ad38c1

another lint

edae11a

westonpace requested changes Oct 30, 2024

View reviewed changes

broccoliSpicy added 4 commits October 31, 2024 14:39

address PR comments

0f688fd

address PR comments 2

ca96e21

remove a test script

553e124

lint

dcabd0f

broccoliSpicy requested a review from westonpace October 31, 2024 18:58

westonpace approved these changes Oct 31, 2024

View reviewed changes

broccoliSpicy merged commit 54053e6 into lancedb:main Oct 31, 2024
23 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: bitpack with miniblock #3067

feat: bitpack with miniblock #3067

broccoliSpicy commented Oct 30, 2024 •

edited

Loading

codecov-commenter commented Oct 30, 2024 •

edited

Loading

github-actions bot commented Oct 30, 2024

westonpace left a comment

westonpace Oct 30, 2024

broccoliSpicy Oct 30, 2024

broccoliSpicy Oct 31, 2024

westonpace Oct 30, 2024

broccoliSpicy Oct 31, 2024

westonpace Oct 30, 2024

broccoliSpicy Oct 31, 2024

westonpace Oct 30, 2024

broccoliSpicy Oct 30, 2024

broccoliSpicy Oct 31, 2024

westonpace Oct 30, 2024

broccoliSpicy Oct 30, 2024

westonpace Oct 30, 2024

broccoliSpicy Oct 30, 2024

broccoliSpicy Oct 30, 2024 •

edited

Loading

broccoliSpicy Oct 31, 2024

westonpace Oct 30, 2024

broccoliSpicy Oct 31, 2024

westonpace Oct 30, 2024

broccoliSpicy Oct 30, 2024

westonpace Oct 30, 2024

broccoliSpicy Oct 31, 2024

westonpace Oct 30, 2024

broccoliSpicy Oct 31, 2024

		// Copy for memory alignment
		let chunk_in_u8: Vec<u8> = data.to_vec();

feat: bitpack with miniblock #3067

feat: bitpack with miniblock #3067

Conversation

broccoliSpicy commented Oct 30, 2024 • edited Loading

codecov-commenter commented Oct 30, 2024 • edited Loading

Codecov Report

github-actions bot commented Oct 30, 2024

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

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

broccoliSpicy Oct 30, 2024 • edited Loading

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

broccoliSpicy commented Oct 30, 2024 •

edited

Loading

codecov-commenter commented Oct 30, 2024 •

edited

Loading

broccoliSpicy Oct 30, 2024 •

edited

Loading