(iceberg-cpp) branch main updated: fix: guard equality delete bounds against missing fields (#857)

[ga...@apache.org]

This is an automated email from the ASF dual-hosted git repository.

wgtmac pushed a commit to branch main
in repository https://gitbox.apache.org/repos/asf/iceberg-cpp.git


The following commit(s) were added to refs/heads/main by this push:
     new 54423808 fix: guard equality delete bounds against missing fields (#857)
54423808 is described below

commit 54423808e51855612a7ac4e47d478710e3b085ce
Author: YangJie <ya...@baidu.com>
AuthorDate: Wed Jul 29 16:50:27 2026 +0800

    fix: guard equality delete bounds against missing fields (#857)
    
    ## What
    
    `internal::CanContainEqDeletesForFile` crashes with
    `std::bad_optional_access` when an equality delete file has lower/upper
    bounds for some of its equality fields but not others.
    `EqualityDeleteFile::LowerBound`/`UpperBound` return
    `Result<std::optional<...>>` (i.e. `std::expected<std::optional<...>,
    Error>`), and a field with no bound yields an engaged `expected`
    wrapping a disengaged `optional`. The guard checked
    `std::expected::has_value()`, which only reports the error state and
    does not detect the empty inner `optional`, so a missing bound slipped
    past and `->value()` threw at the range check.
    
    The throw is an uncaught exception escaping the `Result`-based call
    chain, so it terminates the caller instead of surfacing as an error. It
    is reachable during scan planning (`ManifestGroup` →
    `DeleteFileIndex::ForEntry`) and commit validation
    (`MergingSnapshotUpdate::ValidateNoNewDeletesForDataFiles` →
    `ForDataFile`). Equality delete files with bounds for only some fields
    are produced legitimately by per-column metrics modes
    (`counts`/`none`/`truncate`) and by cross-engine writers, so this is
    reachable in normal reader use.
    
    Fixes #856.
    
    ## How
    
    Unwrap the `expected` with `ICEBERG_ASSIGN_OR_RAISE` before the guard,
    so `has_value()` tests the inner `optional`. This matches the data-side
    guard already used a few lines above in the same function, and matches
    Java's `DeleteFileIndex.canContainEqDeletesForFile`, which guards all
    four bounds uniformly and assumes may-match on any missing bound (it
    never prunes on missing statistics).
    
    As a side effect this propagates a genuine bound-deserialization error
    upward instead of silently treating it as may-match. That is consistent
    with the data-side `Literal::Deserialize` calls in the same function,
    which already propagate via `ICEBERG_ASSIGN_OR_RAISE`.
    
    ## Testing
    
    Added `DeleteFileIndexTest.TestEqualityDeletePartialFieldBounds`: an
    equality delete over fields `{1, 2}` with bounds for field 1 only,
    evaluated against a data file that has bounds for both (field 1's range
    overlapping so evaluation reaches field 2). Verified fail-without
    (throws `std::bad_optional_access`) / pass-with (returns the delete file
    as may-match). `null_value_counts` is pinned to 0 for both fields so the
    null short-circuits cannot mask the range-check path if the schema
    fields later change nullability. Full `manifest_test` passes.
---
 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 9a8be06b..5f3f5174 100644
--- a/src/iceberg/delete_file_index.cc
+++ b/src/iceberg/delete_file_index.cc
@@ -145,8 +145,8 @@ Result<bool> 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<bool> 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 0c8c8821..fea9b6a0 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>(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>(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) {