Skip to content

Commit c77506c

Browse files
committed
fix: guard equality delete bounds against missing fields (#856)
CanContainEqDeletesForFile checked std::expected::has_value() on the result of LowerBound/UpperBound, which only reports the error state and not the inner std::optional. When an equality delete file has bounds for some equality fields but not others, the missing-bound field yields an engaged expected wrapping a disengaged optional, passes the guard, and throws std::bad_optional_access at the range check. The exception escapes the Result-based call chain (scan planning via ForEntry, commit validation via ForDataFile), crashing the process. Unwrap the expected with ICEBERG_ASSIGN_OR_RAISE so has_value() tests the inner optional, matching the data-side guard in the same function and Java's uniform null check. This also propagates a genuine bound deserialization error instead of silently treating it as may-match, consistent with the data-side handling. Add a regression test covering an equality delete with bounds for only some fields; null_value_counts are pinned to 0 so the null short-circuits cannot mask the range-check path if the schema fields later change nullability.
1 parent f9e83ff commit c77506c

2 files changed

Lines changed: 69 additions & 4 deletions

File tree

‎src/iceberg/delete_file_index.cc‎

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -145,8 +145,8 @@ Result<bool> CanContainEqDeletesForFile(const DataFile& data_file,
145145
continue; // Missing bounds, assume may match
146146
}
147147

148-
auto delete_lower = delete_file.LowerBound(field_id);
149-
auto delete_upper = delete_file.UpperBound(field_id);
148+
ICEBERG_ASSIGN_OR_RAISE(auto delete_lower, delete_file.LowerBound(field_id));
149+
ICEBERG_ASSIGN_OR_RAISE(auto delete_upper, delete_file.UpperBound(field_id));
150150
if (!delete_lower.has_value() || !delete_upper.has_value()) {
151151
continue; // Missing bounds, assume may match
152152
}
@@ -158,8 +158,8 @@ Result<bool> CanContainEqDeletesForFile(const DataFile& data_file,
158158
ICEBERG_ASSIGN_OR_RAISE(auto data_upper,
159159
Literal::Deserialize(data_upper_it->second, primitive_type));
160160

161-
if (!RangesOverlap(data_lower, data_upper, delete_lower->value().get(),
162-
delete_upper->value().get())) {
161+
if (!RangesOverlap(data_lower, data_upper, delete_lower->get(),
162+
delete_upper->get())) {
163163
return false; // Ranges don't overlap - cannot match
164164
}
165165
}

‎src/iceberg/test/delete_file_index_test.cc‎

Lines changed: 65 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -840,6 +840,71 @@ TEST_P(DeleteFileIndexTest, TestEqualityDeletesGroup) {
840840
}
841841
}
842842

843+
// Regression test: an equality delete file that carries bounds for some equality
844+
// fields but not others must not crash. CanContainEqDeletesForFile guarded the
845+
// missing-bound case with std::expected::has_value() (which only reports the error
846+
// state), so a field whose bound was absent produced an engaged expected wrapping a
847+
// disengaged optional, and dereferencing it threw std::bad_optional_access. Such
848+
// files are produced legitimately by per-column metrics modes (counts/none/truncate)
849+
// and by cross-engine writers.
850+
TEST_P(DeleteFileIndexTest, TestEqualityDeletePartialFieldBounds) {
851+
auto partition_a = PartitionValues({Literal::Int(0)});
852+
853+
auto serialize = [](const Literal& literal) {
854+
auto result = literal.Serialize();
855+
EXPECT_THAT(result, IsOk());
856+
return result.value();
857+
};
858+
859+
// Equality delete over fields {1 (int id), 2 (string data)} but with bounds only
860+
// for field 1. Field 1's bounds must deserialize cleanly, otherwise
861+
// ConvertBoundsIfNeeded fails first and masks the missing-bound path.
862+
//
863+
// null_value_counts is set to 0 for both fields so the null short-circuits in
864+
// CanContainEqDeletesForFile (ContainsNull/AllNull) never fire, which keeps the
865+
// repro alive even if the SetUp schema fields are later changed to optional: with a
866+
// zero count ContainsNull returns false regardless of nullability, so evaluation
867+
// always reaches the range check that dereferences the missing field-2 bound.
868+
auto eq_delete = std::make_shared<DataFile>(DataFile{
869+
.content = DataFile::Content::kEqualityDeletes,
870+
.file_path = "/path/to/eq-delete-partial-bounds.parquet",
871+
.file_format = FileFormatType::kParquet,
872+
.partition = partition_a,
873+
.record_count = 10,
874+
.file_size_in_bytes = 100,
875+
.null_value_counts = {{1, 0}, {2, 0}},
876+
.lower_bounds = {{1, serialize(Literal::Int(0))}},
877+
.upper_bounds = {{1, serialize(Literal::Int(100))}},
878+
.equality_ids = {1, 2},
879+
.partition_spec_id = partitioned_spec_->spec_id(),
880+
});
881+
882+
// Data file with full bounds for both equality fields. Field 1's range overlaps the
883+
// delete's, so evaluation proceeds to field 2, which lacks a delete-side bound.
884+
auto data_file = std::make_shared<DataFile>(DataFile{
885+
.file_path = "/path/to/data-partial-bounds.parquet",
886+
.file_format = FileFormatType::kParquet,
887+
.partition = partition_a,
888+
.record_count = 100,
889+
.file_size_in_bytes = 1000,
890+
.null_value_counts = {{1, 0}, {2, 0}},
891+
.lower_bounds = {{1, serialize(Literal::Int(0))},
892+
{2, serialize(Literal::String("a"))}},
893+
.upper_bounds = {{1, serialize(Literal::Int(100))},
894+
{2, serialize(Literal::String("z"))}},
895+
.partition_spec_id = partitioned_spec_->spec_id(),
896+
});
897+
898+
internal::EqualityDeletes group(*schema_);
899+
auto entry = MakeDeleteEntry(/*snapshot_id=*/1000L, /*sequence_number=*/1, eq_delete);
900+
EXPECT_THAT(group.Add(std::move(entry)), IsOk());
901+
902+
// Must not throw. With a missing field-2 bound the delete may still match, so it is
903+
// returned rather than pruned.
904+
ICEBERG_UNWRAP_OR_FAIL(auto filtered, group.Filter(/*seq=*/0, *data_file));
905+
EXPECT_EQ(filtered.size(), 1);
906+
}
907+
843908
TEST_P(DeleteFileIndexTest, TestMixDeleteFilesAndDVs) {
844909
auto version = GetParam();
845910
if (version < 3) {

0 commit comments

Comments
 (0)