fix: return error instead of panicking on values too wide for miniblock

[zhangyue19921010]

Return error instead of panicking on values too wide for miniblock

Summary by CodeRabbit

• Bug Fixes
  • Improved handling of oversized values during miniblock encoding by returning a clear InvalidInput error instead of failing unexpectedly.
  • Fixed fixed-width and fixed-size-list miniblock encoding paths to consistently propagate invalid input failures.
  • Expanded test coverage for wide binary and list inputs (including boolean lists) to verify the error type and that the message includes both the “too wide” detail and the expected value requirement.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

[yanghua]

Left two comments you can consider.

[yanghua]

Is there always 2 values? May it be 16?

If the answer is yes, I'd suggest that we do not hardcode.

[zhangyue19921010]

changed.

[yanghua]

Maybe we also need to add a test for fxl?

[zhangyue19921010]

done.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 2a0a2369-2290-4697-af7a-6bfaef457d12

📥 Commits

Reviewing files that changed from the base of the PR and between 64f25c4 and cf609d4.

📒 Files selected for processing (1)

• rust/lance-encoding/src/encodings/physical/value.rs

💤 Files with no reviewable changes (1)

• rust/lance-encoding/src/encodings/physical/value.rs

---

📝 Walkthrough

Walkthrough

The ValueEncoder miniblock path now returns explicit Result errors when values are too wide for miniblock encoding. Helper functions, compressor call sites, and tests were updated to propagate and verify InvalidInput instead of panicking.

Changes

ValueEncoder Result-based error handling

Layer / File(s)	Summary
Width validation returns Result instead of asserting rust/lance-encoding/src/encodings/physical/value.rs	find_log_vals_per_chunk now returns Result<(u64, u64)> and emits Error::invalid_input(...) when the computed miniblock size exceeds MAX_MINIBLOCK_BYTES.
Propagate Result through chunk helpers and compressor rust/lance-encoding/src/encodings/physical/value.rs	chunk_data, chunk_fsl, and miniblock_fsl now return Result, wrap successful outputs in Ok(...), and MiniBlockCompressor::compress forwards the fallible paths with ? or direct Result return.
Error message tests rust/lance-encoding/src/encodings/physical/value.rs	Wide binary and fixed-size-list helpers were added, and the test asserts InvalidInput with both the miniblock width message and the formatted values-required detail.

Estimated code review effort: 2 (Simple) | ~12 minutes

Sequence Diagram(s)

sequenceDiagram
  participant Compress as MiniBlockCompressor::compress
  participant ChunkData as chunk_data / miniblock_fsl
  participant Finder as find_log_vals_per_chunk

  Compress->>ChunkData: chunk_data(fixed_width) / miniblock_fsl(chunk)
  ChunkData->>Finder: find_log_vals_per_chunk(...)
  Finder-->>ChunkData: Err(InvalidInput) if size exceeds MAX_MINIBLOCK_BYTES
  ChunkData-->>Compress: propagate Err via ?

Loading

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly matches the main change: converting miniblock width panics into an error return.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

_{Comment @coderabbitai help to get the list of available commands.}

[coderabbitai]

🧹 Nitpick comments (1)

rust/lance-encoding/src/encodings/physical/value.rs (1)

30-54: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Document the "2 * values_per_word" minimum-chunk rationale.

The width-check and its num_values = 2 * values_per_word calculation encode an important invariant (minimum viable miniblock chunk is 2 words, i.e. 2 or 16 values depending on word size) that previously caused reviewer confusion (see past comment asking "Is there always 2 values? May it be 16?"). The current code correctly generalizes this, but a short inline comment explaining why the minimum is 2 * values_per_word (rather than 1) would prevent the same question from resurfacing for future readers.

📝 Suggested doc comment

     fn find_log_vals_per_chunk(bytes_per_word, values_per_word) -> Result<(u64, u64)> {
         let mut size_bytes = 2 * bytes_per_word;
         let (mut log_num_vals, mut num_vals) = match values_per_word {
             1 => (1, 2),
             8 => (3, 8),
             _ => unreachable!(),
         };
 
+        // A miniblock chunk must hold at least 2 words worth of values (log_num_values >= 1),
+        // so the smallest possible chunk requires `2 * values_per_word` values. If even that
+        // minimum doesn't fit, the value is too wide to encode as a miniblock at all.
         if size_bytes >= MAX_MINIBLOCK_BYTES {
             let num_values = 2 * values_per_word;

As per coding guidelines, "Add doc comments to magic constants, thresholds, and non-obvious transformation functions, explaining what the value represents and why it was chosen."

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@rust/lance-encoding/src/encodings/physical/value.rs` around lines 30 - 54,
Add a short inline comment in find_log_vals_per_chunk explaining why the minimum
miniblock chunk is modeled as 2 * values_per_word, since the width check and
num_values calculation rely on that invariant. Clarify that the smallest viable
chunk is two words, which corresponds to 2 values for 1-word types or 16 values
for 8-word types, so future readers understand why the threshold is not a single
word.

Source: Coding guidelines

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Nitpick comments:
In `@rust/lance-encoding/src/encodings/physical/value.rs`:
- Around line 30-54: Add a short inline comment in find_log_vals_per_chunk
explaining why the minimum miniblock chunk is modeled as 2 * values_per_word,
since the width check and num_values calculation rely on that invariant. Clarify
that the smallest viable chunk is two words, which corresponds to 2 values for
1-word types or 16 values for 8-word types, so future readers understand why the
threshold is not a single word.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 55fa1469-8e84-4b76-8509-8621336448d1

📥 Commits

Reviewing files that changed from the base of the PR and between 9c04698 and 64f25c4.

📒 Files selected for processing (1)

• rust/lance-encoding/src/encodings/physical/value.rs

[yanghua]

LGTM