Skip to content

Conversation

@Xuanwo
Copy link
Collaborator

@Xuanwo Xuanwo commented Dec 29, 2025

This PR will avoid panic while hitting non-null empty multi-vector


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.

@Xuanwo Xuanwo requested a review from BubbleCal December 29, 2025 10:32
@github-actions github-actions bot added the bug Something isn't working label Dec 29, 2025
@github-actions
Copy link
Contributor

Code Review

P0: Bug in flat.rs - Panic replaced with silently propagated panic

The change in flat.rs from .await.unwrap() to .await? addresses the wrong layer of the problem:

// Before
.await.unwrap()

// After
.await?

The .unwrap() here is for the JoinHandle result from spawn_blocking, which panics if the spawned task panicked. Replacing it with ? changes the behavior: now if the spawned task panics, the panic is converted to a JoinError and propagated as an error rather than panicking the current task. This is actually a behavior change that may hide panics.

Recommendation: If the intent is to properly handle task panics, this change is fine but should be documented. However, the original issue ("panic while hitting non-null empty multi-vector") should be fixed by the error handling in multivec_distance, not by changing how join errors are handled.

P1: Error message could include more context

The error in multivec_distance is clear but consider including which field/column this is from if available, to help users identify which data is malformed in large datasets.

Test Coverage

The test adequately covers the new error path. 👍


Overall the core fix in multivec_distance converting a panic into a proper error for empty multi-vectors is correct and well-tested.

Copy link
Contributor

@BubbleCal BubbleCal left a comment

Choose a reason for hiding this comment

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

Where does it panic before this change?

let multivector = v.as_fixed_size_list();
match distance_type {
if multivector.len() == 0 {
return Err(ArrowError::InvalidArgumentError(format!(
Copy link
Contributor

Choose a reason for hiding this comment

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

can we just assume the distance to a empty multivector is NaN and then exclude it from the results?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Let me know if 0a61738 is what you want

@Xuanwo
Copy link
Collaborator Author

Xuanwo commented Dec 29, 2025

Where does it panic before this change?

Our tokio runtime worker panic at

Resulted by

multivector
.values()
.as_primitive::<UInt8Type>()
.values()
.chunks_exact(dim)
.map(|v| hamming::hamming(q, v))
.min_by(|a, b| a.partial_cmp(b).unwrap())
.unwrap()

@codecov
Copy link

codecov bot commented Dec 29, 2025

Codecov Report

❌ Patch coverage is 90.90909% with 3 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
rust/lance/src/io/exec/knn.rs 75.00% 0 Missing and 2 partials ⚠️
rust/lance-linalg/src/distance.rs 96.00% 1 Missing ⚠️

📢 Thoughts on this report? Let us know!

@Xuanwo Xuanwo merged commit 768d66a into main Dec 30, 2025
31 checks passed
@Xuanwo Xuanwo deleted the xuanwo/fix-downstream-build branch December 30, 2025 10:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants