WZhuo commented on code in PR #822:
URL: https://github.com/apache/iceberg-cpp/pull/822#discussion_r3536130026
##########
src/iceberg/avro/avro_data_util.cc:
##########
@@ -54,6 +54,22 @@ Status AppendFieldToBuilder(const ::avro::NodePtr& avro_node,
const arrow::MetadataColumnContext&
metadata_context,
::arrow::ArrayBuilder* array_builder);
+Status AppendRowLineageValue(int32_t field_id,
Review Comment:
**Suggestion:** `AppendRowLineageValue` is defined verbatim in both
`avro_data_util.cc:57` and `avro_direct_decoder.cc:186` with identical logic.
If the inheritance rules change (e.g., a v4 spec), both copies must be updated
in sync.
Consider extracting this function into the shared `metadata_column_util.cc`
(or a new internal utility header), alongside `HasRowLineageValue`. Both Avro
callers already cast to `Int64Builder*` and delegate — this would consolidate
that logic into one place.
```cpp
// In metadata_column_util.cc (or metadata_column_util_internal.h)
inline Status AppendRowLineageValue(int32_t field_id,
const arrow::MetadataColumnContext&
metadata_context,
::arrow::Int64Builder* int_builder) {
if (!arrow::HasRowLineageValue(field_id, metadata_context)) {
ICEBERG_ARROW_RETURN_NOT_OK(int_builder->AppendNull());
} else if (field_id == MetadataColumns::kRowIdColumnId) {
ICEBERG_ARROW_RETURN_NOT_OK(int_builder->Append(
metadata_context.first_row_id.value() +
metadata_context.next_file_pos));
} else {
ICEBERG_ARROW_RETURN_NOT_OK(
int_builder->Append(metadata_context.data_sequence_number.value()));
}
return {};
}
```
##########
src/iceberg/arrow/metadata_column_util.cc:
##########
@@ -53,4 +62,41 @@ Result<std::shared_ptr<::arrow::Array>>
MakeRowPositionArray(int64_t start_posit
return array;
}
+Result<std::shared_ptr<::arrow::Array>> MakeRowIdArray(
+ std::optional<int64_t> first_row_id, int64_t start_position, int64_t
num_rows,
+ ::arrow::MemoryPool* pool) {
+ ::arrow::Int64Builder builder(pool);
+ ICEBERG_ARROW_RETURN_NOT_OK(builder.Reserve(num_rows));
+ if (!first_row_id.has_value()) {
+ ICEBERG_ARROW_RETURN_NOT_OK(builder.AppendNulls(num_rows));
+ } else {
+ for (int64_t row_index = 0; row_index < num_rows; ++row_index) {
Review Comment:
**Suggestion:** `MakeRowIdArray` and `MakeLastUpdatedSequenceNumberArray`
both iterate row-by-row calling `Int64Builder::Append()` in a loop, but Arrow's
builder provides faster bulk APIs since the output length is known upfront
(`num_rows`):
- `Reserve(num_rows)` — already called here (good!), avoids O(n)
reallocations
- `AppendValues(const int64_t* values, int64_t length)` — appends a full
buffer in one call
- `AppendNulls(int64_t length)` — already used for the all-null case (good!)
The common cases are bulk-friendly:
- **All-synthesized** (hot path): every row gets `first_row_id + pos` (for
`_row_id`) or `data_sequence_number` (for `_last_updated_sequence_number`).
These can be computed into a `std::vector<int64_t>` and flushed with one
`AppendValues()` call instead of N individual `Append()` calls.
- **All-null**: already uses `AppendNulls()`. ✅
For the `_row_id` all-synthesized case, something like:
```cpp
std::vector<int64_t> values(num_rows);
for (int64_t i = 0; i < num_rows; ++i) {
values[i] = first_row_id.value() + start_position + i;
}
ICEBERG_ARROW_RETURN_NOT_OK(builder.AppendValues(values.data(),
values.size()));
```
And for `_last_updated_sequence_number`, the all-synthesized case is even
simpler:
```cpp
std::vector<int64_t> values(num_rows, data_sequence_number.value());
ICEBERG_ARROW_RETURN_NOT_OK(builder.AppendValues(values.data(),
values.size()));
```
This avoids per-element bounds checks and virtual dispatch overhead in the
loop.
##########
src/iceberg/inheritable_metadata.cc:
##########
@@ -64,6 +64,7 @@ Status BaseInheritableMetadata::Apply(ManifestEntry& entry) {
if (entry.data_file) {
entry.data_file->partition_spec_id = spec_id_;
+ entry.data_file->data_sequence_number = entry.sequence_number;
Review Comment:
**Suggestion:** `BaseInheritableMetadata::Apply()` only propagates
`data_sequence_number` to the `DataFile`, but the Java equivalent
(`InheritableMetadataFactory.BaseInheritableMetadata.apply()`) sets four fields:
```java
file.setSpecId(specId);
file.setDataSequenceNumber(manifestEntry.dataSequenceNumber());
file.setFileSequenceNumber(manifestEntry.fileSequenceNumber());
file.setManifestLocation(manifestLocation);
```
The C++ `DataFile` struct already has `spec_id` (set elsewhere). But
`file_sequence_number` and `manifest_location` are missing from both the struct
and `Apply()`. While no current C++ consumer needs these yet, future features
(incremental reads, changelog scans) will — filling the gap now avoids a latent
Java drift that's easy to miss later.
Suggestion: either add the fields and propagate them now, or leave an
explicit `// TODO: file_sequence_number and manifest_location` comment so the
gap is visible.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
