feat: evolute all_null_layout to constant layout #5641

Xuanwo · 2026-01-06T18:26:32Z

This PR will evolute all_null_layout to constant layout as mentioned in #5631

The logic for checking validity and constant values is a bit clumsy, but I haven't found a better approach yet.

Parts of this PR were drafted with assistance from Codex (with gpt-5.2) and fully reviewed and edited by me. I take full responsibility for all changes.

github-actions · 2026-01-06T18:27:39Z

Code Review

Overall, this PR cleanly evolves AllNullLayout to ConstantLayout with appropriate backward-compatible protobuf field numbering (slot 2 reused). The implementation is thorough with good test coverage.

P1 Issues

1. Potential integer overflow in is_constant_values (primitive.rs)

In the fixed-width byte comparison path:

let start = (base + i) * byte_width;
if buf[start..start + byte_width] != scalar_bytes[..] {

If base and i are large, and/or byte_width is significant (e.g., 16 bytes for Decimal128), this multiplication could overflow on 32-bit systems. Consider using checked_mul or ensuring the arithmetic cannot overflow via explicit usize bounds.

2. Inconsistent validity handling in encode_constant_page vs detection

The encoder detects constant pages by examining leaf validity via Self::leaf_validity(&repdef, num_values as usize), but the condition for using constant encoding only checks all_null case explicitly:

let all_null = leaf_validity
    .as_ref()
    .map(|validity| validity.count_set_bits() == 0)
    .unwrap_or(false);

When leaf_validity is None (meaning all-valid), it falls through to the constant detection path. This is correct, but the constant detection (find_constant_scalar) then re-derives validity from arrays. Ensure these two validity derivations are semantically consistent across all repdef configurations.

3. Missing validation in decode_scalar_from_inline_value

The function constructs an ArrayData with exactly one buffer (the inline value), but for Boolean data type, Arrow expects a different buffer layout (bit-packed). This could cause issues when decoding boolean constants via inline path. The code should either:

Handle boolean specially (bit-pack the single value)
Reject boolean inline values in try_inline_value

Minor Suggestions

The PR removes all_null_layout and simple_all_null_layout from pb21 but the macro impl_common_protobuf_utils still generates these functions. This leaves dead code that should be cleaned up or the macro simplified.
Consider adding a test for boolean constants to verify correct handling through both inline and value buffer paths.

codecov · 2026-01-06T19:05:35Z

Codecov Report

❌ Patch coverage is 85.23560% with 141 lines in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
...coding/src/encodings/logical/primitive/constant.rs	78.32%	66 Missing and 9 partials ⚠️
rust/lance-arrow/src/scalar.rs	83.08%	28 Missing and 6 partials ⚠️
.../lance-encoding/src/encodings/logical/primitive.rs	92.26%	24 Missing and 6 partials ⚠️
rust/lance-encoding/src/testing.rs	80.00%	2 Missing ⚠️

📢 Thoughts on this report? Let us know!

westonpace

This looks pretty great for a first pass. I have a few thoughts but otherwise this is good!

westonpace · 2026-01-16T13:43:12Z

protos/encodings_v2_1.proto

 }

-// A layout used for pages where all values are null
+// A layout used for pages where all (visible) values are the same scalar value.


What does "visible" mean here?

Except NULL which is invisible, maybe I should just use non-null values?

westonpace · 2026-01-16T13:44:58Z

protos/encodings_v2_1.proto

+  // This MUST only be used for types where a single non-null element is represented by a single
+  // fixed-width Arrow value buffer (i.e. no offsets buffer, no child data).


If we have a single value that is represented by multiple buffers couldn't we concatenate them with a header that gives us information on how to disassemble them? Or maybe we have a different approach for that case?

I did not support that in this PR. One reason is that it would make the encode/decode logic quite complex. Another reason is that I suspect such data is unlikely to share the same constant value.

westonpace · 2026-01-16T13:45:04Z

protos/encodings_v2_1.proto

+  //
+  // Constraints:
+  // - MUST be absent for an all-null page
+  // - MUST be <= 32 bytes if present


Why? If it is larger than 32 bytes do we put it elsewhere?

Yes, if it exceeds 32B, we will place it in a dedicated buffer instead of in the metadata.

The intention here is to avoid bloating our metadata too much. The size is set to the largest fixed data type we support (256B), though I am open to adjusting it.

westonpace · 2026-01-16T13:49:35Z

rust/lance-encoding/src/encodings/logical/primitive.rs

+        use arrow_data::transform::MutableArrayData;
+
+        let data = array.to_data();
+        let mut mutable = MutableArrayData::new(vec![&data], /*use_nulls=*/ true, 1);
+        mutable.extend(0, idx, idx + 1);
+        Ok(make_array(mutable.freeze()))


MutableArrayData is pretty cool. I didn't know it existed. I wonder if we could someday replace some of our data builders with it in the future 🤔

I didn't know about it either! I learned this from GPT-5.2 😆

rust/lance-encoding/src/encodings/logical/primitive.rs

westonpace · 2026-01-16T14:00:24Z

rust/lance-encoding/src/encodings/logical/primitive.rs

+        Ok(Some(scalar))
+    }
+
+    fn encode_constant_page(


Could this go in the submodule?

Yep, will do.

rust/lance-encoding/src/encodings/logical/primitive/constant.rs

Xuanwo added 2 commits January 7, 2026 01:16

Update proto

bc50afb

feat: evolute all_null_layout to constant layout

131449b

Xuanwo requested a review from westonpace January 6, 2026 18:26

github-actions bot added the enhancement New feature or request label Jan 6, 2026

Xuanwo marked this pull request as draft January 6, 2026 18:34

Xuanwo marked this pull request as ready for review January 14, 2026 11:17

Merge branch 'main' into xuanwo/constant-layout

fad42b3

Xuanwo mentioned this pull request Jan 14, 2026

feat: compress complex all null #4990

Open

westonpace added this to the Lance File Version 2.2 milestone Jan 14, 2026

westonpace mentioned this pull request Jan 15, 2026

feat: add a MVP for column statistics at dataset level on Rust side #5639

Open

westonpace reviewed Jan 16, 2026

View reviewed changes

Xuanwo added 5 commits January 21, 2026 21:42

Remove unrelated logic

521a363

remove not needed tests

10db3a7

refactor

164588a

refactor constants

57a049c

Merge branch 'main' into xuanwo/constant-layout

b882173

		// This MUST only be used for types where a single non-null element is represented by a single
		// fixed-width Arrow value buffer (i.e. no offsets buffer, no child data).

feat: evolute all_null_layout to constant layout #5641

Are you sure you want to change the base?

feat: evolute all_null_layout to constant layout #5641

Uh oh!

Conversation

Xuanwo commented Jan 6, 2026

Uh oh!

github-actions bot commented Jan 6, 2026

Code Review

P1 Issues

Minor Suggestions

Uh oh!

codecov bot commented Jan 6, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

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!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

codecov bot commented Jan 6, 2026 •

edited

Loading