Copilot commented on code in PR #1007: URL: https://github.com/apache/iceberg-cpp/pull/1007#discussion_r4259708458
########## src/iceberg/inspect/refs_table.cc: ########## @@ -0,0 +1,109 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +#include "iceberg/inspect/refs_table.h" + +#include <algorithm> +#include <memory> +#include <optional> +#include <string> +#include <utility> +#include <vector> + +#include "iceberg/arrow_row_builder_internal.h" +#include "iceberg/inspect/metadata_table_stream_internal.h" +#include "iceberg/schema.h" +#include "iceberg/schema_field.h" +#include "iceberg/snapshot.h" +#include "iceberg/table.h" +#include "iceberg/table_metadata.h" +#include "iceberg/type.h" +#include "iceberg/util/macros.h" + +namespace iceberg { +namespace { + +struct RefRow { + std::string name; + std::string type; + int64_t snapshot_id; + std::optional<int64_t> max_ref_age_ms; + std::optional<int32_t> min_snapshots_to_keep; + std::optional<int64_t> max_snapshot_age_ms; +}; + +Status AppendOptional(ArrowArray* array, const auto& value) { + if (!value.has_value()) { + return AppendNull(array); + } + return AppendInt(array, static_cast<int64_t>(*value)); +} + +Status AppendRef(ArrowRowBuilder& builder, const RefRow& ref) { + ICEBERG_RETURN_UNEXPECTED(AppendString(builder.column(0), ref.name)); + ICEBERG_RETURN_UNEXPECTED(AppendString(builder.column(1), ref.type)); + ICEBERG_RETURN_UNEXPECTED(AppendInt(builder.column(2), ref.snapshot_id)); + ICEBERG_RETURN_UNEXPECTED(AppendOptional(builder.column(3), ref.max_ref_age_ms)); + ICEBERG_RETURN_UNEXPECTED(AppendOptional(builder.column(4), ref.min_snapshots_to_keep)); + ICEBERG_RETURN_UNEXPECTED(AppendOptional(builder.column(5), ref.max_snapshot_age_ms)); + return builder.FinishRow(); +} + +} // namespace + +RefsTable::RefsTable(std::shared_ptr<Table> table) : MetadataTable(std::move(table)) {} + +RefsTable::~RefsTable() = default; + +const std::shared_ptr<Schema>& RefsTable::schema() const { + static const auto schema = std::make_shared<Schema>(std::vector<SchemaField>{ + SchemaField::MakeRequired(1, "name", string()), + SchemaField::MakeRequired(2, "type", string()), + SchemaField::MakeRequired(3, "snapshot_id", int64()), + SchemaField::MakeOptional(4, "max_reference_age_in_ms", int64()), + SchemaField::MakeOptional(5, "min_snapshots_to_keep", int32()), + SchemaField::MakeOptional(6, "max_snapshot_age_in_ms", int64()), + }); + return schema; +} + +Result<std::unique_ptr<RefsTable>> RefsTable::Make(std::shared_ptr<Table> table) { + ICEBERG_PRECHECK(table != nullptr, "Table cannot be null"); + return std::unique_ptr<RefsTable>(new RefsTable(std::move(table))); +} + +Result<ArrowArrayStream> RefsTable::Scan() { + std::vector<RefRow> rows; + for (const auto& [name, ref] : source_table()->metadata()->refs) { + ICEBERG_PRECHECK(ref != nullptr, "Snapshot reference '{}' is null", name); + RefRow row{.name = name, + .type = ref->type() == SnapshotRefType::kBranch ? "BRANCH" : "TAG", + .snapshot_id = ref->snapshot_id, + .max_ref_age_ms = ref->max_ref_age_ms()}; + if (const auto* branch = std::get_if<SnapshotRef::Branch>(&ref->retention)) { + row.min_snapshots_to_keep = branch->min_snapshots_to_keep; + row.max_snapshot_age_ms = branch->max_snapshot_age_ms; + } + rows.push_back(std::move(row)); + } + std::ranges::sort(rows, {}, &RefRow::name); Review Comment: This introduces C++20-only features in implementation code: abbreviated function templates (`const auto&` parameter) and `std::ranges::sort`. If the project intends to support C++17 builds (the presence of `cpp20_compatibility_internal.h` suggests that may be the case), this will be a compile-time break. Consider rewriting `AppendOptional` as an explicit function template and replacing `std::ranges::sort` with `std::sort(rows.begin(), rows.end(), ...)` (or using a shared compatibility wrapper if one exists in this repo). ########## src/iceberg/inspect/refs_table.cc: ########## @@ -0,0 +1,109 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +#include "iceberg/inspect/refs_table.h" + +#include <algorithm> +#include <memory> +#include <optional> +#include <string> +#include <utility> +#include <vector> + +#include "iceberg/arrow_row_builder_internal.h" +#include "iceberg/inspect/metadata_table_stream_internal.h" +#include "iceberg/schema.h" +#include "iceberg/schema_field.h" +#include "iceberg/snapshot.h" +#include "iceberg/table.h" +#include "iceberg/table_metadata.h" +#include "iceberg/type.h" +#include "iceberg/util/macros.h" + +namespace iceberg { +namespace { + +struct RefRow { + std::string name; + std::string type; + int64_t snapshot_id; + std::optional<int64_t> max_ref_age_ms; + std::optional<int32_t> min_snapshots_to_keep; + std::optional<int64_t> max_snapshot_age_ms; +}; + +Status AppendOptional(ArrowArray* array, const auto& value) { Review Comment: This introduces C++20-only features in implementation code: abbreviated function templates (`const auto&` parameter) and `std::ranges::sort`. If the project intends to support C++17 builds (the presence of `cpp20_compatibility_internal.h` suggests that may be the case), this will be a compile-time break. Consider rewriting `AppendOptional` as an explicit function template and replacing `std::ranges::sort` with `std::sort(rows.begin(), rows.end(), ...)` (or using a shared compatibility wrapper if one exists in this repo). ########## src/iceberg/inspect/refs_table.cc: ########## @@ -0,0 +1,109 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +#include "iceberg/inspect/refs_table.h" + +#include <algorithm> +#include <memory> +#include <optional> +#include <string> +#include <utility> +#include <vector> + +#include "iceberg/arrow_row_builder_internal.h" +#include "iceberg/inspect/metadata_table_stream_internal.h" +#include "iceberg/schema.h" +#include "iceberg/schema_field.h" +#include "iceberg/snapshot.h" +#include "iceberg/table.h" +#include "iceberg/table_metadata.h" +#include "iceberg/type.h" +#include "iceberg/util/macros.h" + +namespace iceberg { +namespace { + +struct RefRow { + std::string name; + std::string type; + int64_t snapshot_id; + std::optional<int64_t> max_ref_age_ms; + std::optional<int32_t> min_snapshots_to_keep; + std::optional<int64_t> max_snapshot_age_ms; +}; + +Status AppendOptional(ArrowArray* array, const auto& value) { + if (!value.has_value()) { + return AppendNull(array); + } + return AppendInt(array, static_cast<int64_t>(*value)); +} + Review Comment: `AppendOptional` always appends via `AppendInt(... int64_t ...)`, but column 4 in this table schema is `int32` (`min_snapshots_to_keep`). If `AppendInt` is implemented as an int64 nanoarrow append, this will fail at runtime (or emit incorrect data) when appending into an int32 array. Prefer a typed helper (e.g., overloads for `std::optional<int32_t>` vs `std::optional<int64_t>`, or a template that dispatches to the correct append function based on the optional's value type / Arrow type). ########## src/iceberg/inspect/metadata_table_stream_internal.h: ########## @@ -0,0 +1,149 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +#pragma once + +#include <cstddef> +#include <functional> +#include <memory> +#include <optional> +#include <utility> +#include <vector> + +#include <nanoarrow/nanoarrow.h> + +#include "iceberg/arrow_c_data_util_internal.h" +#include "iceberg/arrow_row_builder_internal.h" +#include "iceberg/inspect/metadata_table.h" +#include "iceberg/nanoarrow_status_internal.h" +#include "iceberg/schema.h" +#include "iceberg/schema_internal.h" +#include "iceberg/util/macros.h" +#include "iceberg/util/stream.h" + +namespace iceberg::internal { + +/// \brief Arrow stream backed by a metadata-table row iterator. +template <typename Row> +class MetadataTableRowsStream { + public: + using AppendRow = std::function<Status(ArrowRowBuilder&, const Row&)>; + + static Result<std::unique_ptr<MetadataTableRowsStream>> Make( + const Schema& schema, std::unique_ptr<Stream<Row>> rows, AppendRow append_row) { + ArrowSchema arrow_schema{}; + ICEBERG_RETURN_UNEXPECTED(ToArrowSchema(schema, &arrow_schema)); + return std::unique_ptr<MetadataTableRowsStream>(new MetadataTableRowsStream( + std::move(rows), std::move(append_row), std::move(arrow_schema))); + } + + ~MetadataTableRowsStream() { + auto status = Close(); + static_cast<void>(status); + } + + Status Close() { + rows_.reset(); + append_row_ = nullptr; + if (arrow_schema_.release != nullptr) { + ArrowSchemaRelease(&arrow_schema_); + } + return {}; + } + + Result<std::optional<ArrowArray>> Next() { + ICEBERG_PRECHECK(arrow_schema_.release != nullptr, + "Cannot read from a closed metadata table stream"); + ICEBERG_ASSIGN_OR_RAISE(auto builder, ArrowRowBuilder::Make(&arrow_schema_)); + while (builder.num_rows() < MetadataTable::kBatchSize) { + ICEBERG_ASSIGN_OR_RAISE(auto row, rows_->Next()); + if (!row.has_value()) { + break; + } + ICEBERG_RETURN_UNEXPECTED(append_row_(builder, *row)); + } + if (builder.num_rows() == 0) { + return std::nullopt; + } + + ICEBERG_ASSIGN_OR_RAISE(auto array, std::move(builder).Finish()); + return array; + } Review Comment: The new stream implementation adds batching logic via `MetadataTable::kBatchSize`, but the added tests only cover a single-batch case (3 rows). Add a unit test that produces `kBatchSize + 1` rows and asserts: (1) two batches are returned, (2) batch sizes are `kBatchSize` and `1`, and (3) no extra empty batch is emitted. This will prevent regressions in the row iteration / batch boundary behavior. -- 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]
