Skip to content

fix(encoding): prevent bits_per_value=0 in FullZipLayout for FSL with AllNull child - #9130

Open
westonpace wants to merge 2 commits into
lance-format:mainfrom
westonpace:fix/fsl-all-nulls
Open

fix(encoding): prevent bits_per_value=0 in FullZipLayout for FSL with AllNull child#9130
westonpace wants to merge 2 commits into
lance-format:mainfrom
westonpace:fix/fsl-all-nulls

Conversation

@westonpace

Copy link
Copy Markdown
Member

If we had a fixed-size-list array where each list was non-null but consisted only of null elements (e.g. FSL<2> = [[NULL, NULL], [NULL, NULL], [NULL, NULL]]) then we would store it with bits_per_values=0 and the resulting file would be unreadable.

This PR fixes the issue. Arguably we could do it more efficiently if we turn this into a special case (we currently still store the validity bytes which is redundant information) but this is probably a unique enough case to not worry about too much yet.

Details of fix with four coordinated changes in value.rs:

  1. fsl_to_encoding: instead of returning constant(None) early for AllNull children, fall through to the inner-encoding match, set has_validity=true, and emit fsl(dim, has_validity=true, constant(None)). This tells the decoder that validity bytes are present per row.

  2. nullable_per_value_fsl AllNull arm: add an all-zero validity buffer to validity_iters and increment bytes_per_row by cum_dim.div_ceil(8). bytes_per_row is now >= 1, so bits_per_value > 0 in the layout.

  3. ValueDecompressor::from_fsl: handle Compression::Constant as a terminal case. Only the validity bytes count toward bits_per_value; bits_per_item=0 signals the all-null path to unzip_decompress.

  4. unzip_decompress: when bits_per_item==0 produce DataBlock::AllNull for the inner block instead of FixedWidth{bits_per_value:0}. AllNull::into_arrow creates a properly-typed all-null array for any element type.

Backward compatibility: existing files whose AllNull-child FSL used constant(None) directly as the inner encoding still decode via ConstantDecompressor unchanged; only new files take the new path.

… AllNull child

When nullable_per_value_fsl received a FixedSizeList whose child DataBlock
was AllNull (no Nullable wrapper), it left bytes_per_row at 0 and wrote
bits_per_value=0 into the FullZipLayout proto.  On read, if no ctrl-word
bytes existed either (no outer null buffer, no rep/def levels), the decoder
would error with "per-row byte width must be greater than 0".

Fix with four coordinated changes in value.rs:

1. fsl_to_encoding: instead of returning constant(None) early for AllNull
   children, fall through to the inner-encoding match, set has_validity=true,
   and emit fsl(dim, has_validity=true, constant(None)).  This tells the
   decoder that validity bytes are present per row.

2. nullable_per_value_fsl AllNull arm: add an all-zero validity buffer to
   validity_iters and increment bytes_per_row by cum_dim.div_ceil(8).
   bytes_per_row is now >= 1, so bits_per_value > 0 in the layout.

3. ValueDecompressor::from_fsl: handle Compression::Constant as a terminal
   case.  Only the validity bytes count toward bits_per_value; bits_per_item=0
   signals the all-null path to unzip_decompress.

4. unzip_decompress: when bits_per_item==0 produce DataBlock::AllNull for the
   inner block instead of FixedWidth{bits_per_value:0}.  AllNull::into_arrow
   creates a properly-typed all-null array for any element type.

Backward compatibility: existing files whose AllNull-child FSL used
constant(None) directly as the inner encoding still decode via
ConstantDecompressor unchanged; only new files take the new path.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@github-actions github-actions Bot added A-encoding Encoding, IO, file reader/writer bug Something isn't working labels Sep 10, 2026

@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 current-reader round trip is fixed, but stable writers must keep emitting value-encoding shapes understood by the latest released stable reader. Preserve the v2.1 forward-compatibility contract—for example, retain a FixedSizeList → Flat terminal with masked leaf-width payload bytes, or restrict the new encoding composition to an unstable file version.

