Copilot commented on code in PR #1007: URL: https://github.com/apache/iceberg-cpp/pull/1007#discussion_r4260921708
########## src/iceberg/inspect/refs_table.cc: ########## @@ -0,0 +1,122 @@ +/* + * 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 <cstdint> +#include <memory> +#include <optional> +#include <string> +#include <utility> +#include <variant> +#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(); +} Review Comment: `AppendOptional()` always appends via `AppendInt(... int64_t ...)`, but column 4 (`min_snapshots_to_keep`) is defined as `int32()` in the schema. If `AppendInt` enforces Arrow type compatibility (or writes width-specific buffers), this can fail at runtime or produce incorrectly typed data. Consider making `AppendOptional` type-aware (e.g., overload for `std::optional<int32_t>` vs `std::optional<int64_t>`, or a template using `if constexpr` on `value_type` to call the correct append helper) so the appended value matches the declared Arrow column type. ########## src/iceberg/test/CMakeLists.txt: ########## @@ -191,9 +191,11 @@ if(ICEBERG_BUILD_BUNDLE) add_iceberg_test(metadata_table_test USE_BUNDLE SOURCES - history_table_test.cc - metadata_table_test.cc - snapshots_table_test.cc) + inspect/history_table_test.cc + inspect/metadata_table_stream_test.cc + inspect/metadata_table_test.cc + inspect/refs_table_test.cc + inspect/snapshots_table_test.cc) Review Comment: This CMakeLists change points the `metadata_table_test` target at `inspect/*.cc` source paths, but the diffs shown for `history_table_test.cc` and `snapshots_table_test.cc` are still under `src/iceberg/test/` (non-`inspect/`) and no corresponding moved/added `inspect/history_table_test.cc` or `inspect/snapshots_table_test.cc` files are included here. As-is, this will break the build with missing source files. Fix by either (a) actually moving/adding the test sources under `src/iceberg/test/inspect/` to match these paths, or (b) keeping the original paths in CMake and only adjusting includes as needed. -- 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]
