fix(blob): allow blob files with different schema ids in a bunch#402
fix(blob): allow blob files with different schema ids in a bunch#402duanyyyyyyy wants to merge 1 commit into
Conversation
| ASSERT_OK(blob_bunch->Add(blob_tail)); | ||
| ASSERT_EQ(blob_bunch->Files().size(), 2); | ||
| } | ||
|
|
There was a problem hiding this comment.
Could you add an integration test in blob_table_inte_test.cpp that uses WriteNextSchema to simulate an ALTER TABLE scenario?
| // bunch. The Java guard `if (!isBlobFile(file))` around this check was dropped | ||
| // when porting, which turned that legitimate case into a spurious error. | ||
| if (!BlobUtils::IsBlobFile(file->file_name)) { | ||
| if (file->schema_id != files_[0]->schema_id) { |
There was a problem hiding this comment.
BlobBunch::Add already rejects non-blob files at the beginning, so the new if (!BlobUtils::IsBlobFile(file->file_name)) guard around the schema-id check is unreachable. Functionally this is fine, but it may confuse readers. Could we remove the schema-id check entirely for BlobBunch and replace the comment with something like: “BlobBunch only contains blob files. Unlike vector-store files, blob files may span schema ids as long as they belong to the same blob field.”
duanyyyyyyy
force-pushed
the
fix/blob-bunch-schema-id
branch
from
9666fb4 to
58a6ee5
Compare
|
|
duanyyyyyyy
force-pushed
the
fix/blob-bunch-schema-id
branch
2 times, most recently
from
1a5cdde to
41044f1
Compare
| ])") | ||
| .ValueOrDie()); | ||
| ASSERT_OK(ScanAndRead(table_path, schema->field_names(), expected_array)); | ||
| } |
There was a problem hiding this comment.
I tested this integration test locally against the unmodified/pre-fix code, and it still passes. So I don’t think this test actually hits the regression scenario.
The test creates two f1 blob files with adjacent row ranges: [0, 0] under schema 0 and [1, 1] under schema 1. However, RangeHelper::MergeOverlappingRanges only merges overlapping ranges, not adjacent ranges. As a result, these two files are split into different need_merge_files groups, and each group creates its own BlobBunch with only one blob file. The old schema-id check is therefore never exercised.
To cover the real scenario, the integration test needs to ensure that two blob files with different schema ids are added to the same BlobBunch, for example all with first row id 0.
duanyyyyyyy
force-pushed
the
fix/blob-bunch-schema-id
branch
from
41044f1 to
7ebab5a
Compare
BlobBunch::Add required every file in a bunch to share the same schema id. After schema evolution that adds/drops OTHER columns, a single blob field's contiguous files can carry different schema ids (an old appended file plus a compacted one). MergeRangesAndSort groups them into one bunch by row id, so the check raised a spurious "All files in a blob bunch should have the same schema id." A blob column's on-disk layout is schema-independent and the bunch is read with the first file's schema regardless, so blob files may span schema ids. BlobBunch only ever holds blob files (non-blob files are rejected up front), so the check is removed. This matches Paimon Java, whose SpecialFieldBunch.add guards the same check with if (!isBlobFile(file)). Adds a unit test (TestAddBlobFilesWithDifferentSchemaId) and an integration test (TestBlobBunchSpanningSchemaIds, which uses the existing WriteNextSchema helper to simulate an ALTER TABLE between two appends of the same blob field).
duanyyyyyyy
force-pushed
the
fix/blob-bunch-schema-id
branch
from
a40c2b9 to
f47b53f
Compare
3 participants
Purpose
DataEvolutionSplitRead::BlobBunch::Addrejected a bunch whose blob filescarry different schema ids ("All files in a blob bunch should have the same
schema id.").
Linked issue: close #xxx
After schema evolution that adds/drops OTHER columns, a single blob field's
contiguous files can legitimately have different schema ids — e.g. an old
appended file (schema N) followed by a compacted file (schema M) covering the
next row range.
MergeRangesAndSortgroups them into one bunch purely by rowid, so the check fires and a normal read fails.
Paimon Java
SpecialFieldBunch.addguards this exact check withif (!isBlobFile(file))— blob files are exempt because a blob column's on-disklayout is schema-independent, and the bunch is read with the first file's schema
regardless. That guard was dropped when porting to C++.
What this PR does
if (!IsBlobFile(file))guard so blob files may span schema ids;the
write_cols(field identity) check is unchanged.TestAddBlobFilesWithDifferentSchemaId.Tests
API and Format
Documentation
Generative AI tooling