DataBlock::AllNull(_) => {
return ProtobufUtils21::constant(None);
has_validity = true;
ProtobufUtils21::constant(None)

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.

This makes stable v2.1 writers emit FixedSizeList(has_validity=true) → Constant(None). Lance v11.0.0 is the latest stable release and resolves stable to v2.1, but its ValueDecompressor::from_fsl accepts only nested FixedSizeList and a terminal Flat; this terminal reaches _ => unreachable!(). Files written through this new path therefore panic in that released reader. The same-version round-trip test cannot detect this forward-compatibility break. Please keep stable writes on an encoding shape v11 understands (for example, a Flat terminal with masked payload bytes at the actual leaf width), or version-gate this composition to an unstable format.

Reproducer against v11.0.0
#[test]
fn accepts_nested_constant_fsl_value_encoding() {
    let encoding = super::ProtobufUtils21::fsl(
        4,
        true,
        super::ProtobufUtils21::constant(None),
    );
    let Compression::FixedSizeList(fsl) = encoding.compression.unwrap() else {
        panic!("expected fixed-size-list encoding");
    };

    let _ = ValueDecompressor::from_fsl(fsl.as_ref());
}

cargo test -p lance-encoding accepts_nested_constant_fsl_value_encoding -- --nocapture

Expected: the released stable reader accepts the stable v2.1 encoding. Observed: internal error: entered unreachable code in ValueDecompressor::from_fsl.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

The old writer would have crashed without this change. We are not making things worse in terms of compatibility.

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.

The old writer failure is the bug being fixed, but it does not make the new stable encoding forward-compatible: this patch makes a v2.1 file readable by the current reader while the released stable v11 reader still panics on its nested Constant terminal. The repository contract requires every new stable-format write to remain readable by the released stable reader, even when the old writer could not successfully produce that edge case, so this finding remains.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I accept this as a known risk. Masked payload bytes would be inefficient and this is a fairly niche corner case already.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Actually, I guess I'll try and fix it.

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.

The follow-up commit only permits zero-width pages in the current FullZip scheduler; it does not change fsl_to_encoding, which still emits stable v2.1 FixedSizeList → Constant. The released v11 reader therefore still reaches its unsupported terminal, so the compatibility finding remains.

@lance-gatekeeper lance-gatekeeper Bot added the K-changes Latest Gatekeeper recommendation requests changes. label Sep 10, 2026
…alue=0

Files written by the buggy encoder (before the previous commit) contained
FullZip pages with bits_per_value=0 and constant(None) inner encoding and
no ctrl-word bytes.  The decoder previously rejected these with:

  "Invalid encoding: per-row byte width must be greater than 0"

Two changes in primitive.rs:

1. create_decoder: remove the hard error for total_bytes_per_value==0.
   This case is now valid for constant-null pages with no ctrl-word bytes.

2. FixedFullZipDecoder::drain: add an early-return path for
   total_bytes_per_value==0 that skips buffer slicing (which would
   divide-by-zero) and returns a task whose single item has an empty
   data buffer.  ConstantDecompressor::decompress ignores its input and
   returns AllNull(num_rows), so the decode task already produces the
   correct all-null output.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@lance-gatekeeper lance-gatekeeper Bot removed the K-changes Latest Gatekeeper recommendation requests changes. label Sep 10, 2026

@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 follow-up broadens the current reader to accept zero-width FullZip pages, but it does not address the stable-format compatibility blocker: v2.1 writers still emit a FixedSizeList → Constant terminal that the released v11 reader cannot decode. Keep stable output on a v11-readable shape (such as FixedSizeList → Flat with masked leaf-width payload bytes), or restrict this composition to an unstable format version.

@lance-gatekeeper lance-gatekeeper Bot added the K-changes Latest Gatekeeper recommendation requests changes. label Sep 10, 2026
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 bug Something isn't working K-changes Latest Gatekeeper recommendation requests changes.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant