Skip to content

Commit 680b90b

Browse files
committed
Align deletion vector writer with Java behavior
- Reuse `WriteResult` for delete writers and report referenced/rewritten delete files. - Add shared DV read helper and refactor DV loading/writing paths. - Preserve source delete files in `PositionDeleteIndex` and tighten DV serialization. - Generate Iceberg-CPP `created-by` metadata consistently in CMake and Meson. - Fold DV writer tests into existing test targets.
1 parent e03d8d0 commit 680b90b

29 files changed

Lines changed: 615 additions & 503 deletions

CMakeLists.txt

Lines changed: 3 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -28,10 +28,9 @@ project(Iceberg
2828
DESCRIPTION "Iceberg C++ Project"
2929
LANGUAGES CXX)
3030

31-
set(ICEBERG_VERSION_SUFFIX "-SNAPSHOT")
32-
set(ICEBERG_VERSION_STRING "${PROJECT_VERSION}${ICEBERG_VERSION_SUFFIX}")
33-
configure_file("${CMAKE_CURRENT_SOURCE_DIR}/src/iceberg/version.h.in"
34-
"${CMAKE_BINARY_DIR}/src/iceberg/version.h")
31+
include(IcebergBuildUtils)
32+
iceberg_configure_version_header("${CMAKE_CURRENT_SOURCE_DIR}/src/iceberg/version.h.in"
33+
"${CMAKE_BINARY_DIR}/src/iceberg/version.h")
3534

3635
set(CMAKE_CXX_STANDARD 23)
3736
set(CMAKE_CXX_STANDARD_REQUIRED ON)
@@ -79,7 +78,6 @@ if(ICEBERG_BUILD_REST_INTEGRATION_TESTS AND WIN32)
7978
endif()
8079

8180
include(CMakeParseArguments)
82-
include(IcebergBuildUtils)
8381
include(IcebergSanitizer)
8482
include(IcebergSccache)
8583
include(IcebergThirdpartyToolchain)

cmake_modules/IcebergBuildUtils.cmake

Lines changed: 22 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -20,6 +20,28 @@
2020

2121
include(CMakePackageConfigHelpers)
2222

23+
function(iceberg_configure_version_header INPUT_FILE OUTPUT_FILE)
24+
set(ICEBERG_VERSION_SUFFIX "-SNAPSHOT")
25+
set(ICEBERG_VERSION_STRING "${PROJECT_VERSION}${ICEBERG_VERSION_SUFFIX}")
26+
set(ICEBERG_GIT_COMMIT_ID "unknown")
27+
28+
find_package(Git QUIET)
29+
if(GIT_FOUND)
30+
execute_process(COMMAND "${GIT_EXECUTABLE}" rev-parse HEAD
31+
WORKING_DIRECTORY "${PROJECT_SOURCE_DIR}"
32+
OUTPUT_VARIABLE ICEBERG_GIT_COMMIT_ID
33+
RESULT_VARIABLE ICEBERG_GIT_RESULT
34+
OUTPUT_STRIP_TRAILING_WHITESPACE ERROR_QUIET)
35+
if(NOT ICEBERG_GIT_RESULT EQUAL 0 OR NOT ICEBERG_GIT_COMMIT_ID)
36+
set(ICEBERG_GIT_COMMIT_ID "unknown")
37+
endif()
38+
endif()
39+
40+
set(ICEBERG_FULL_VERSION_STRING
41+
"Apache Iceberg-CPP ${ICEBERG_VERSION_STRING} (commit ${ICEBERG_GIT_COMMIT_ID})")
42+
configure_file("${INPUT_FILE}" "${OUTPUT_FILE}" @ONLY)
43+
endfunction()
44+
2345
function(iceberg_install_cmake_package PACKAGE_NAME EXPORT_NAME)
2446
set(CONFIG_CMAKE "${PACKAGE_NAME}-config.cmake")
2547
set(BUILT_CONFIG_CMAKE "${CMAKE_CURRENT_BINARY_DIR}/${CONFIG_CMAKE}")

