Skip to content

refactor(encoding): align block codecs with miniblock - #8324

Open
Xuanwo wants to merge 1 commit into
mainfrom
xuanwo/generic-block-v5-1-contract
Open

refactor(encoding): align block codecs with miniblock#8324
Xuanwo wants to merge 1 commit into
mainfrom
xuanwo/generic-block-v5-1-contract

Conversation

@Xuanwo

@Xuanwo Xuanwo commented Aug 6, 2026

Copy link
Copy Markdown
Collaborator

Stack 1 of 10 for Lance generic block sequence compression. Builds on merged #8038.

This refactors block compression to follow the existing mini-block architecture: concrete codecs own descriptor construction, payload framing, and validation, while the shared strategy only dispatches. It removes the previous plan/factory shape so malformed or unsupported codec trees fail at the concrete codec boundary.

This layer introduces no protobuf variants or production selector changes. Stable Lance 2.0–2.2 bytes and reader behavior remain unchanged.

Stack navigation

@lance-gatekeeper lance-gatekeeper Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Gate recommendation: request changes.

The codec-owned descriptor direction is worthwhile, but stable writers must preserve the selected compression configuration byte-for-byte. Keep that configuration with the selected codec and add exact descriptor regressions for omitted Zstd levels and explicit LZ4 levels.

Please mark this PR with the breaking-change label.

Ok(LanceBuffer::from(compressed))
Ok((
Some(LanceBuffer::from(compressed)),
ProtobufUtils21::wrapped(self.compressor.config(), inner_encoding)?,

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Serializing self.compressor.config() changes stable General descriptors from the selected configuration. With explicit Zstd and no level, construction normalizes None to runtime level 0 and this emits Some(0); LZ4 does the inverse for explicit levels because its runtime config() always returns None. Keep the originally selected CompressionConfig with this codec and serialize that exact value.

Reproducer

I ran this addable regression at 2228153208f842a2411316688c6c10916ce6b07b:

#[test]
#[cfg(feature = "zstd")]
fn gate_reproducer_general_block_preserves_absent_zstd_level() {
    let mut params = CompressionParams::new();
    params.columns.insert(
        "dict_values".to_string(),
        CompressionFieldParams {
            compression: Some("zstd".to_string()),
            compression_level: None,
            ..Default::default()
        },
    );
    let strategy = strategy(TestEncoding::StructuralU32, params);
    let field = create_test_field("dict_values", DataType::FixedSizeBinary(3));
    let data = create_fixed_width_block(24, 1024);

    let compressor = strategy.create_block_compressor(&field, &data).unwrap();
    let (_, encoding) = compressor.compress(data).unwrap();
    let Some(Compression::General(general)) = encoding.compression.as_ref() else {
        panic!("expected general compression");
    };

    assert_eq!(general.compression.as_ref().unwrap().level, None);
}

cargo test -p lance-encoding gate_reproducer_general_block_preserves_absent_zstd_level --lib

Expected None; observed Some(0).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

A-encoding Encoding, IO, file reader/writer

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant