fix: Iceberg positional delete compare base64 encoded value with raw value

[PingLiuPing]

When reading Iceberg positional delete files, the IcebergSplitReader optimizes away irrelevant delete files by comparing the delete file's upper_bounds statistic against the current split's starting row offset.
This PR fixes a bugs in that logic. Iceberg stores column statistics (lower_bounds / upper_bounds) in manifests as Base64-encoded, little-endian binary values (for long type). For a long column (such as the positional delete pos column), the value is an 8-byte little-endian integer, Base64-encoded.
Previously it passed the raw Base64 string directly to folly::to<uint64_t>(), which attempts to parse it as a decimal string. For example, the value CgAAAAAAAAA= should be decoded to integer 10, but folly::to<uint64_t>("CgAAAAAAAAA=") throws exception.
This PR fix this by decoding the upper bound first and then reinterpret the bytes as uint64_t and then apply folly::Endian::little() to convert from the iceberg spec specified little-endian encoding to host byte order before comparing against splitOffset_.

This fixes the issue prestodb/presto#27237

[netlify]

✅ Deploy Preview for meta-velox canceled.

Name	Link
🔨 Latest commit	d9ae324
🔍 Latest deploy log	https://app.netlify.com/projects/meta-velox/deploys/69a5ca314c4ee100085218c2

[mbasmanova]

@PingLiuPing Would you confirm that the test fails without the fix?

[PingLiuPing]

Yes,

unknown file: Failure
C++ exception with description "Exception: VeloxRuntimeError
Error Source: RUNTIME
Error Code: INVALID_STATE
Reason: Operator::getOutput failed for [operator: TableScan, plan node ID: 0]: Non-digit character found: "AgAAAAAAAAA="
Retriable: False
Function: operator()

"AgAAAAAAAAA=" is base64 encoded value.

[mbasmanova]

Can we use std::string_view?

[PingLiuPing]

Thanks.

[mbasmanova]

Where do these numbers come from?

[PingLiuPing]

They are defined in https://iceberg.apache.org/spec/#position-delete-files, added a comment to explain the value.

[mbasmanova]

/claude-review

[github-actions]

✅ Claude Code Review

Requested by @mbasmanova

---

Summary

I've completed a thorough analysis of this PR fixing the Iceberg positional delete Base64 decoding issue.

Issues Found

🟡 Suggestion: Error Handling Enhancement

File: IcebergSplitReader.cpp:91-95
Issue: The Base64 decode operation in encoding::Base64::decode(iter->second) could potentially throw an exception, but there's no explicit error handling around it. While the VELOX_CHECK_EQ will catch size mismatches, malformed Base64 strings could still cause issues.
Suggested Fix: Consider wrapping the decode operation in a try-catch or using a Base64 decode variant that returns a status/optional to handle malformed input gracefully.

🟡 Suggestion: Const Correctness

File: IcebergSplitReader.cpp:95-96
Issue: The variable posDeleteUpperBound could be const since it's only written to once via std::memcpy and then only read.
Suggested Fix:

const uint64_t posDeleteUpperBound = folly::Endian::little(
    *reinterpret_cast<const uint64_t*>(decodedBound.data()));

🟡 Suggestion: Alignment Consideration

File: IcebergSplitReader.cpp:96-98
Issue: Using std::memcpy is the safe approach for unaligned access, but the comment doesn't explain why this is necessary (avoiding undefined behavior from unaligned access).
Suggested Fix: Add a comment explaining this protects against potential alignment issues:

// Use memcpy to avoid potential alignment issues with reinterpret_cast
std::memcpy(&posDeleteUpperBound, decodedBound.data(), sizeof(uint64_t));

🟢 Nitpick: Include Organization

File: IcebergSplitReader.cpp:19-21
Issue: The new includes (folly/lang/Bits.h and velox/common/encode/Base64.h) are added at the top but could be better organized with other folly includes grouped together.

Positive Observations

✅ Correctness: The fix properly addresses the core issue by decoding Base64 before parsing as uint64_t, which is exactly what the Iceberg specification requires.

✅ Endianness Handling: Proper use of folly::Endian::little() to handle the little-endian format specified by Iceberg.

✅ Memory Safety: Appropriate use of std::memcpy to avoid alignment issues and undefined behavior.

✅ Size Validation: The VELOX_CHECK_EQ ensures the decoded size matches expectations before proceeding.

✅ Test Coverage: The test properly demonstrates the fix by encoding a test value using the same process (little-endian + Base64).

✅ Code Organization: Constants moved to header file with clear naming (kPosId, kFilePathId) improving maintainability.

✅ Boundary Case Handling: The logic correctly handles the case where upper bound equals split offset (should continue processing).

The fix is technically sound and addresses a real bug where Base64-encoded values were being incorrectly parsed as decimal strings, which would throw exceptions. The implementation follows Velox patterns and properly handles the Iceberg specification for storing statistics as Base64-encoded little-endian binary values.

---

ℹ️ About this response

This review was generated by Claude Code. It analyzed the PR diff and codebase to provide feedback.

Limitations:

• Claude may miss context from files not in the diff
• Large PRs may be truncated
• Always apply human judgment to AI suggestions

Available commands:

• /claude-review - Request a code review
• /claude-query <question> - Ask a question about the PR or codebase

[mbasmanova]

Thanks.

[meta-codesync]

@kKPulla has imported this pull request. If you are a Meta employee, you can view this in D94940646.

[czentgr]

Thanks for the quick fix.

[meta-codesync]

@kKPulla merged this pull request in fb5b296.