fix: guard equality delete bounds against missing fields - #857
fix: guard equality delete bounds against missing fields#857LuciferYang wants to merge 1 commit into
Conversation
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.
There was a problem hiding this comment.
Pull request overview
Fixes a crash in equality-delete pruning where per-field bounds may be missing: CanContainEqDeletesForFile previously guarded the outer Result/expected but not the inner std::optional, allowing a missing bound to trigger std::bad_optional_access and terminate callers during planning/validation.
Changes:
- Unwrap
EqualityDeleteFile::{LowerBound,UpperBound}viaICEBERG_ASSIGN_OR_RAISEso the guard correctly checks the returnedstd::optionaland propagates real deserialization errors. - Update the range-overlap call sites to use the unwrapped optional-bound values safely.
- Add a regression test covering equality deletes with bounds for only a subset of equality fields.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| src/iceberg/delete_file_index.cc | Fixes the optional-bound guard by unwrapping Result<optional<…>> before checking presence; prevents std::bad_optional_access and correctly propagates deserialization errors. |
| src/iceberg/test/delete_file_index_test.cc | Adds a regression test that reproduces partial-field bounds and asserts the delete is treated as “may match” without throwing. |
|
@wgtmac when you have a moment, could you take a look? This fixes a |
What
internal::CanContainEqDeletesForFilecrashes withstd::bad_optional_accesswhen an equality delete file has lower/upper bounds for some of its equality fields but not others.EqualityDeleteFile::LowerBound/UpperBoundreturnResult<std::optional<...>>(i.e.std::expected<std::optional<...>, Error>), and a field with no bound yields an engagedexpectedwrapping a disengagedoptional. The guard checkedstd::expected::has_value(), which only reports the error state and does not detect the empty inneroptional, 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
expectedwithICEBERG_ASSIGN_OR_RAISEbefore the guard, sohas_value()tests the inneroptional. This matches the data-side guard already used a few lines above in the same function, and matches Java'sDeleteFileIndex.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::Deserializecalls in the same function, which already propagate viaICEBERG_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 (throwsstd::bad_optional_access) / pass-with (returns the delete file as may-match).null_value_countsis pinned to 0 for both fields so the null short-circuits cannot mask the range-check path if the schema fields later change nullability. Fullmanifest_testpasses.