From c77506cefec3167dd647c21d0aff37f2e97b5c70 Mon Sep 17 00:00:00 2001 From: yangjie01 Date: Mon, 27 Jul 2026 12:37:19 +0800 Subject: [PATCH] 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. --- src/iceberg/delete_file_index.cc | 8 +-- src/iceberg/test/delete_file_index_test.cc | 65 ++++++++++++++++++++++ 2 files changed, 69 insertions(+), 4 deletions(-) diff --git a/src/iceberg/delete_file_index.cc b/src/iceberg/delete_file_index.cc index 9a8be06b8..5f3f51742 100644 --- a/src/iceberg/delete_file_index.cc +++ b/src/iceberg/delete_file_index.cc @@ -145,8 +145,8 @@ Result CanContainEqDeletesForFile(const DataFile& data_file, continue; // Missing bounds, assume may match } - auto delete_lower = delete_file.LowerBound(field_id); - auto delete_upper = delete_file.UpperBound(field_id); + ICEBERG_ASSIGN_OR_RAISE(auto delete_lower, delete_file.LowerBound(field_id)); + ICEBERG_ASSIGN_OR_RAISE(auto delete_upper, delete_file.UpperBound(field_id)); if (!delete_lower.has_value() || !delete_upper.has_value()) { continue; // Missing bounds, assume may match } @@ -158,8 +158,8 @@ Result CanContainEqDeletesForFile(const DataFile& data_file, ICEBERG_ASSIGN_OR_RAISE(auto data_upper, Literal::Deserialize(data_upper_it->second, primitive_type)); - if (!RangesOverlap(data_lower, data_upper, delete_lower->value().get(), - delete_upper->value().get())) { + if (!RangesOverlap(data_lower, data_upper, delete_lower->get(), + delete_upper->get())) { return false; // Ranges don't overlap - cannot match } } diff --git a/src/iceberg/test/delete_file_index_test.cc b/src/iceberg/test/delete_file_index_test.cc index 0c8c8821b..fea9b6a04 100644 --- a/src/iceberg/test/delete_file_index_test.cc +++ b/src/iceberg/test/delete_file_index_test.cc @@ -840,6 +840,71 @@ TEST_P(DeleteFileIndexTest, TestEqualityDeletesGroup) { } } +// Regression test: an equality delete file that carries bounds for some equality +// fields but not others must not crash. CanContainEqDeletesForFile guarded the +// missing-bound case with std::expected::has_value() (which only reports the error +// state), so a field whose bound was absent produced an engaged expected wrapping a +// disengaged optional, and dereferencing it threw std::bad_optional_access. Such +// files are produced legitimately by per-column metrics modes (counts/none/truncate) +// and by cross-engine writers. +TEST_P(DeleteFileIndexTest, TestEqualityDeletePartialFieldBounds) { + auto partition_a = PartitionValues({Literal::Int(0)}); + + auto serialize = [](const Literal& literal) { + auto result = literal.Serialize(); + EXPECT_THAT(result, IsOk()); + return result.value(); + }; + + // Equality delete over fields {1 (int id), 2 (string data)} but with bounds only + // for field 1. Field 1's bounds must deserialize cleanly, otherwise + // ConvertBoundsIfNeeded fails first and masks the missing-bound path. + // + // null_value_counts is set to 0 for both fields so the null short-circuits in + // CanContainEqDeletesForFile (ContainsNull/AllNull) never fire, which keeps the + // repro alive even if the SetUp schema fields are later changed to optional: with a + // zero count ContainsNull returns false regardless of nullability, so evaluation + // always reaches the range check that dereferences the missing field-2 bound. + auto eq_delete = std::make_shared(DataFile{ + .content = DataFile::Content::kEqualityDeletes, + .file_path = "/path/to/eq-delete-partial-bounds.parquet", + .file_format = FileFormatType::kParquet, + .partition = partition_a, + .record_count = 10, + .file_size_in_bytes = 100, + .null_value_counts = {{1, 0}, {2, 0}}, + .lower_bounds = {{1, serialize(Literal::Int(0))}}, + .upper_bounds = {{1, serialize(Literal::Int(100))}}, + .equality_ids = {1, 2}, + .partition_spec_id = partitioned_spec_->spec_id(), + }); + + // Data file with full bounds for both equality fields. Field 1's range overlaps the + // delete's, so evaluation proceeds to field 2, which lacks a delete-side bound. + auto data_file = std::make_shared(DataFile{ + .file_path = "/path/to/data-partial-bounds.parquet", + .file_format = FileFormatType::kParquet, + .partition = partition_a, + .record_count = 100, + .file_size_in_bytes = 1000, + .null_value_counts = {{1, 0}, {2, 0}}, + .lower_bounds = {{1, serialize(Literal::Int(0))}, + {2, serialize(Literal::String("a"))}}, + .upper_bounds = {{1, serialize(Literal::Int(100))}, + {2, serialize(Literal::String("z"))}}, + .partition_spec_id = partitioned_spec_->spec_id(), + }); + + internal::EqualityDeletes group(*schema_); + auto entry = MakeDeleteEntry(/*snapshot_id=*/1000L, /*sequence_number=*/1, eq_delete); + EXPECT_THAT(group.Add(std::move(entry)), IsOk()); + + // Must not throw. With a missing field-2 bound the delete may still match, so it is + // returned rather than pruned. + ICEBERG_UNWRAP_OR_FAIL(auto filtered, group.Filter(/*seq=*/0, *data_file)); + EXPECT_EQ(filtered.size(), 1); +} + TEST_P(DeleteFileIndexTest, TestMixDeleteFilesAndDVs) { auto version = GetParam(); if (version < 3) {