WZhuo commented on code in PR #822:
URL: https://github.com/apache/iceberg-cpp/pull/822#discussion_r3536130037
##########
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.
--
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]
