wgtmac commented on code in PR #1066: URL: https://github.com/apache/iceberg-cpp/pull/1066#discussion_r4425224027
########## src/iceberg/inspect/metadata_scan.h: ########## @@ -0,0 +1,123 @@ +/* + * 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_scan.h +/// \brief Configure metadata table scans, plan tasks, and read rows as Arrow batches. + +#include <memory> +#include <string> +#include <vector> + +#include "iceberg/arrow_c_data.h" +#include "iceberg/iceberg_export.h" +#include "iceberg/inspect/metadata_scan_task.h" +#include "iceberg/result.h" +#include "iceberg/type_fwd.h" +#include "iceberg/util/error_collector.h" +#include "iceberg/util/stream.h" + +namespace iceberg { + +using MetadataScanTaskStream = Stream<std::shared_ptr<MetadataScanTask>>; +using MetadataScanTaskStreamPtr = std::unique_ptr<MetadataScanTaskStream>; + +namespace internal { + +/// \brief Scan settings and the source metadata captured by the builder. +struct MetadataScanContext { + MetadataTableKind kind; + std::shared_ptr<const TableMetadata> table_metadata; + std::string metadata_file_location; + std::shared_ptr<Schema> schema; // metadata table schema + std::shared_ptr<Schema> projected_schema; + std::shared_ptr<FileIO> io; + bool case_sensitive = true; +}; + +} // namespace internal + +/// \brief Configure a scan over the metadata captured when this builder is created. +class ICEBERG_EXPORT MetadataScanBuilder : public ErrorCollector { + public: + /// \brief Create a builder for a metadata table; source and schema must be non-null. + /// + /// The schema must describe the metadata table, not the source data table. + static Result<std::unique_ptr<MetadataScanBuilder>> Make( + std::shared_ptr<const Table> source, MetadataTableKind kind, + std::shared_ptr<Schema> schema); + + /// \brief Select columns by name; nested columns use dot-separated paths. + /// + /// An empty list selects no columns; `*` selects all columns. Names are resolved + /// immediately, using the current CaseSensitive() setting. + MetadataScanBuilder& Select(const std::vector<std::string>& columns); + + /// \brief Set the output schema to a projection of the metadata table schema. + MetadataScanBuilder& Project(std::shared_ptr<Schema> schema); + + /// \brief Use case-sensitive column names; enabled by default. + MetadataScanBuilder& CaseSensitive(bool value); + + /// \brief Validate the settings and build a scan with its own copy of them. + /// + /// Returns validation errors or NotImplemented for unsupported metadata table kinds. + /// Later builder changes and table refreshes do not change the returned scan. + Result<std::unique_ptr<MetadataScan>> Build(); + + private: + MetadataScanBuilder(std::shared_ptr<const Table> source, MetadataTableKind kind, + std::shared_ptr<Schema> schema); + + internal::MetadataScanContext context_; +}; + +/// \brief A metadata scan that can plan reusable tasks or read their rows as Arrow. +class ICEBERG_EXPORT MetadataScan { + public: + virtual ~MetadataScan(); + + /// \brief Return a fresh task stream; consuming it may report planning errors. + /// + /// Destroying the stream releases its resources, including after a partial read. + virtual Result<MetadataScanTaskStreamPtr> PlanTasksStream() const = 0; + + /// \brief Collect all tasks, returning any error from PlanTasksStream(). + virtual Result<std::vector<std::shared_ptr<MetadataScanTask>>> PlanTasks() const; + + /// \brief Read every task as one Arrow stream with the requested output schema. + /// + /// The default implementation collects tasks, then reads them in planning order. + /// The returned stream owns its read state and can outlive this scan. The caller + /// must release the stream and each returned schema or array independently. + /// Releasing the stream early closes the active task stream. To customize + /// batch size, open planned tasks with MetadataScanTaskReader instead. Review Comment: The task now carries the projection, so callers no longer need the scan context. The reader only takes a batch size. ########## src/iceberg/util/stream.h: ########## @@ -136,4 +141,26 @@ class Stream { std::optional<Error> error_; }; +template <typename T> +std::unique_ptr<Stream<T>> Stream<T>::FromVector(std::vector<T> values) { + class VectorStream final : public Stream<T> { + public: + explicit VectorStream(std::vector<T> values) : values_(std::move(values)) {} + + protected: + Result<std::optional<T>> NextImpl() override { + if (index_ == values_.size()) { + return std::nullopt; + } + return Result<std::optional<T>>(std::in_place, std::in_place, + std::move_if_noexcept(values_[index_++])); Review Comment: Fixed by binding the element before calling `move_if_noexcept`. Added a `vector<bool>` test too. ########## src/iceberg/inspect/metadata_scan.h: ########## @@ -0,0 +1,123 @@ +/* + * 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_scan.h +/// \brief Configure metadata table scans, plan tasks, and read rows as Arrow batches. + +#include <memory> +#include <string> +#include <vector> + +#include "iceberg/arrow_c_data.h" +#include "iceberg/iceberg_export.h" +#include "iceberg/inspect/metadata_scan_task.h" +#include "iceberg/result.h" +#include "iceberg/type_fwd.h" +#include "iceberg/util/error_collector.h" +#include "iceberg/util/stream.h" + +namespace iceberg { + +using MetadataScanTaskStream = Stream<std::shared_ptr<MetadataScanTask>>; +using MetadataScanTaskStreamPtr = std::unique_ptr<MetadataScanTaskStream>; + +namespace internal { + +/// \brief Scan settings and the source metadata captured by the builder. +struct MetadataScanContext { + MetadataTableKind kind; + std::shared_ptr<const TableMetadata> table_metadata; + std::string metadata_file_location; Review Comment: Removed both fields for now. We can add `FileIO` when a task actually needs to read files. ########## src/iceberg/inspect/metadata_scan_util_internal.h: ########## @@ -0,0 +1,140 @@ +/* + * 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_scan_util_internal.h +/// \brief Read in-memory metadata as Arrow batches produced on demand. + +#include <algorithm> +#include <cstdint> +#include <memory> +#include <optional> +#include <type_traits> +#include <utility> +#include <vector> + +#include "iceberg/arrow_c_data.h" +#include "iceberg/arrow_c_data_util_internal.h" +#include "iceberg/arrow_row_builder_internal.h" +#include "iceberg/result.h" +#include "iceberg/schema.h" +#include "iceberg/schema_internal.h" +#include "iceberg/util/macros.h" + +namespace iceberg::internal { + +/// \brief Build one Arrow batch per read, retaining the row producer's owners. +template <typename AppendValue> +class MetadataBatchSource { + public: + MetadataBatchSource(std::shared_ptr<iceberg::Schema> schema, + std::vector<int32_t> field_ids, size_t num_rows, int64_t batch_size, + AppendValue append_value) + : schema_(std::move(schema)), + field_ids_(std::move(field_ids)), + num_rows_(num_rows), + batch_size_(static_cast<size_t>(batch_size)), + append_value_(std::move(append_value)) {} + + Status Close() { return {}; } + + Result<ArrowSchema> Schema() { + ArrowSchema schema{}; + ICEBERG_RETURN_UNEXPECTED(ToArrowSchema(*schema_, &schema)); + return schema; + } + + Result<std::optional<ArrowArray>> Next() { + if (error_) { + return unexpected(*error_); + } + auto result = NextBatch(); + if (!result) { + error_ = result.error(); + } + return result; + } + + private: + Result<std::optional<ArrowArray>> NextBatch() { + if (index_ == num_rows_) { + return std::nullopt; + } + const size_t end = index_ + std::min(batch_size_, num_rows_ - index_); + ICEBERG_ASSIGN_OR_RAISE(auto builder, ArrowRowBuilder::Make(*schema_)); + while (index_ < end) { + for (size_t i = 0; i < field_ids_.size(); ++i) { + ICEBERG_RETURN_UNEXPECTED(append_value_(builder.column(static_cast<int64_t>(i)), + field_ids_[i], index_)); + } + ICEBERG_RETURN_UNEXPECTED(builder.FinishRow()); + ++index_; + } + ICEBERG_ASSIGN_OR_RAISE(auto batch, std::move(builder).Finish()); + return batch; + } + + const std::shared_ptr<iceberg::Schema> schema_; + const std::vector<int32_t> field_ids_; + const size_t num_rows_; + const size_t batch_size_; + AppendValue append_value_; + size_t index_ = 0; + std::optional<Error> error_; +}; + +/// \brief Create a stream that builds at most batch_size metadata rows per read. +/// +/// Each output column selects a whole canonical column by field ID, without +/// tightening its nullability. The row producer must capture shared ownership +/// of everything it uses; it is invoked only when the stream is read. +template <typename AppendValue> +Result<ArrowArrayStream> MakeMetadataBatchStream( + const std::shared_ptr<Schema>& canonical_schema, + const std::shared_ptr<Schema>& projected_schema, size_t num_rows, int64_t batch_size, + AppendValue&& append_value) { + ICEBERG_PRECHECK(batch_size > 0, "Batch size must be positive"); + const auto& output_schema = projected_schema ? projected_schema : canonical_schema; + std::vector<int32_t> field_ids; + field_ids.reserve(output_schema->fields().size()); + for (const auto& field : output_schema->fields()) { + ICEBERG_ASSIGN_OR_RAISE(auto canonical_field, + canonical_schema->FindFieldById(field.field_id())); Review Comment: Switched to `GetFieldById()` and moved projection checks to `Build()`. Added a test that rejects a nested field ID as a top-level column. ########## src/iceberg/inspect/snapshots_table.cc: ########## @@ -19,164 +19,136 @@ #include "iceberg/inspect/snapshots_table.h" -#include <chrono> -#include <cstddef> #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/nanoarrow_status_internal.h" +#include "iceberg/inspect/metadata_scan_task.h" +#include "iceberg/inspect/metadata_scan_task_reader.h" +#include "iceberg/inspect/metadata_scan_util_internal.h" #include "iceberg/schema.h" #include "iceberg/schema_field.h" -#include "iceberg/schema_internal.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 { - -Status AppendSnapshot(ArrowRowBuilder& builder, const Snapshot& snapshot) { - ICEBERG_RETURN_UNEXPECTED( - AppendInt(builder.column(0), std::chrono::duration_cast<std::chrono::microseconds>( - snapshot.timestamp_ms.time_since_epoch()) - .count())); - ICEBERG_RETURN_UNEXPECTED(AppendInt(builder.column(1), snapshot.snapshot_id)); - - if (snapshot.parent_snapshot_id.has_value()) { - ICEBERG_RETURN_UNEXPECTED(AppendInt(builder.column(2), *snapshot.parent_snapshot_id)); - } else { - ICEBERG_RETURN_UNEXPECTED(AppendNull(builder.column(2))); - } - - auto operation = snapshot.Operation(); - if (operation.has_value()) { - ICEBERG_RETURN_UNEXPECTED(AppendString(builder.column(3), *operation)); - } else { - ICEBERG_RETURN_UNEXPECTED(AppendNull(builder.column(3))); - } - - ICEBERG_RETURN_UNEXPECTED(AppendString(builder.column(4), snapshot.manifest_list)); - - if (!snapshot.summary.empty()) { - if (snapshot.summary.contains(SnapshotSummaryFields::kOperation)) { - auto summary = snapshot.summary; - summary.erase(SnapshotSummaryFields::kOperation); - ICEBERG_RETURN_UNEXPECTED(AppendStringMap(builder.column(5), summary)); - } else { - ICEBERG_RETURN_UNEXPECTED(AppendStringMap(builder.column(5), snapshot.summary)); - } - } else { - ICEBERG_RETURN_UNEXPECTED(AppendNull(builder.column(5))); - } - - return builder.FinishRow(); -} - -class SnapshotsTableStream { - public: - static Result<std::unique_ptr<SnapshotsTableStream>> Make( - std::shared_ptr<const TableMetadata> metadata, const iceberg::Schema& schema) { - ArrowSchema arrow_schema{}; - ICEBERG_RETURN_UNEXPECTED(ToArrowSchema(schema, &arrow_schema)); - return std::unique_ptr<SnapshotsTableStream>( - new SnapshotsTableStream(std::move(metadata), std::move(arrow_schema))); - } - - ~SnapshotsTableStream() = default; - - Status Close() { - metadata_.reset(); - if (arrow_schema_.release != nullptr) { - ArrowSchemaRelease(&arrow_schema_); - } - return {}; - } - - Result<std::optional<ArrowArray>> Next() { - if (metadata_ == nullptr) [[unlikely]] { - return InvalidArgument("Cannot read from a closed snapshots table stream"); - } - const auto& snapshots = metadata_->snapshots; - if (next_snapshot_ == snapshots.size()) { - return std::nullopt; - } - - ICEBERG_ASSIGN_OR_RAISE(auto builder, ArrowRowBuilder::Make(&arrow_schema_)); - while (next_snapshot_ < snapshots.size() && - builder.num_rows() < MetadataTable::kBatchSize) { - const auto& snapshot = snapshots[next_snapshot_++]; - if (snapshot == nullptr) [[unlikely]] { - continue; - } - ICEBERG_RETURN_UNEXPECTED(AppendSnapshot(builder, *snapshot)); - } - if (builder.num_rows() == 0) { - return std::nullopt; - } - - ICEBERG_ASSIGN_OR_RAISE(auto array, std::move(builder).Finish()); - return array; - } - - Result<ArrowSchema> Schema() { - if (arrow_schema_.release == nullptr) [[unlikely]] { - return InvalidArgument("Cannot read schema from a closed snapshots table stream"); - } - ArrowSchema schema_copy{}; - ICEBERG_NANOARROW_RETURN_UNEXPECTED( - ArrowSchemaDeepCopy(&arrow_schema_, &schema_copy)); - return schema_copy; - } - private: - SnapshotsTableStream(std::shared_ptr<const TableMetadata> metadata, - ArrowSchema arrow_schema) - : metadata_(std::move(metadata)), arrow_schema_(std::move(arrow_schema)) {} - - std::shared_ptr<const TableMetadata> metadata_; - ArrowSchema arrow_schema_{}; - size_t next_snapshot_ = 0; -}; - -} // namespace - -SnapshotsTable::SnapshotsTable(std::shared_ptr<Table> table) +// SnapshotsTable implementation +SnapshotsTable::SnapshotsTable(std::shared_ptr<const Table> table) : MetadataTable(std::move(table)) {} SnapshotsTable::~SnapshotsTable() = default; const std::shared_ptr<Schema>& SnapshotsTable::schema() const { + return CanonicalSchema(); +} + +const std::shared_ptr<Schema>& SnapshotsTable::CanonicalSchema() { static const auto schema = std::make_shared<Schema>(std::vector<SchemaField>{ SchemaField::MakeRequired(1, "committed_at", timestamp_tz()), SchemaField::MakeRequired(2, "snapshot_id", int64()), SchemaField::MakeOptional(3, "parent_id", int64()), SchemaField::MakeOptional(4, "operation", string()), SchemaField::MakeOptional(5, "manifest_list", string()), - SchemaField::MakeOptional(6, "summary", - std::make_shared<iceberg::MapType>( - SchemaField::MakeRequired(7, "key", string()), + SchemaField::MakeOptional( + 6, "summary", + std::make_shared<MapType>(SchemaField::MakeRequired(7, "key", string()), SchemaField::MakeRequired(8, "value", string())))}); return schema; } Result<std::unique_ptr<SnapshotsTable>> SnapshotsTable::Make( - std::shared_ptr<Table> table) { + std::shared_ptr<const Table> table) { ICEBERG_PRECHECK(table != nullptr, "Table cannot be null"); return std::unique_ptr<SnapshotsTable>(new SnapshotsTable(std::move(table))); } -Result<ArrowArrayStream> SnapshotsTable::Scan() { +Result<ArrowArrayStream> SnapshotsTable::ReadRows( + std::shared_ptr<const TableMetadata> metadata, + const std::shared_ptr<Schema>& projected_schema, int64_t batch_size) { + std::vector<size_t> snapshot_indices; + for (size_t i = 0; i < metadata->snapshots.size(); ++i) { + if (metadata->snapshots[i]) { Review Comment: Restored `ScanSkipsNullSnapshots`. It includes a null entry and checks that the two valid snapshots are still returned. ########## src/iceberg/inspect/snapshots_table.cc: ########## @@ -19,164 +19,136 @@ #include "iceberg/inspect/snapshots_table.h" -#include <chrono> -#include <cstddef> #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/nanoarrow_status_internal.h" +#include "iceberg/inspect/metadata_scan_task.h" +#include "iceberg/inspect/metadata_scan_task_reader.h" +#include "iceberg/inspect/metadata_scan_util_internal.h" #include "iceberg/schema.h" #include "iceberg/schema_field.h" -#include "iceberg/schema_internal.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 { - -Status AppendSnapshot(ArrowRowBuilder& builder, const Snapshot& snapshot) { - ICEBERG_RETURN_UNEXPECTED( - AppendInt(builder.column(0), std::chrono::duration_cast<std::chrono::microseconds>( - snapshot.timestamp_ms.time_since_epoch()) - .count())); - ICEBERG_RETURN_UNEXPECTED(AppendInt(builder.column(1), snapshot.snapshot_id)); - - if (snapshot.parent_snapshot_id.has_value()) { - ICEBERG_RETURN_UNEXPECTED(AppendInt(builder.column(2), *snapshot.parent_snapshot_id)); - } else { - ICEBERG_RETURN_UNEXPECTED(AppendNull(builder.column(2))); - } - - auto operation = snapshot.Operation(); - if (operation.has_value()) { - ICEBERG_RETURN_UNEXPECTED(AppendString(builder.column(3), *operation)); - } else { - ICEBERG_RETURN_UNEXPECTED(AppendNull(builder.column(3))); - } - - ICEBERG_RETURN_UNEXPECTED(AppendString(builder.column(4), snapshot.manifest_list)); - - if (!snapshot.summary.empty()) { - if (snapshot.summary.contains(SnapshotSummaryFields::kOperation)) { - auto summary = snapshot.summary; - summary.erase(SnapshotSummaryFields::kOperation); - ICEBERG_RETURN_UNEXPECTED(AppendStringMap(builder.column(5), summary)); - } else { - ICEBERG_RETURN_UNEXPECTED(AppendStringMap(builder.column(5), snapshot.summary)); - } - } else { - ICEBERG_RETURN_UNEXPECTED(AppendNull(builder.column(5))); - } - - return builder.FinishRow(); -} - -class SnapshotsTableStream { - public: - static Result<std::unique_ptr<SnapshotsTableStream>> Make( - std::shared_ptr<const TableMetadata> metadata, const iceberg::Schema& schema) { - ArrowSchema arrow_schema{}; - ICEBERG_RETURN_UNEXPECTED(ToArrowSchema(schema, &arrow_schema)); - return std::unique_ptr<SnapshotsTableStream>( - new SnapshotsTableStream(std::move(metadata), std::move(arrow_schema))); - } - - ~SnapshotsTableStream() = default; - - Status Close() { - metadata_.reset(); - if (arrow_schema_.release != nullptr) { - ArrowSchemaRelease(&arrow_schema_); - } - return {}; - } - - Result<std::optional<ArrowArray>> Next() { - if (metadata_ == nullptr) [[unlikely]] { - return InvalidArgument("Cannot read from a closed snapshots table stream"); - } - const auto& snapshots = metadata_->snapshots; - if (next_snapshot_ == snapshots.size()) { - return std::nullopt; - } - - ICEBERG_ASSIGN_OR_RAISE(auto builder, ArrowRowBuilder::Make(&arrow_schema_)); - while (next_snapshot_ < snapshots.size() && - builder.num_rows() < MetadataTable::kBatchSize) { - const auto& snapshot = snapshots[next_snapshot_++]; - if (snapshot == nullptr) [[unlikely]] { - continue; - } - ICEBERG_RETURN_UNEXPECTED(AppendSnapshot(builder, *snapshot)); - } - if (builder.num_rows() == 0) { - return std::nullopt; - } - - ICEBERG_ASSIGN_OR_RAISE(auto array, std::move(builder).Finish()); - return array; - } - - Result<ArrowSchema> Schema() { - if (arrow_schema_.release == nullptr) [[unlikely]] { - return InvalidArgument("Cannot read schema from a closed snapshots table stream"); - } - ArrowSchema schema_copy{}; - ICEBERG_NANOARROW_RETURN_UNEXPECTED( - ArrowSchemaDeepCopy(&arrow_schema_, &schema_copy)); - return schema_copy; - } - private: - SnapshotsTableStream(std::shared_ptr<const TableMetadata> metadata, - ArrowSchema arrow_schema) - : metadata_(std::move(metadata)), arrow_schema_(std::move(arrow_schema)) {} - - std::shared_ptr<const TableMetadata> metadata_; - ArrowSchema arrow_schema_{}; - size_t next_snapshot_ = 0; -}; - -} // namespace - -SnapshotsTable::SnapshotsTable(std::shared_ptr<Table> table) +// SnapshotsTable implementation +SnapshotsTable::SnapshotsTable(std::shared_ptr<const Table> table) : MetadataTable(std::move(table)) {} SnapshotsTable::~SnapshotsTable() = default; const std::shared_ptr<Schema>& SnapshotsTable::schema() const { + return CanonicalSchema(); +} + +const std::shared_ptr<Schema>& SnapshotsTable::CanonicalSchema() { static const auto schema = std::make_shared<Schema>(std::vector<SchemaField>{ SchemaField::MakeRequired(1, "committed_at", timestamp_tz()), SchemaField::MakeRequired(2, "snapshot_id", int64()), SchemaField::MakeOptional(3, "parent_id", int64()), SchemaField::MakeOptional(4, "operation", string()), SchemaField::MakeOptional(5, "manifest_list", string()), - SchemaField::MakeOptional(6, "summary", - std::make_shared<iceberg::MapType>( - SchemaField::MakeRequired(7, "key", string()), + SchemaField::MakeOptional( + 6, "summary", + std::make_shared<MapType>(SchemaField::MakeRequired(7, "key", string()), SchemaField::MakeRequired(8, "value", string())))}); return schema; } Result<std::unique_ptr<SnapshotsTable>> SnapshotsTable::Make( - std::shared_ptr<Table> table) { + std::shared_ptr<const Table> table) { ICEBERG_PRECHECK(table != nullptr, "Table cannot be null"); return std::unique_ptr<SnapshotsTable>(new SnapshotsTable(std::move(table))); } -Result<ArrowArrayStream> SnapshotsTable::Scan() { +Result<ArrowArrayStream> SnapshotsTable::ReadRows( + std::shared_ptr<const TableMetadata> metadata, + const std::shared_ptr<Schema>& projected_schema, int64_t batch_size) { + std::vector<size_t> snapshot_indices; + for (size_t i = 0; i < metadata->snapshots.size(); ++i) { + if (metadata->snapshots[i]) { + snapshot_indices.push_back(i); + } + } + const size_t num_rows = snapshot_indices.size(); + + return internal::MakeMetadataBatchStream( + CanonicalSchema(), projected_schema, num_rows, batch_size, + [metadata = std::move(metadata), snapshot_indices = std::move(snapshot_indices)]( + ArrowArray* column, int32_t field_id, size_t row) -> Status { + const auto& snapshot = *metadata->snapshots[snapshot_indices[row]]; + switch (field_id) { + case 1: // committed_at + return AppendInt(column, UnixMsFromTimePointMs(snapshot.timestamp_ms) * 1000); + case 2: // snapshot_id + return AppendInt(column, snapshot.snapshot_id); + case 3: // parent_id + return snapshot.parent_snapshot_id + ? AppendInt(column, *snapshot.parent_snapshot_id) + : AppendNull(column); + case 4: { // operation + auto operation = snapshot.Operation(); + return operation ? AppendString(column, *operation) : AppendNull(column); + } + case 5: // manifest_list + return snapshot.manifest_list.empty() Review Comment: Added this to `ScanEmptySummary`. It now checks that an empty manifest list produces null. ########## src/iceberg/inspect/snapshots_table.cc: ########## @@ -19,164 +19,136 @@ #include "iceberg/inspect/snapshots_table.h" -#include <chrono> -#include <cstddef> #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/nanoarrow_status_internal.h" +#include "iceberg/inspect/metadata_scan_task.h" +#include "iceberg/inspect/metadata_scan_task_reader.h" +#include "iceberg/inspect/metadata_scan_util_internal.h" #include "iceberg/schema.h" #include "iceberg/schema_field.h" -#include "iceberg/schema_internal.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 { - -Status AppendSnapshot(ArrowRowBuilder& builder, const Snapshot& snapshot) { - ICEBERG_RETURN_UNEXPECTED( - AppendInt(builder.column(0), std::chrono::duration_cast<std::chrono::microseconds>( - snapshot.timestamp_ms.time_since_epoch()) - .count())); - ICEBERG_RETURN_UNEXPECTED(AppendInt(builder.column(1), snapshot.snapshot_id)); - - if (snapshot.parent_snapshot_id.has_value()) { - ICEBERG_RETURN_UNEXPECTED(AppendInt(builder.column(2), *snapshot.parent_snapshot_id)); - } else { - ICEBERG_RETURN_UNEXPECTED(AppendNull(builder.column(2))); - } - - auto operation = snapshot.Operation(); - if (operation.has_value()) { - ICEBERG_RETURN_UNEXPECTED(AppendString(builder.column(3), *operation)); - } else { - ICEBERG_RETURN_UNEXPECTED(AppendNull(builder.column(3))); - } - - ICEBERG_RETURN_UNEXPECTED(AppendString(builder.column(4), snapshot.manifest_list)); - - if (!snapshot.summary.empty()) { - if (snapshot.summary.contains(SnapshotSummaryFields::kOperation)) { - auto summary = snapshot.summary; - summary.erase(SnapshotSummaryFields::kOperation); - ICEBERG_RETURN_UNEXPECTED(AppendStringMap(builder.column(5), summary)); - } else { - ICEBERG_RETURN_UNEXPECTED(AppendStringMap(builder.column(5), snapshot.summary)); - } - } else { - ICEBERG_RETURN_UNEXPECTED(AppendNull(builder.column(5))); - } - - return builder.FinishRow(); -} - -class SnapshotsTableStream { - public: - static Result<std::unique_ptr<SnapshotsTableStream>> Make( - std::shared_ptr<const TableMetadata> metadata, const iceberg::Schema& schema) { - ArrowSchema arrow_schema{}; - ICEBERG_RETURN_UNEXPECTED(ToArrowSchema(schema, &arrow_schema)); - return std::unique_ptr<SnapshotsTableStream>( - new SnapshotsTableStream(std::move(metadata), std::move(arrow_schema))); - } - - ~SnapshotsTableStream() = default; - - Status Close() { - metadata_.reset(); - if (arrow_schema_.release != nullptr) { - ArrowSchemaRelease(&arrow_schema_); - } - return {}; - } - - Result<std::optional<ArrowArray>> Next() { - if (metadata_ == nullptr) [[unlikely]] { - return InvalidArgument("Cannot read from a closed snapshots table stream"); - } - const auto& snapshots = metadata_->snapshots; - if (next_snapshot_ == snapshots.size()) { - return std::nullopt; - } - - ICEBERG_ASSIGN_OR_RAISE(auto builder, ArrowRowBuilder::Make(&arrow_schema_)); - while (next_snapshot_ < snapshots.size() && - builder.num_rows() < MetadataTable::kBatchSize) { - const auto& snapshot = snapshots[next_snapshot_++]; - if (snapshot == nullptr) [[unlikely]] { - continue; - } - ICEBERG_RETURN_UNEXPECTED(AppendSnapshot(builder, *snapshot)); - } - if (builder.num_rows() == 0) { - return std::nullopt; - } - - ICEBERG_ASSIGN_OR_RAISE(auto array, std::move(builder).Finish()); - return array; - } - - Result<ArrowSchema> Schema() { - if (arrow_schema_.release == nullptr) [[unlikely]] { - return InvalidArgument("Cannot read schema from a closed snapshots table stream"); - } - ArrowSchema schema_copy{}; - ICEBERG_NANOARROW_RETURN_UNEXPECTED( - ArrowSchemaDeepCopy(&arrow_schema_, &schema_copy)); - return schema_copy; - } - private: - SnapshotsTableStream(std::shared_ptr<const TableMetadata> metadata, - ArrowSchema arrow_schema) - : metadata_(std::move(metadata)), arrow_schema_(std::move(arrow_schema)) {} - - std::shared_ptr<const TableMetadata> metadata_; - ArrowSchema arrow_schema_{}; - size_t next_snapshot_ = 0; -}; - -} // namespace - -SnapshotsTable::SnapshotsTable(std::shared_ptr<Table> table) +// SnapshotsTable implementation +SnapshotsTable::SnapshotsTable(std::shared_ptr<const Table> table) : MetadataTable(std::move(table)) {} SnapshotsTable::~SnapshotsTable() = default; const std::shared_ptr<Schema>& SnapshotsTable::schema() const { + return CanonicalSchema(); +} + +const std::shared_ptr<Schema>& SnapshotsTable::CanonicalSchema() { static const auto schema = std::make_shared<Schema>(std::vector<SchemaField>{ SchemaField::MakeRequired(1, "committed_at", timestamp_tz()), SchemaField::MakeRequired(2, "snapshot_id", int64()), SchemaField::MakeOptional(3, "parent_id", int64()), SchemaField::MakeOptional(4, "operation", string()), SchemaField::MakeOptional(5, "manifest_list", string()), - SchemaField::MakeOptional(6, "summary", - std::make_shared<iceberg::MapType>( - SchemaField::MakeRequired(7, "key", string()), + SchemaField::MakeOptional( + 6, "summary", + std::make_shared<MapType>(SchemaField::MakeRequired(7, "key", string()), SchemaField::MakeRequired(8, "value", string())))}); return schema; } Result<std::unique_ptr<SnapshotsTable>> SnapshotsTable::Make( - std::shared_ptr<Table> table) { + std::shared_ptr<const Table> table) { ICEBERG_PRECHECK(table != nullptr, "Table cannot be null"); return std::unique_ptr<SnapshotsTable>(new SnapshotsTable(std::move(table))); } -Result<ArrowArrayStream> SnapshotsTable::Scan() { +Result<ArrowArrayStream> SnapshotsTable::ReadRows( + std::shared_ptr<const TableMetadata> metadata, + const std::shared_ptr<Schema>& projected_schema, int64_t batch_size) { + std::vector<size_t> snapshot_indices; + for (size_t i = 0; i < metadata->snapshots.size(); ++i) { + if (metadata->snapshots[i]) { + snapshot_indices.push_back(i); + } + } + const size_t num_rows = snapshot_indices.size(); + + return internal::MakeMetadataBatchStream( + CanonicalSchema(), projected_schema, num_rows, batch_size, + [metadata = std::move(metadata), snapshot_indices = std::move(snapshot_indices)]( + ArrowArray* column, int32_t field_id, size_t row) -> Status { + const auto& snapshot = *metadata->snapshots[snapshot_indices[row]]; + switch (field_id) { + case 1: // committed_at + return AppendInt(column, UnixMsFromTimePointMs(snapshot.timestamp_ms) * 1000); + case 2: // snapshot_id + return AppendInt(column, snapshot.snapshot_id); + case 3: // parent_id + return snapshot.parent_snapshot_id + ? AppendInt(column, *snapshot.parent_snapshot_id) + : AppendNull(column); + case 4: { // operation + auto operation = snapshot.Operation(); + return operation ? AppendString(column, *operation) : AppendNull(column); + } + case 5: // manifest_list + return snapshot.manifest_list.empty() + ? AppendNull(column) + : AppendString(column, snapshot.manifest_list); + case 6: { // summary; operation is exposed as its own column + if (snapshot.summary.empty()) { + return AppendNull(column); + } + auto summary = snapshot.summary; + summary.erase(SnapshotSummaryFields::kOperation); + return AppendStringMap(column, summary); + } + default: + return InvalidArgument("Unknown snapshots metadata column id: {}", field_id); + } + }); +} + +// SnapshotsScan implementation +SnapshotsScan::SnapshotsScan(internal::MetadataScanContext context, + std::shared_ptr<StaticMetadataTask> task) + : MetadataScan(std::move(context)), task_(std::move(task)) {} + +SnapshotsScan::~SnapshotsScan() = default; + +Result<std::unique_ptr<SnapshotsScan>> SnapshotsScan::Make( + internal::MetadataScanContext context) { + ICEBERG_PRECHECK(context.kind == MetadataTableKind::kSnapshots, + "Unexpected metadata table kind for snapshots scan"); + ICEBERG_PRECHECK(context.table_metadata != nullptr, "Table metadata cannot be null"); + ICEBERG_PRECHECK(context.schema != nullptr, "Metadata table schema cannot be null"); + ICEBERG_PRECHECK(context.projected_schema != nullptr, + "Projected schema cannot be null"); + ICEBERG_ASSIGN_OR_RAISE(auto task, + StaticMetadataTask::Make(context.kind, context.table_metadata)); Review Comment: Done. The task now keeps `projected_schema`, and the reader options only have `batch_size`. Added tests for opening projected tasks with only a batch size set. ########## src/iceberg/test/snapshots_table_test.cc: ########## @@ -21,212 +21,127 @@ #include <cstdint> #include <memory> -#include <string> #include <utility> #include <vector> -#include <arrow/array.h> -#include <arrow/c/bridge.h> -#include <arrow/record_batch.h> -#include <gmock/gmock.h> #include <gtest/gtest.h> -#include "iceberg/constants.h" -#include "iceberg/inspect/metadata_table.h" +#include "iceberg/inspect/metadata_scan_task_reader.h" +#include "iceberg/schema.h" +#include "iceberg/schema_field.h" +#include "iceberg/snapshot.h" +#include "iceberg/table_metadata.h" +#include "iceberg/test/arrow_test_util.h" #include "iceberg/test/matchers.h" #include "iceberg/test/metadata_table_test_base.h" +#include "iceberg/type.h" +#include "iceberg/util/timepoint.h" namespace iceberg { -namespace { - -std::vector<std::pair<std::string, std::string>> GetMapEntries( - const std::shared_ptr<::arrow::MapArray>& map_array, int64_t row) { - auto keys = std::static_pointer_cast<::arrow::StringArray>(map_array->keys()); - auto values = std::static_pointer_cast<::arrow::StringArray>(map_array->items()); - std::vector<std::pair<std::string, std::string>> entries; - entries.reserve(map_array->value_length(row)); - const auto offset = map_array->value_offset(row); - for (int64_t index = offset; index < offset + map_array->value_length(row); ++index) { - entries.emplace_back(keys->GetString(index), values->GetString(index)); - } - return entries; -} - -} // namespace class SnapshotsTableTest : public MetadataTableTestBase { protected: void SetUp() override { MetadataTableTestBase::SetUp(); + metadata_->snapshots = { + std::make_shared<Snapshot>( + Snapshot{.snapshot_id = 11, + .parent_snapshot_id = std::nullopt, + .timestamp_ms = TimePointMsFromUnixMs(1234), + .manifest_list = "file:/manifest11.avro", + .summary = {{"operation", "append"}, {"total-records", "7"}}}), + std::make_shared<Snapshot>(Snapshot{.snapshot_id = 12, + .parent_snapshot_id = 11, + .timestamp_ms = TimePointMsFromUnixMs(2345), + .manifest_list = "file:/manifest12.avro", + .summary = {{"operation", "delete"}}}), + std::make_shared<Snapshot>(Snapshot{.snapshot_id = 13, + .parent_snapshot_id = 12, + .timestamp_ms = TimePointMsFromUnixMs(3456), + .manifest_list = "file:/manifest13.avro"})}; + metadata_->current_snapshot_id = 13; + ICEBERG_UNWRAP_OR_FAIL(metadata_table_, + table_->Inspect(MetadataTableKind::kSnapshots)); + } - auto [snap1, snap2] = MakeTestSnapshots(); - ICEBERG_UNWRAP_OR_FAIL( - table_, MakeTableWithSnapshots({snap1, snap2}, /*current_snapshot_id=*/2)); - - ICEBERG_UNWRAP_OR_FAIL(snapshots_table_, MetadataTable::Make<SnapshotsTable>(table_)); + static std::shared_ptr<Schema> ExpectedSchema() { + return std::make_shared<Schema>(std::vector<SchemaField>{ + SchemaField::MakeRequired(1, "committed_at", timestamp_tz()), + SchemaField::MakeRequired(2, "snapshot_id", int64()), + SchemaField::MakeOptional(3, "parent_id", int64()), + SchemaField::MakeOptional(4, "operation", string()), + SchemaField::MakeOptional(5, "manifest_list", string()), + SchemaField::MakeOptional( + 6, "summary", + std::make_shared<MapType>(SchemaField::MakeRequired(7, "key", string()), + SchemaField::MakeRequired(8, "value", string())))}); } - std::unique_ptr<MetadataTable> snapshots_table_; + std::unique_ptr<MetadataTable> metadata_table_; }; -TEST_F(SnapshotsTableTest, Construct) { - EXPECT_EQ(snapshots_table_->kind(), MetadataTable::Kind::kSnapshots); - EXPECT_EQ(snapshots_table_->source_table(), table_); - EXPECT_NE(snapshots_table_->schema(), nullptr); -} - -TEST_F(SnapshotsTableTest, Scan) { - // Scan the snapshots table once and verify all columns of the result. - ICEBERG_UNWRAP_OR_FAIL(auto stream, snapshots_table_->Scan()); - ICEBERG_UNWRAP_OR_FAIL(auto batches, ReadAllBatches(std::move(stream))); - ASSERT_EQ(batches.size(), 1); - const auto& batch = batches.front(); - - // Row and column counts. - EXPECT_EQ(batch->num_rows(), 2); - EXPECT_EQ(batch->num_columns(), 6); - - // Column 0: committed_at (timestamptz) — microseconds since epoch. - auto committed_at = std::static_pointer_cast<::arrow::TimestampArray>(batch->column(0)); - EXPECT_EQ(committed_at->Value(0), 1234567890000LL * 1000); - EXPECT_EQ(committed_at->Value(1), 9876543210000LL * 1000); - - // Column 1: snapshot_id (long) — returned in storage order. - auto snapshot_ids = std::static_pointer_cast<::arrow::Int64Array>(batch->column(1)); - EXPECT_EQ(snapshot_ids->Value(0), 1); - EXPECT_EQ(snapshot_ids->Value(1), 2); - - // Column 2: parent_id (long) — first snapshot has no parent. - auto parent_ids = std::static_pointer_cast<::arrow::Int64Array>(batch->column(2)); - EXPECT_TRUE(parent_ids->IsNull(0)); - EXPECT_FALSE(parent_ids->IsNull(1)); - EXPECT_EQ(parent_ids->Value(1), 1); - - // Column 3: operation (string). - auto operations = std::static_pointer_cast<::arrow::StringArray>(batch->column(3)); - EXPECT_EQ(operations->GetString(0), "append"); - EXPECT_EQ(operations->GetString(1), "append"); - - // Column 4: manifest_list (string). - auto manifest_lists = std::static_pointer_cast<::arrow::StringArray>(batch->column(4)); - EXPECT_EQ(manifest_lists->GetString(0), "file:/tmp/manifest1.avro"); - EXPECT_EQ(manifest_lists->GetString(1), "file:/tmp/manifest2.avro"); - - // Column 5: summary (map<string,string>) excludes the separate operation field. - auto summaries = std::static_pointer_cast<::arrow::MapArray>(batch->column(5)); - EXPECT_FALSE(summaries->IsNull(0)); - EXPECT_FALSE(summaries->IsNull(1)); - EXPECT_EQ(summaries->value_length(0), 10); - EXPECT_EQ(summaries->value_length(1), 10); - - auto first_summary = GetMapEntries(summaries, 0); - EXPECT_THAT( - first_summary, - ::testing::Not(::testing::Contains(::testing::Pair("operation", "append")))); - EXPECT_THAT(first_summary, ::testing::Contains(::testing::Pair("total-records", "1"))); - - auto second_summary = GetMapEntries(summaries, 1); - EXPECT_THAT( - second_summary, - ::testing::Not(::testing::Contains(::testing::Pair("operation", "append")))); - EXPECT_THAT(second_summary, ::testing::Contains(::testing::Pair("total-records", "2"))); +TEST_F(SnapshotsTableTest, CanonicalSchema) { + const auto expected = ExpectedSchema(); + EXPECT_EQ(*SnapshotsTable::CanonicalSchema(), *expected); + EXPECT_EQ(*metadata_table_->schema(), *expected); } -TEST_F(SnapshotsTableTest, ScanEmptySnapshotList) { Review Comment: The old snapshot cases are back, including empty input, null entries, batch boundaries, and refresh during a scan. I also kept all cases from #1008 after rebasing. ########## src/iceberg/inspect/metadata_scan_task_reader.cc: ########## @@ -0,0 +1,64 @@ +// 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/metadata_scan_task_reader.h" + +#include <memory> +#include <utility> + +#include "iceberg/inspect/history_table.h" +#include "iceberg/inspect/metadata_scan_task.h" +#include "iceberg/inspect/metadata_table.h" +#include "iceberg/inspect/snapshots_table.h" +#include "iceberg/util/checked_cast.h" +#include "iceberg/util/macros.h" + +namespace iceberg { + +MetadataScanTaskReader::MetadataScanTaskReader(Options options) + : options_(std::move(options)) {} + +MetadataScanTaskReader::~MetadataScanTaskReader() = default; + +Result<std::unique_ptr<MetadataScanTaskReader>> MetadataScanTaskReader::Make( + Options options) { + ICEBERG_PRECHECK(options.batch_size > 0, "batch size must be positive"); + return std::unique_ptr<MetadataScanTaskReader>( + new MetadataScanTaskReader(std::move(options))); +} + +Result<ArrowArrayStream> MetadataScanTaskReader::Open( + const MetadataScanTask& task) const { + switch (task.task_kind()) { + case MetadataScanTask::TaskKind::kStatic: { + const auto& static_task = internal::checked_cast<const StaticMetadataTask&>(task); Review Comment: I'd prefer to keep the reader separate for now, like `FileScanTaskReader`. Tasks now keep the projection, and each `Open()` creates its own read state. We can revisit the dispatch when we add more metadata tables. ########## src/iceberg/inspect/history_table.cc: ########## @@ -45,13 +56,94 @@ const std::shared_ptr<Schema>& HistoryTable::schema() const { return schema; } -Result<std::unique_ptr<HistoryTable>> HistoryTable::Make(std::shared_ptr<Table> table) { +Result<std::unique_ptr<HistoryTable>> HistoryTable::Make( + std::shared_ptr<const Table> table) { ICEBERG_PRECHECK(table != nullptr, "Table cannot be null"); return std::unique_ptr<HistoryTable>(new HistoryTable(std::move(table))); } -Result<ArrowArrayStream> HistoryTable::Scan() { - return NotSupported("Scan is not supported for the history table"); +Result<ArrowArrayStream> HistoryTable::ReadRows( + std::shared_ptr<const TableMetadata> metadata, + const std::shared_ptr<Schema>& projected_schema, int64_t batch_size) { + std::unordered_map<int64_t, const Snapshot*> snapshots_by_id; + for (const auto& snapshot : metadata->snapshots) { + if (snapshot) { + snapshots_by_id.emplace(snapshot->snapshot_id, snapshot.get()); + } + } + + // Walk the current snapshot's ancestry; the insert check stops on cycles. + std::unordered_set<int64_t> ancestors; + for (auto current = metadata->current_snapshot_id; + current != kInvalidSnapshotId && ancestors.insert(current).second;) { + auto it = snapshots_by_id.find(current); + if (it == snapshots_by_id.end() || !it->second->parent_snapshot_id) { + break; + } + current = *it->second->parent_snapshot_id; + } + + const size_t num_rows = metadata->snapshot_log.size(); + return internal::MakeMetadataBatchStream( + CanonicalSchema(), projected_schema, num_rows, batch_size, + [metadata = std::move(metadata), snapshots_by_id = std::move(snapshots_by_id), + ancestors = std::move(ancestors)](ArrowArray* column, int32_t field_id, + size_t row) -> Status { + const auto& entry = metadata->snapshot_log[row]; + switch (field_id) { + case 1: // made_current_at + return AppendInt(column, UnixMsFromTimePointMs(entry.timestamp_ms) * 1000); + case 2: // snapshot_id + return AppendInt(column, entry.snapshot_id); + case 3: { // parent_id; null when the snapshot has been expired + auto it = snapshots_by_id.find(entry.snapshot_id); + return it != snapshots_by_id.end() && it->second->parent_snapshot_id + ? AppendInt(column, *it->second->parent_snapshot_id) + : AppendNull(column); + } + case 4: // is_current_ancestor + return AppendBoolean(column, ancestors.contains(entry.snapshot_id)); + default: + return InvalidArgument("Unknown history metadata column id: {}", field_id); + } + }); +} + +// HistoryScan implementation +HistoryScan::HistoryScan(internal::MetadataScanContext context, + std::shared_ptr<StaticMetadataTask> task) + : MetadataScan(std::move(context)), task_(std::move(task)) {} + +HistoryScan::~HistoryScan() = default; + +Result<std::unique_ptr<HistoryScan>> HistoryScan::Make( + internal::MetadataScanContext context) { + ICEBERG_PRECHECK(context.kind == MetadataTableKind::kHistory, + "Unexpected metadata table kind for history scan"); + ICEBERG_PRECHECK(context.table_metadata != nullptr, "Table metadata cannot be null"); + ICEBERG_PRECHECK(context.schema != nullptr, "Metadata table schema cannot be null"); + ICEBERG_PRECHECK(context.projected_schema != nullptr, + "Projected schema cannot be null"); + ICEBERG_ASSIGN_OR_RAISE(auto task, + StaticMetadataTask::Make(context.kind, context.table_metadata)); + return std::unique_ptr<HistoryScan>( + new HistoryScan(std::move(context), std::move(task))); +} + +Result<MetadataScanTaskStreamPtr> HistoryScan::PlanTasksStream() const { Review Comment: Both scans now use the base `PlanTasks()` and `ToArrowArrayStream()` methods. Only `PlanTasksStream()` stays in each scan. ########## src/iceberg/inspect/metadata_scan_task.cc: ########## @@ -0,0 +1,44 @@ +// Licensed to the Apache Software Foundation (ASF) under one Review Comment: Done. All four task/reader files now use `/* ... */` license headers. ########## src/iceberg/inspect/history_table.cc: ########## @@ -45,13 +56,94 @@ const std::shared_ptr<Schema>& HistoryTable::schema() const { return schema; } -Result<std::unique_ptr<HistoryTable>> HistoryTable::Make(std::shared_ptr<Table> table) { +Result<std::unique_ptr<HistoryTable>> HistoryTable::Make( + std::shared_ptr<const Table> table) { ICEBERG_PRECHECK(table != nullptr, "Table cannot be null"); return std::unique_ptr<HistoryTable>(new HistoryTable(std::move(table))); } -Result<ArrowArrayStream> HistoryTable::Scan() { - return NotSupported("Scan is not supported for the history table"); +Result<ArrowArrayStream> HistoryTable::ReadRows( + std::shared_ptr<const TableMetadata> metadata, + const std::shared_ptr<Schema>& projected_schema, int64_t batch_size) { + std::unordered_map<int64_t, const Snapshot*> snapshots_by_id; + for (const auto& snapshot : metadata->snapshots) { + if (snapshot) { + snapshots_by_id.emplace(snapshot->snapshot_id, snapshot.get()); + } + } + + // Walk the current snapshot's ancestry; the insert check stops on cycles. + std::unordered_set<int64_t> ancestors; + for (auto current = metadata->current_snapshot_id; + current != kInvalidSnapshotId && ancestors.insert(current).second;) { Review Comment: Added `CyclicAncestryTerminates` with a two-snapshot cycle. It checks the output and makes sure the scan finishes. -- 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]
