fix(index): report shuffle-stage failures instead of panicking - #9405
Open
LuciferYang wants to merge 3 commits into
Open
LuciferYang wants to merge 3 commits into
LuciferYang wants to merge 3 commits into
Conversation
Two streaming shuffle paths unwrapped tokio::spawn JoinHandle results (.map(|x| x.unwrap())), so a panic inside a transform task re-panicked in the pipeline and aborted the process instead of surfacing as a lance Error. Convert JoinError into Error::internal. Assisted-by: GLM-5.3
Contributor
There was a problem hiding this comment.
✅ Gate recommendation: approve.
The change handles the reachable duplicate-column failure at its source and prevents both shuffle streams from re-panicking on transform-task failures, while preserving their existing concurrency and ordering. The focused regression test covers the invalid-input path and its diagnostic context.
Contributor
Author
|
mac-build failed in lance-index::vector::kmeans::tests::test_hierarchical_kmeans_is_seeded_and_balanced, a size-balance assertion in the kmeans trainer. This PR only touches the shuffle streams in rust/lance/src/index/vector/builder.rs, so it cannot reach that test; it passes locally on this branch. Every other job is green. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #9404
Two steps of the same failure. Attaching precomputed partition ids to a batch that already has
__ivf_part_idhittry_with_column's duplicate-name error behind an.expect, so it panicked inside the spawned transform task; the stream then unwrapped the resultingJoinError, re-raising that panic in the consumer instead of returning it frombuild(). The attach now returns aninvalid_inputerror naming the batch schema, and both shuffle streams (shuffle_dataandreshuffle_partitions) turn aJoinErrorinto an error rather than unwrapping it.The
JoinErrormapping is the safety net for any other panic in that closure, so it stays even though the one reachable panic is now handled at the source.How was this patch tested?
test_shuffle_data_reports_duplicate_partition_id_columndrivesshuffle_datawith a precomputed partitions file and a batch that already carries the partition column, and asserts the error names the attach. Restoring the.expectmakes the test fail with the error theJoinErrormapping produces,shuffle transform task failed: task N panicked with message "failed to add part id column: ...", which is the mapping doing its job; restoring both lines makes the panic escape the stream again.