src/iceberg/CMakeLists.txt

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -197,6 +197,7 @@ set(ICEBERG_DATA_SOURCES
197197
data/delete_filter.cc
198198
data/delete_loader.cc
199199
data/deletion_vector_writer.cc
200+
data/dv_util.cc
200201
data/equality_delete_writer.cc
201202
data/file_scan_task_reader.cc
202203
data/position_delete_writer.cc

src/iceberg/data/data_writer.cc

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -58,7 +58,7 @@ class DataWriter::Impl {
5858
return {};
5959
}
6060

61-
Result<FileWriter::WriteResult> Metadata() {
61+
Result<WriteResult> Metadata() {
6262
ICEBERG_CHECK(closed_, "Cannot get metadata before closing the writer");
6363

6464
ICEBERG_ASSIGN_OR_RAISE(auto metrics, writer_->metrics());
@@ -98,7 +98,7 @@ class DataWriter::Impl {
9898
options_.spec ? std::make_optional(options_.spec->spec_id()) : std::nullopt,
9999
});
100100

101-
FileWriter::WriteResult result;
101+
WriteResult result;
102102
result.data_files.push_back(std::move(data_file));
103103
return result;
104104
}
@@ -127,6 +127,6 @@ Result<int64_t> DataWriter::Length() const { return impl_->Length(); }
127127

128128
Status DataWriter::Close() { return impl_->Close(); }
129129

130-
Result<FileWriter::WriteResult> DataWriter::Metadata() { return impl_->Metadata(); }
130+
Result<WriteResult> DataWriter::Metadata() { return impl_->Metadata(); }
131131

132132
} // namespace iceberg

src/iceberg/data/delete_loader.cc

Lines changed: 34 additions & 84 deletions
Original file line numberDiff line numberDiff line change
@@ -31,6 +31,7 @@
3131

3232
#include "iceberg/arrow/nanoarrow_status_internal.h"
3333
#include "iceberg/arrow_c_data_guard_internal.h"
34+
#include "iceberg/data/dv_util_internal.h"
3435
#include "iceberg/deletes/position_delete_index.h"
3536
#include "iceberg/deletes/position_delete_range_consumer.h"
3637
#include "iceberg/file_io.h"
@@ -88,17 +89,31 @@ bool StringEquals(const ArrowArrayView* view, int64_t row_idx, std::string_view
8889
return sv.data != nullptr && std::memcmp(sv.data, target.data(), target.size()) == 0;
8990
}
9091

92+
Status ValidateDV(const DataFile& dv, std::string_view data_file_path) {
93+
ICEBERG_PRECHECK(dv.content_offset.has_value(), "Invalid DV, offset cannot be null: {}",
94+
ContentFileUtil::DVDesc(dv));
95+
ICEBERG_PRECHECK(dv.content_size_in_bytes.has_value(), "Invalid DV, length is null: {}",
96+
ContentFileUtil::DVDesc(dv));
97+
ICEBERG_PRECHECK(
98+
dv.content_size_in_bytes.value() <= std::numeric_limits<int32_t>::max(),
99+
"Can't read DV larger than 2GB: {}", dv.content_size_in_bytes.value());
100+
ICEBERG_PRECHECK(dv.referenced_data_file.has_value() &&
101+
dv.referenced_data_file.value() == data_file_path,
102+
"DV is expected to reference {}, not {}", data_file_path,
103+
dv.referenced_data_file.value_or(""));
104+
return {};
105+
}
106+
91107
} // namespace
92108

93109
DeleteLoader::DeleteLoader(std::shared_ptr<FileIO> io) : io_(std::move(io)) {}
94110

95111
DeleteLoader::~DeleteLoader() = default;
96112

