Copilot commented on code in PR #1008: URL: https://github.com/apache/iceberg-cpp/pull/1008#discussion_r4260984247
########## src/iceberg/inspect/metadata_table_columns_internal.h: ########## @@ -0,0 +1,94 @@ +/* + * 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 + +/// \file iceberg/inspect/metadata_table_columns_internal.h +/// \brief Build only the selected columns of metadata table rows. + +#include <algorithm> +#include <cstddef> +#include <cstdint> +#include <span> +#include <vector> + +#include "iceberg/arrow_c_data.h" +#include "iceberg/arrow_row_builder_internal.h" +#include "iceberg/result.h" +#include "iceberg/schema.h" +#include "iceberg/schema_field.h" +#include "iceberg/type.h" +#include "iceberg/util/macros.h" + +namespace iceberg::internal { + +/// \brief Append the value of one metadata table column for a row. +template <typename Row> +using AppendColumnFn = Status (*)(ArrowArray* column, const Row& row); + +/// \brief A metadata table column and the function that appends its values. +template <typename Row> +struct MetadataTableColumn { + int32_t field_id; + AppendColumnFn<Row> append; +}; + +/// \brief Return the append functions for the columns of a projected schema. +/// +/// \param schema The metadata table schema. +/// \param projected_schema Projection of `schema` with the columns to return. +/// \param columns The columns of `schema` with their append functions. +/// \return One append function per column of `projected_schema`, in the same order, +/// or NotSupported if only some fields of a nested column are selected. +template <typename Row> +Result<std::vector<AppendColumnFn<Row>>> SelectAppendFns( + const Schema& schema, const Schema& projected_schema, + std::span<const MetadataTableColumn<Row>> columns) { + std::vector<AppendColumnFn<Row>> append_fns; + append_fns.reserve(projected_schema.fields().size()); + for (const auto& projected_field : projected_schema.fields()) { + ICEBERG_ASSIGN_OR_RAISE(auto field, schema.FindFieldById(projected_field.field_id())); + ICEBERG_CHECK(field.has_value(), "Metadata table column '{}' is not in the schema", + projected_field.name()); + if (*field->get().type() != *projected_field.type()) { + return NotSupported("Cannot select nested fields of metadata table column '{}'", + projected_field.name()); + } + auto column = std::ranges::find(columns, projected_field.field_id(), + &MetadataTableColumn<Row>::field_id); + ICEBERG_CHECK(column != columns.end(), + "Metadata table column '{}' has no append function", + projected_field.name()); Review Comment: `ICEBERG_CHECK` is used for conditions that can be triggered by input/projection mismatch (e.g., a projected schema containing a field ID not represented in `columns`). In library code, hard checks can terminate the process rather than returning a recoverable error. Prefer returning a structured error (e.g., `InvalidArgument` / `NotSupported`) instead of `ICEBERG_CHECK` for both the missing-field and missing-append-function cases. ########## src/iceberg/test/snapshots_table_test.cc: ########## @@ -132,6 +133,104 @@ TEST_F(SnapshotsTableTest, Scan) { EXPECT_THAT(second_summary, ::testing::Contains(::testing::Pair("total-records", "2"))); } +TEST_F(SnapshotsTableTest, ScanSelectedColumns) { + // Selected columns are returned in schema order, not in selection order. + ICEBERG_UNWRAP_OR_FAIL( + auto stream, snapshots_table_->Scan(ColumnSelection{ + .names = std::vector<std::string>{"operation", "snapshot_id"}})); + ICEBERG_UNWRAP_OR_FAIL(auto batches, ReadAllBatches(std::move(stream))); + ASSERT_EQ(batches.size(), 1); + const auto& batch = batches.front(); + EXPECT_EQ(batch->num_rows(), 2); + ASSERT_EQ(batch->num_columns(), 2); + EXPECT_EQ(batch->column_name(0), "snapshot_id"); + EXPECT_EQ(batch->column_name(1), "operation"); + ASSERT_EQ(batch->column(0)->type_id(), ::arrow::Type::INT64); + ASSERT_EQ(batch->column(1)->type_id(), ::arrow::Type::STRING); + + auto snapshot_ids = std::static_pointer_cast<::arrow::Int64Array>(batch->column(0)); + EXPECT_EQ(snapshot_ids->Value(0), 1); + EXPECT_EQ(snapshot_ids->Value(1), 2); + auto operations = std::static_pointer_cast<::arrow::StringArray>(batch->column(1)); + EXPECT_EQ(operations->GetString(0), "append"); + EXPECT_EQ(operations->GetString(1), "append"); +} + +TEST_F(SnapshotsTableTest, ScanSelectedColumnsIgnoresUnknownNames) { + ICEBERG_UNWRAP_OR_FAIL(auto stream, + snapshots_table_->Scan(ColumnSelection{ + .names = std::vector<std::string>{"parent_id", "missing"}})); + ICEBERG_UNWRAP_OR_FAIL(auto batches, ReadAllBatches(std::move(stream))); + ASSERT_EQ(batches.size(), 1); + ASSERT_EQ(batches.front()->num_columns(), 1); + EXPECT_EQ(batches.front()->column_name(0), "parent_id"); +} + +TEST_F(SnapshotsTableTest, ScanSelectedColumnsCaseSensitivity) { + ICEBERG_UNWRAP_OR_FAIL( + auto insensitive_stream, + snapshots_table_->Scan(ColumnSelection{ + .names = std::vector<std::string>{"SNAPSHOT_ID"}, .case_sensitive = false})); + ICEBERG_UNWRAP_OR_FAIL(auto insensitive_batches, + ReadAllBatches(std::move(insensitive_stream))); + ASSERT_EQ(insensitive_batches.size(), 1); + ASSERT_EQ(insensitive_batches.front()->num_columns(), 1); + EXPECT_EQ(insensitive_batches.front()->column_name(0), "snapshot_id"); + + // A case-sensitive selection does not match, so no columns are returned. + ICEBERG_UNWRAP_OR_FAIL(auto sensitive_stream, + snapshots_table_->Scan(ColumnSelection{ + .names = std::vector<std::string>{"SNAPSHOT_ID"}})); + ICEBERG_UNWRAP_OR_FAIL(auto sensitive_batches, + ReadAllBatches(std::move(sensitive_stream))); + ASSERT_EQ(sensitive_batches.size(), 1); + EXPECT_EQ(sensitive_batches.front()->num_columns(), 0); +} + +TEST_F(SnapshotsTableTest, ScanAllColumnsSelection) { + std::vector<std::optional<std::vector<std::string>>> selections = { + std::nullopt, std::vector<std::string>{"*"}}; + for (const auto& names : selections) { + ICEBERG_UNWRAP_OR_FAIL(auto stream, + snapshots_table_->Scan(ColumnSelection{.names = names})); + ICEBERG_UNWRAP_OR_FAIL(auto batches, ReadAllBatches(std::move(stream))); + ASSERT_EQ(batches.size(), 1); + EXPECT_EQ(batches.front()->num_rows(), 2); + EXPECT_EQ(batches.front()->num_columns(), 6); + } +} + +TEST_F(SnapshotsTableTest, ScanNoColumns) { + // An empty selection keeps the row count, for example to count snapshots. + ICEBERG_UNWRAP_OR_FAIL( + auto stream, + snapshots_table_->Scan(ColumnSelection{.names = std::vector<std::string>{}})); + ICEBERG_UNWRAP_OR_FAIL(auto batches, ReadAllBatches(std::move(stream))); + ASSERT_EQ(batches.size(), 1); + EXPECT_EQ(batches.front()->num_rows(), 2); + EXPECT_EQ(batches.front()->num_columns(), 0); +} + +TEST_F(SnapshotsTableTest, ScanSelectedMapColumn) { + // Selecting the map value selects the whole summary map. + ICEBERG_UNWRAP_OR_FAIL(auto stream, + snapshots_table_->Scan(ColumnSelection{ + .names = std::vector<std::string>{"summary.value"}})); + ICEBERG_UNWRAP_OR_FAIL(auto batches, ReadAllBatches(std::move(stream))); + ASSERT_EQ(batches.size(), 1); + const auto& batch = batches.front(); + ASSERT_EQ(batch->num_columns(), 1); + EXPECT_EQ(batch->column_name(0), "summary"); + ASSERT_EQ(batch->column(0)->type_id(), ::arrow::Type::MAP); + auto summaries = std::static_pointer_cast<::arrow::MapArray>(batch->column(0)); + EXPECT_EQ(summaries->value_length(0), 10); Review Comment: This assertion hard-codes the expected map size (`10`), which makes the test brittle if the fixture snapshot summary contents change (even if projection behavior is still correct). Consider asserting on specific expected keys/values (or at least non-emptiness and type) rather than an exact entry count. -- 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]
