test(merge_insert): cover the reordered full-schema duplicate-key path - #8637
Open
LuciferYang wants to merge 1 commit into
Open
test(merge_insert): cover the reordered full-schema duplicate-key path#8637LuciferYang wants to merge 1 commit into
LuciferYang wants to merge 1 commit into
Conversation
A full-schema source whose fields are merely reordered was classified as a partial schema on the indexed slow path, and the target key columns were then located by offsetting the source key positions. That offset landed on a nullable payload column, so every matched row looked unmatched and took the insert branch, which carries the real target _rowaddr into the in-place column rewrite. A repeated key fed the same address into the update stream twice and the reconciler reported a row-address ordering violation (lance-format#8282) instead of the ambiguity the caller has to act on. PR lance-format#8509 fixes that lookup for lance-format#8280, and measurement shows it fixes this shape too: before it the merge fails with "update row address (0, 0) is missing from the target fragment", and after it the merge fails with the ambiguous-key rejection naming id = 1. The two regressions lance-format#8509 adds cover partial-schema sources with a single row, so nothing guards the full-schema InsertAll duplicate-key path this test exercises. Both routing preconditions are asserted rather than assumed, because the ambiguity rejection has several call sites and the v2 fast-path one emits a byte-identical message: without the index the duplicate is caught by the v2 writer, and without the subschema misclassification no _rowaddr reaches the update stream. Either way the reconciler is never involved and a message-only assertion would still pass. The message is asserted instead of the Error variant on purpose. This error crosses the sort/repartition in update_fragments, so DataFusion returns it shared behind an Arc and the conversion in lance-core yields Execution or InvalidInput depending on how many partitions still hold a reference. The variant tracks the machine's CPU count; the text does not.
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.
Summary
Adds one regression test for the shape reported in #8282: an indexed
merge_insertwhose source carries every target field in a different order and repeats a join key.That shape has no coverage today. #8509 fixed the misaligned target-key lookup behind #8280, and measurement shows the same change also fixes #8282, but the two regressions #8509 added both use a partial-schema source with a single row, so nothing guards the full-schema
InsertAllduplicate-key path where the misread reached the row-address reconciler.What it pins, and why the assertions look the way they do
Verified in both directions: on
526988981(the #8509 merge) the test passes; reverting the offset lookup makes it fail withMerge insert update row address (0, 0) is missing from the target fragment; next target row address is (0, 1), which is exactly what #8282 reported.Two preconditions are asserted rather than assumed, because the ambiguity rejection the test expects has several call sites and the v2 fast-path one emits a byte-identical message:
!can_use_create_plan(...)keeps the merge on the v1 slow path. Without it the duplicate is caught by the v2 writer and the reconciler is never involved.matches!(check_compatible_schema(...), SchemaComparison::Subschema)pins the misclassification. A reordered full schema fails the order-sensitive full match and falls through toSubschema, which setsadd_row_addr. That_rowaddris what turns the misread into an internal error rather than silently dropped rows. Making the full match order-insensitive would route this source toRewriteRows, and a message-only assertion would still pass.The negative check (
!error.contains("is missing from the target fragment")) runs before the positive one so a reconciler regression is what gets reported.The message is asserted instead of the
Errorvariant deliberately. This error crosses the sort/repartition inupdate_fragments, so DataFusion returns it shared behind anArcand the conversion inlance-coreyieldsExecutionorInvalidInputdepending on how many partitions still hold a reference. The variant tracks the machine's CPU count; the text does not.Validation
cargo test -p lance --lib test_indexed_full_schema_reordered_duplicate_keys_are_ambiguouscargo test -p lance --lib dataset::write::merge_insert::tests(181 passed)cargo clippy -p lance --tests -- -D warningscargo fmt --all -- --checkScope
Test-only; no production code changes. If maintainers agree the coverage is right, #8282 can be closed as fixed by #8509.
One limit worth stating: the
dataset_opsartifact from #8282 is not public, so this reproduces the reported mechanism rather than replaying that session. The evidence it is the same path is the reported op parameters (rotate: 215, dup_first: true, i.e. a rotated column order with a repeated first key), the identical failure point, and the identical error text before #8509. The report's storage version 2.0, stable row ids disabled, and longer preceding mutation history were not reproduced.