feat: packed struct encoding by broccoliSpicy · Pull Request #3186 · lance-format/lance

broccoliSpicy · 2024-11-29T15:03:46Z

This PR tries to add packed struct encoding.

During encoding, it packs a struct with fixed width fields, producing a row oriented FixedWidthDataBlock, then use ValueCompressor to compressor to a MiniBlock Layout.

during decoding, it first uses ValueDecompressor to get the row-oriented FixedWidthDataBlock, then construct a StructDataBlock for output.

#3173 #2601

broccoliSpicy · 2024-11-29T22:17:55Z

the current main(lance 2.0 packed-struct encoding) doesn't work with the benchmark script in python/benchmarks/test_packed_struct.py -k "read"

codecov-commenter · 2024-12-04T22:26:51Z

Codecov Report

Attention: Patch coverage is 88.92734% with 32 lines in your changes missing coverage. Please review.

Project coverage is 78.51%. Comparing base (ef9d0c2) to head (b15eec9).

Files with missing lines	Patch %	Lines
...encoding/src/encodings/physical/struct_encoding.rs	88.67%	10 Missing and 2 partials ⚠️
rust/lance-file/src/v2/reader.rs	57.14%	6 Missing and 3 partials ⚠️
rust/lance-encoding/src/data.rs	96.36%	2 Missing ⚠️
rust/lance-encoding/src/decoder.rs	89.47%	0 Missing and 2 partials ⚠️
rust/lance-encoding/src/encoder.rs	77.77%	1 Missing and 1 partial ⚠️
.../lance-encoding/src/encodings/logical/primitive.rs	75.00%	1 Missing and 1 partial ⚠️
rust/lance-encoding/src/statistics.rs	95.34%	2 Missing ⚠️
rust/lance-encoding/src/encodings/physical.rs	0.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3186      +/-   ##
==========================================
+ Coverage   78.45%   78.51%   +0.06%     
==========================================
  Files         244      245       +1     
  Lines       84554    84818     +264     
  Branches    84554    84818     +264     
==========================================
+ Hits        66333    66595     +262     
+ Misses      15418    15406      -12     
- Partials     2803     2817      +14

Flag	Coverage Δ
unittests	`78.51% <88.92%> (+0.06%)`	⬆️

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.

broccoliSpicy · 2024-12-05T15:15:59Z

packed-struct with FixedSizeList is not supported for now.

of struct is fixed-width data block when using packed-struct encoding.

broccoliSpicy · 2024-12-09T17:00:16Z

lance 2.1 benchmark result(10 million rows):
write:

random access read(100 randomly generated indices for random access):

random access read, 1000 randomly generated indices for random access:

full scan read(batch_size: 1000):
when parquet read batch size not set:

when parquet read batch size is also set to 1000:

full scan read(lance batch_size: 16 * 1024):
when parquet read batch size not set:

when parquet read batch size is also set to 16 * 1024

westonpace

A few suggestions but this approach seems like a good starting point overall.

westonpace · 2024-12-10T12:57:57Z

    /// the projection will be invalid and the read will fail.
+    /// If the field is a `struct datatype` with `packed` set to true in the field metadata,
+    /// the whole struct has one column index.
+    /// To support nested `packed-struct encoding`, this method need to be further adjusted.


Suggested change

/// To support nested `packed-struct encoding`, this method need to be further adjusted.

This sentence seems redundant

this sentence is to document nested packed struct encoding is not supported at the moment.

westonpace · 2024-12-10T13:10:47Z

+            {
+                column_indices.push(curr_column_idx);
+                curr_column_idx += 1;
+                packed_struct_fields_num = field.children.len();


This won't be correct on nested structs. For example:

A-+ | | | | B D-+ | | | | | | C E F

If A is packed then field.children.len() will be 2 but there are 5 elements that need to be skipped. I think it would be easiest to just do our own DFS here.

pub fn from_whole_schema(schema: &Schema, version: LanceFileVersion) -> Self { let schema = Arc::new(schema.clone()); let is_structural = version >= LanceFileVersion::V2_1; let mut counter = 0; let mut column_indices = Vec::new(); for field in schema.fields { from_whole_field(field, &mut counter, &mut column_indices); } Self { schema, column_indices, } } fn from_whole_field(field: &Field, counter: &mut u32, column_indices: &mut Vec<u32>) { if is_packed_struct(field) { add column to column indices return early } if is_primitive or !is_structural { add column to column indices } for child in field.children { recurse } }

nested packed struct is not supported for now, in fact, the packed struct encoding will panic if encounter nested packed struct so currently in 2.1 nested packed struct data can not be written.

inside PrimitiveStructuralEncoder::do_flush:

// if the `data_block` is a `StructDataBlock`, then this is a struct with packed struct encoding. if let DataBlock::Struct(ref struct_data_block) = data_block { if struct_data_block .children .iter() .any(|child| !matches!(child, DataBlock::FixedWidth(_))) { panic!("packed struct encoding currently only supports fixed-width fields.") } }

westonpace · 2024-12-10T13:11:07Z

+            }
+            if field_metadata
+                .get("packed")
+                .map(|v| v == "true")