97-
Status DeleteLoader::LoadPositionDelete(const std::shared_ptr<DataFile>& file,
98-
PositionDeleteIndex& index,
113+
Status DeleteLoader::LoadPositionDelete(const DataFile& file, PositionDeleteIndex& index,
99114
std::string_view data_file_path) const {
100-
// TODO(gangwu): push down path filter to open the file.
101-
ICEBERG_ASSIGN_OR_RAISE(auto reader, OpenDeleteFile(*file, PosDeleteSchema(), io_));
115+
// TODO(gangwu): Add cache hooks, worker pool, and filter pushdown.
116+
ICEBERG_ASSIGN_OR_RAISE(auto reader, OpenDeleteFile(file, PosDeleteSchema(), io_));
102117

103118
ICEBERG_ASSIGN_OR_RAISE(auto arrow_schema, reader->Schema());
104119
internal::ArrowSchemaGuard schema_guard(&arrow_schema);
@@ -116,19 +131,15 @@ Status DeleteLoader::LoadPositionDelete(const std::shared_ptr<DataFile>& file,
116131
// `ForEachPositionDelete`. Trusts the hint -- spec-compliant writers
117132
// only set it when all rows share one data file.
118133
const bool use_referenced_data_file_fast_path =
119-
file->referenced_data_file.has_value() &&
120-
file->referenced_data_file.value() == data_file_path;
134+
file.referenced_data_file.has_value() &&
135+
file.referenced_data_file.value() == data_file_path;
121136

122137
// Filter-path staging buffer; reused across batches via `clear()`.
123138
std::vector<int64_t> positions;
124139
// Scratch buffer for `ForEachPositionDelete`'s bulk dispatch path;
125140
// reused across batches and across both routing branches.
126141
std::vector<uint32_t> bulk_scratch;
127142

128-
// Whether any position for the target data file came from this file, so the
129-
// source delete file is recorded once.
130-
bool contributed = false;
131-
132143
while (true) {
133144
ICEBERG_ASSIGN_OR_RAISE(auto batch_opt, reader->Next());
134145
if (!batch_opt.has_value()) break;
@@ -160,9 +171,6 @@ Status DeleteLoader::LoadPositionDelete(const std::shared_ptr<DataFile>& file,
160171
const int64_t* pos_data = Int64ValuesBuffer(pos_view);
161172

162173
if (use_referenced_data_file_fast_path) {
163-
// Reaching here means length > 0 (empty batches are skipped above), so
164-
// this file contributes positions for the target data file.
165-
contributed = true;
166174
ForEachPositionDelete(std::span<const int64_t>(pos_data, length), index,
167175
bulk_scratch);
168176
continue;
@@ -177,67 +185,10 @@ Status DeleteLoader::LoadPositionDelete(const std::shared_ptr<DataFile>& file,
177185
positions.push_back(pos_data[i]);
178186
}
179187
}
180-
if (!positions.empty()) {
181-
contributed = true;
182-
}
183188
ForEachPositionDelete(positions, index, bulk_scratch);
184189
}
185190

186-
ICEBERG_RETURN_UNEXPECTED(reader->Close());
187-
188-
// Record the source delete file so callers (e.g. DeletionVectorWriter) can
189-
// report file-scoped position deletes as rewritten.
190-
if (contributed) {
191-
index.AddDeleteFile(file);
192-
}
193-
return {};
194-
}
195-
196-
Status DeleteLoader::LoadDV(const std::shared_ptr<DataFile>& file,
197-
PositionDeleteIndex& index,
198-
std::string_view data_file_path) const {
199-
// A deletion vector must reference exactly one data file; without it the
200-
// caller cannot know which data file the positions apply to.
201-
ICEBERG_PRECHECK(file->referenced_data_file.has_value(),
202-
"Deletion vector requires referenced_data_file: {}", file->file_path);
203-
204-
// The DV must reference the data file being read. A mismatch means the caller
205-
// grouped delete files incorrectly; failing here avoids silently applying the
206-
// wrong DV or hiding bad metadata behind an empty index.
207-
ICEBERG_PRECHECK(file->referenced_data_file.value() == data_file_path,
208-
"Deletion vector references {}, not the requested data file {}",
209-
file->referenced_data_file.value(), data_file_path);
210-
211-
// For deletion vectors, content_offset and content_size_in_bytes point directly
212-
// at the DV blob bytes within the Puffin file and are required by the spec.
213-
ICEBERG_PRECHECK(
214-
file->content_offset.has_value() && file->content_size_in_bytes.has_value(),
215-
"Deletion vector requires content_offset and content_size_in_bytes: {}",
216-
file->file_path);
217-
218-
const int64_t offset = file->content_offset.value();
219-
const int64_t length = file->content_size_in_bytes.value();
220-
ICEBERG_PRECHECK(offset >= 0 && length >= 0,
221-
"Invalid deletion vector offset/length: offset={}, length={}", offset,
222-
length);
223-
ICEBERG_PRECHECK(length <= std::numeric_limits<int32_t>::max(),
224-
"Cannot read deletion vector larger than 2GB: {}", length);
225-
226-
ICEBERG_ASSIGN_OR_RAISE(auto input_file, io_->NewInputFile(file->file_path));
227-
ICEBERG_ASSIGN_OR_RAISE(auto stream, input_file->Open());
228-
229-
std::vector<std::byte> bytes(static_cast<size_t>(length));
230-
ICEBERG_RETURN_UNEXPECTED(stream->ReadFully(offset, bytes));
231-
ICEBERG_RETURN_UNEXPECTED(stream->Close());
232-
233-
std::span<const uint8_t> blob(reinterpret_cast<const uint8_t*>(bytes.data()),
234-
bytes.size());
235-
// Deserialize validates the blob length and cardinality against `file` and
236-
// retains it as the source delete file.
237-
ICEBERG_ASSIGN_OR_RAISE(auto dv, PositionDeleteIndex::Deserialize(blob, file));
238-
239-
index.Merge(dv);
240-
return {};
191+
return reader->Close();
241192
}
242193

243194
Result<PositionDeleteIndex> DeleteLoader::LoadPositionDeletes(
@@ -247,19 +198,14 @@ Result<PositionDeleteIndex> DeleteLoader::LoadPositionDeletes(
247198
ICEBERG_PRECHECK(file != nullptr, "Delete file must not be null");
248199
}
249200

250-
PositionDeleteIndex index;
251-
252-
// A single deletion vector replaces all other deletes for a data file, so it
253-
// is read through the dedicated DV path and validated against the requested
254-
// data file.
255201
if (ContentFileUtil::ContainsSingleDV(delete_files)) {
256-
ICEBERG_RETURN_UNEXPECTED(LoadDV(delete_files.front(), index, data_file_path));
257-
return index;
202+
const auto& dv = delete_files.front();
203+
ICEBERG_RETURN_UNEXPECTED(ValidateDV(*dv, data_file_path));
204+
return DVUtil::ReadDV(dv, io_);
258205
}
259206

260-
// Otherwise all entries must be position delete files. A DV must not be mixed
261-
// with position deletes: once a DV applies, readers ignore matching position
262-
// deletes, so a mixed list means the caller grouped delete files incorrectly.
207+
std::vector<std::shared_ptr<DataFile>> position_delete_files;
208+
263209
for (const auto& file : delete_files) {
264210
ICEBERG_PRECHECK(!file->IsDeletionVector(),
265211
"Deletion vector cannot be mixed with position delete files: {}",
@@ -268,14 +214,18 @@ Result<PositionDeleteIndex> DeleteLoader::LoadPositionDeletes(
268214
"Expected position delete file but got content type {}",
269215
ToString(file->content));
270216

271-
// A file-scoped position delete for another data file has nothing for the
272-
// target path; skip it as an optimization.
273217
if (file->referenced_data_file.has_value() &&
274218
file->referenced_data_file.value() != data_file_path) {
275219
continue;
276220
}
277221

278-
ICEBERG_RETURN_UNEXPECTED(LoadPositionDelete(file, index, data_file_path));
222+
position_delete_files.push_back(file);
223+
}
224+
225+
PositionDeleteIndex index(position_delete_files);
226+
227+
for (const auto& file : position_delete_files) {
228+
ICEBERG_RETURN_UNEXPECTED(LoadPositionDelete(*file, index, data_file_path));
279229
}
280230

281231
return index;

src/iceberg/data/delete_loader.h

Lines changed: 1 addition & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -67,14 +67,9 @@ class ICEBERG_DATA_EXPORT DeleteLoader {
6767

6868
private:
6969
/// \brief Load a single position delete file into the index.
70-
Status LoadPositionDelete(const std::shared_ptr<DataFile>& file,
71-
PositionDeleteIndex& index,
70+
Status LoadPositionDelete(const DataFile& file, PositionDeleteIndex& index,
7271
std::string_view data_file_path) const;
7372

74-
/// \brief Load a single deletion vector file into the index.
75-
Status LoadDV(const std::shared_ptr<DataFile>& file, PositionDeleteIndex& index,
76-
std::string_view data_file_path) const;
77-
7873
std::shared_ptr<FileIO> io_;
7974
};
8075

0 commit comments

Comments
 (0)