fix(index): error on a malformed precomputed partitions file - #9342
Open
LuciferYang wants to merge 2 commits into
Open
LuciferYang wants to merge 2 commits into
LuciferYang wants to merge 2 commits into
Conversation
load_precomputed_partitions extracted the row_id and partition columns with .expect and unchecked downcasts, so a user-supplied file with a missing column or the wrong physical type panicked the build mid-way. Return descriptive input errors naming the expected columns and types. Assisted-by: GLM-5.3
Contributor
There was a problem hiding this comment.
✅ Gate recommendation: approve.
The loader now rejects missing, mistyped, or nullable partition columns before their values enter index construction, while preserving valid multi-batch mappings. This addresses both the panic path and silent null-based misassignment without changing the well-formed file contract.
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 #9341
load_precomputed_partitionspulled therow_idandpartitioncolumns out of a user-supplied file with.expectand an uncheckedas_primitive, so a missing column or a wrong physical type ended the index build in a panic. The user picks that file:create_index(..., precomputed_partition_dataset=<uri>)checks only that the dataset has one fragment and one data file before the builder opens it and scans whatever schema it finds. Both lookups now go throughas_primitive_optand returninvalid_input, naming the column, the type expected and what the file has instead —expected a UInt64 'row_id' column, found Int64, orno such columnwhen it is absent.Nulls get rejected too, and that one is not a panic but a wrong index. Both columns are read through their values buffer, which ignores validity, and the writer that produces these files marks the fields nullable, so a null
partitionwas read as whatever the buffer held at that slot and the row went into an arbitrary partition with nothing reported.The scope is that one function, which is the
create_indexpath. The sibling loader inrust/lance/src/index/vector/ivf/builder.rsthattransform_vectors(partition_ds_uri=...)uses has the same two.expects, and one line further it writeslookup[addr.fragment_id()][addr.row_offset()]with no bounds check, so a file whose row ids belong to a different dataset panics there on an index out of bounds. That needs the bounds check designed alongside the type checks, so it is left for a follow-up rather than half-fixed here.How was this patch tested?
test_load_precomputed_partitions_rejects_wrong_row_id_typeand..._rejects_wrong_partition_typecover the wrong-type cases,..._rejects_missing_partition_columncovers the branch that replaced the.expect, and..._rejects_nullscovers a nullpartition. Each asserts the error variant as well as the message...._merges_batchesis the happy path over two batches, which also pins that the accumulator merges rather than replaces.Both halves were checked by mutation: disabling the null guard fails
..._rejects_nulls, and restoring.expectwith the uncheckedas_primitivefails..._rejects_wrong_row_id_typeinside arrow'scast.rs.