Let's make this a case insensitive comparison

westonpace · 2024-12-10T13:12:13Z

+    fn get_stat(&self, stat: Stat) -> Option<Arc<dyn Array>> {
+        match stat {
+            Stat::MaxLength => {
+                let max_len = self.dimension * self.child.expect_single_stat::<UInt64Type>(stat);


Does every block have MaxLength? Is it possible the FSL's child doesn't have MaxLength?

sorry, this is vestige, statistics for fixedSizeList data block is not supported in this PR.

westonpace · 2024-12-10T13:15:49Z

+                Some(Arc::new(UInt64Array::from(vec![max_len])))
+            }
+            Stat::DataSize => self.child.get_stat(stat),
+            _ => None,


What about NullCount?

removed this code completely as fixed size list statistics is not supported in this PR.

westonpace · 2024-12-10T13:18:45Z

+                let field_metadata = &field.metadata;
+                if field_metadata
+                    .get("packed")
+                    .map(|v| v == "true")


Let's make this case insensitive.

Also, this check repeats in a few places. We could maybe add is_packed_struct to FieldExt in lance-arrow/src/schema.rs

thanks, fixed.
I added two is_packed_struct, one in lance-arrow/src/schema.rs, one in lance-core/src/datatypes.field.rs, not sure how to do only one.

westonpace · 2024-12-10T13:20:55Z

+        let mut children = vec![];
+
+        let bytes_per_row: u32 = bits_per_values.iter().sum::<u32>() / 8;
+        let bytes_per_row = bytes_per_row as u64;


Let's add a debug assert to verify our assumption that bits_per_values.iter().all(|bpv| bpv % 8 == 0)

fixed, thanks.

westonpace · 2024-12-10T13:23:22Z

+                if field_metadata
+                    .get("packed")
+                    .map(|v| v == "true")
+                    .unwrap_or(false)


Also case insensitive (or replace with call to helper method)

github-actions Bot added enhancement New feature or request python labels Nov 29, 2024

packed-struct encoding

72d6920

broccoliSpicy force-pushed the packed-struct-encoding branch from ff7ddfd to 72d6920 Compare December 4, 2024 16:57

correct estimated_bytes computation

409a465

broccoliSpicy changed the title ~~feat: packed struct encoding(draft, wip)~~ feat: packed struct encoding Dec 4, 2024

broccoliSpicy requested a review from westonpace December 4, 2024 22:02

broccoliSpicy changed the title ~~feat: packed struct encoding~~ feat: packed struct encoding(wip) Dec 4, 2024

a workable version for schema with more than one columns

cd8343e

broccoliSpicy changed the title ~~feat: packed struct encoding(wip)~~ feat: packed struct encoding Dec 5, 2024

broccoliSpicy and others added 7 commits December 5, 2024 10:21

remove fixed-size-list field in benchmark script

90f82e7

Merge branch 'main' into packed-struct-encoding

4297877

remove the added statistics gathering for FixedSizeListBlock

5bfc8c8

Merge remote-tracking branch 'origin/packed-struct-encoding'

292f0d7

fix merge error

ba4c6d6

fmt

ce89ff1

fmt

9a5a13f

broccoliSpicy mentioned this pull request Dec 6, 2024

additional functionalities to support packed-struct encoding #3187

Open

broccoliSpicy added 3 commits December 6, 2024 18:07

fix bug with create_structural_field_scheduler

c38ed70

correct estimate size in StructDataBlockBuilder.

c28b3cf

add check in PrimitiveStructuralEncoder::do_flush to make sure fields

40e29c9

of struct is fixed-width data block when using packed-struct encoding.

broccoliSpicy and others added 3 commits December 9, 2024 15:20

use random float number in test_packed_struct.py

f54c873

fmt

5f632f5

Merge branch 'main' into packed-struct-encoding

ac74812

westonpace approved these changes Dec 10, 2024

View reviewed changes

address PR comments

bf73273

broccoliSpicy and others added 4 commits December 10, 2024 10:49

Merge remote-tracking branch 'origin/packed-struct-encoding'

e2b6c9a

Merge branch 'main' into packed-struct-encoding

b15eec9

address PR comments

d4b5a00

Merge remote-tracking branch 'origin/packed-struct-encoding'

7bab88c

broccoliSpicy merged commit c4cb87a into lance-format:main Dec 10, 2024

Conversation

broccoliSpicy commented Nov 29, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

broccoliSpicy commented Nov 29, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov-commenter commented Dec 4, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

broccoliSpicy commented Dec 5, 2024

Uh oh!

broccoliSpicy commented Dec 9, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

westonpace left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

broccoliSpicy commented Nov 29, 2024 •

edited

Loading

broccoliSpicy commented Nov 29, 2024 •

edited

Loading

codecov-commenter commented Dec 4, 2024 •

edited

Loading

broccoliSpicy commented Dec 9, 2024 •

edited

Loading