From 81b1ce630d7f9bb488a68e4b838fbbd51889200f Mon Sep 17 00:00:00 2001 From: Sandeep Gottimukkala Date: Thu, 25 Jun 2026 23:35:59 -0700 Subject: [PATCH 01/16] feat(rest): add scan plan endpoint support to REST catalog client When a table is loaded from a REST catalog that advertises the PlanTableScan endpoint, NewScan() now returns a RestTableScanBuilder whose Build() produces a RestTableScan. PlanFiles() on that scan delegates manifest resolution to the server via POST /plan, GET /plan/{id} (with exponential backoff), POST /tasks/{id}, and DELETE /plan/{id} (best-effort cancel), instead of reading manifests locally. - Add RestTable, RestTableScanBuilder, RestTableScan and RestScanContext - Promote DataTableScan::PlanFiles and TableScanBuilder::Build to virtual - Convert RestCatalog::client_ and paths_ to shared_ptr so RestScanContext can share ownership with live scans --- src/iceberg/catalog/rest/CMakeLists.txt | 2 + src/iceberg/catalog/rest/rest_catalog.cc | 16 +- src/iceberg/catalog/rest/rest_catalog.h | 4 +- src/iceberg/catalog/rest/rest_table.cc | 60 +++++ src/iceberg/catalog/rest/rest_table.h | 61 +++++ src/iceberg/catalog/rest/rest_table_scan.cc | 269 ++++++++++++++++++++ src/iceberg/catalog/rest/rest_table_scan.h | 116 +++++++++ src/iceberg/table_scan.h | 4 +- 8 files changed, 527 insertions(+), 5 deletions(-) create mode 100644 src/iceberg/catalog/rest/rest_table.cc create mode 100644 src/iceberg/catalog/rest/rest_table.h create mode 100644 src/iceberg/catalog/rest/rest_table_scan.cc create mode 100644 src/iceberg/catalog/rest/rest_table_scan.h diff --git a/src/iceberg/catalog/rest/CMakeLists.txt b/src/iceberg/catalog/rest/CMakeLists.txt index f64860ff4..0736290bc 100644 --- a/src/iceberg/catalog/rest/CMakeLists.txt +++ b/src/iceberg/catalog/rest/CMakeLists.txt @@ -34,6 +34,8 @@ set(ICEBERG_REST_SOURCES rest_catalog.cc rest_file_io.cc rest_metrics_reporter.cc + rest_table.cc + rest_table_scan.cc rest_util.cc types.cc) diff --git a/src/iceberg/catalog/rest/rest_catalog.cc b/src/iceberg/catalog/rest/rest_catalog.cc index 4a4f990ea..147404636 100644 --- a/src/iceberg/catalog/rest/rest_catalog.cc +++ b/src/iceberg/catalog/rest/rest_catalog.cc @@ -39,6 +39,7 @@ #include "iceberg/catalog/rest/resource_paths.h" #include "iceberg/catalog/rest/rest_file_io.h" #include "iceberg/catalog/rest/rest_metrics_reporter_internal.h" +#include "iceberg/catalog/rest/rest_table.h" #include "iceberg/catalog/rest/rest_util.h" #include "iceberg/catalog/rest/types.h" #include "iceberg/json_serde_internal.h" @@ -454,7 +455,7 @@ Result> RestCatalog::Make( RestCatalog::RestCatalog(RestCatalogProperties config, std::shared_ptr file_io, std::shared_ptr client, - std::unique_ptr paths, + std::shared_ptr paths, std::unordered_set endpoints, std::unique_ptr auth_manager, std::shared_ptr catalog_session, @@ -899,6 +900,19 @@ Result> RestCatalog::MakeTableFromLoadResult( auto table_catalog = std::make_shared( shared_from_this(), context, identifier, table_config, table_session, table_io); + if (supported_endpoints_.contains(Endpoint::PlanTableScan())) { + RestScanContext rest_ctx{ + .client = client_, + .paths = paths_, + .session = table_session, + .supported_endpoints = supported_endpoints_, + .identifier = identifier, + }; + return RestTable::Make(identifier, std::move(result.metadata), + std::move(result.metadata_location), std::move(table_io), + std::move(table_catalog), std::move(rest_ctx)); + } + return Table::Make(identifier, std::move(result.metadata), std::move(result.metadata_location), std::move(table_io), std::move(table_catalog), RestTableName(name_, identifier), diff --git a/src/iceberg/catalog/rest/rest_catalog.h b/src/iceberg/catalog/rest/rest_catalog.h index 65b0b5eab..8b194b668 100644 --- a/src/iceberg/catalog/rest/rest_catalog.h +++ b/src/iceberg/catalog/rest/rest_catalog.h @@ -69,7 +69,7 @@ class ICEBERG_REST_EXPORT RestCatalog final class TableScopedCatalog; RestCatalog(RestCatalogProperties config, std::shared_ptr file_io, - std::shared_ptr client, std::unique_ptr paths, + std::shared_ptr client, std::shared_ptr paths, std::unordered_set endpoints, std::unique_ptr auth_manager, std::shared_ptr catalog_session, @@ -193,7 +193,7 @@ class ICEBERG_REST_EXPORT RestCatalog final RestCatalogProperties config_; std::shared_ptr file_io_; std::shared_ptr client_; - std::unique_ptr paths_; + std::shared_ptr paths_; std::string name_; std::unordered_set supported_endpoints_; std::unique_ptr auth_manager_; diff --git a/src/iceberg/catalog/rest/rest_table.cc b/src/iceberg/catalog/rest/rest_table.cc new file mode 100644 index 000000000..00f8856d1 --- /dev/null +++ b/src/iceberg/catalog/rest/rest_table.cc @@ -0,0 +1,60 @@ +/* + * 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/catalog/rest/rest_table.h" + +#include +#include + +#include "iceberg/catalog/rest/rest_table_scan.h" +#include "iceberg/result.h" +#include "iceberg/table_metadata.h" +#include "iceberg/util/macros.h" + +namespace iceberg::rest { + +RestTable::RestTable(TableIdentifier identifier, std::shared_ptr metadata, + std::string metadata_location, std::shared_ptr io, + std::shared_ptr catalog, RestScanContext rest_context) + : Table(std::move(identifier), std::move(metadata), std::move(metadata_location), + std::move(io), std::move(catalog)), + rest_context_(std::move(rest_context)) {} + +RestTable::~RestTable() = default; + +Result> RestTable::Make(TableIdentifier identifier, + std::shared_ptr metadata, + std::string metadata_location, + std::shared_ptr io, + std::shared_ptr catalog, + RestScanContext rest_context) { + if (metadata == nullptr) { + return InvalidArgument("Metadata cannot be null"); + } + return std::shared_ptr( + new RestTable(std::move(identifier), std::move(metadata), + std::move(metadata_location), std::move(io), std::move(catalog), + std::move(rest_context))); +} + +Result> RestTable::NewScan() const { + return std::make_unique(metadata_, io_, rest_context_); +} + +} // namespace iceberg::rest diff --git a/src/iceberg/catalog/rest/rest_table.h b/src/iceberg/catalog/rest/rest_table.h new file mode 100644 index 000000000..8e9172b09 --- /dev/null +++ b/src/iceberg/catalog/rest/rest_table.h @@ -0,0 +1,61 @@ +/* + * 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 +#include + +#include "iceberg/catalog/rest/iceberg_rest_export.h" +#include "iceberg/catalog/rest/rest_table_scan.h" +#include "iceberg/result.h" +#include "iceberg/table.h" +#include "iceberg/type_fwd.h" + +/// \file iceberg/catalog/rest/rest_table.h +/// A Table subclass that uses server-side distributed scan planning via the REST catalog. + +namespace iceberg::rest { + +/// \brief A Table whose NewScan() returns a RestTableScanBuilder, delegating +/// PlanFiles() to the REST catalog server's scan planning endpoints. +class ICEBERG_REST_EXPORT RestTable final : public Table { + public: + static Result> Make(TableIdentifier identifier, + std::shared_ptr metadata, + std::string metadata_location, + std::shared_ptr io, + std::shared_ptr catalog, + RestScanContext rest_context); + + ~RestTable() override; + + /// \brief Returns a RestTableScanBuilder that will delegate PlanFiles() to the + /// REST catalog server. + Result> NewScan() const override; + + private: + RestTable(TableIdentifier identifier, std::shared_ptr metadata, + std::string metadata_location, std::shared_ptr io, + std::shared_ptr catalog, RestScanContext rest_context); + + RestScanContext rest_context_; +}; + +} // namespace iceberg::rest diff --git a/src/iceberg/catalog/rest/rest_table_scan.cc b/src/iceberg/catalog/rest/rest_table_scan.cc new file mode 100644 index 000000000..30dcc95df --- /dev/null +++ b/src/iceberg/catalog/rest/rest_table_scan.cc @@ -0,0 +1,269 @@ +/* + * 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/catalog/rest/rest_table_scan.h" + +#include +#include + +#include + +#include "iceberg/catalog/rest/endpoint.h" +#include "iceberg/catalog/rest/error_handlers.h" +#include "iceberg/catalog/rest/http_client.h" +#include "iceberg/catalog/rest/json_serde_internal.h" +#include "iceberg/catalog/rest/resource_paths.h" +#include "iceberg/catalog/rest/types.h" +#include "iceberg/json_serde_internal.h" +#include "iceberg/partition_spec.h" +#include "iceberg/result.h" +#include "iceberg/schema.h" +#include "iceberg/table_metadata.h" +#include "iceberg/util/macros.h" + +namespace iceberg::rest { + +namespace { + +constexpr int64_t kMinSleepMs = 1'000; +constexpr int64_t kMaxSleepMs = 60'000; +constexpr int kMaxRetries = 10; +constexpr int64_t kMaxWaitTimeMs = 5 * 60 * 1'000; + +#define ICEBERG_ENDPOINT_CHECK(endpoints, endpoint) \ + do { \ + if (!endpoints.contains(endpoint)) { \ + return NotSupported("Not supported endpoint: {}", endpoint.ToString()); \ + } \ + } while (0) + +} // namespace + +// RestTableScan + +RestTableScan::RestTableScan(std::shared_ptr metadata, + std::shared_ptr schema, std::shared_ptr io, + internal::TableScanContext context, + RestScanContext rest_context) + : DataTableScan(std::move(metadata), std::move(schema), std::move(io), + std::move(context)), + rest_context_(std::move(rest_context)) {} + +Result> RestTableScan::Make( + std::shared_ptr metadata, std::shared_ptr schema, + std::shared_ptr io, internal::TableScanContext context, + RestScanContext rest_context) { + ICEBERG_PRECHECK(metadata != nullptr, "Table metadata cannot be null"); + ICEBERG_PRECHECK(schema != nullptr, "Schema cannot be null"); + ICEBERG_PRECHECK(io != nullptr, "FileIO cannot be null"); + return std::unique_ptr( + new RestTableScan(std::move(metadata), std::move(schema), std::move(io), + std::move(context), std::move(rest_context))); +} + +Result>> RestTableScan::PlanFiles() const { + TableMetadataCache metadata_cache(metadata_.get()); + ICEBERG_ASSIGN_OR_RAISE(auto specs_by_id, metadata_cache.GetPartitionSpecsById()); + + std::string plan_id; + return PlanTableScan(plan_id, specs_by_id); +} + +Result>> RestTableScan::PlanTableScan( + std::string& plan_id, + const std::unordered_map>& specs) const { + ICEBERG_ENDPOINT_CHECK(rest_context_.supported_endpoints, Endpoint::PlanTableScan()); + + // Build request from scan context + PlanTableScanRequest request; + request.select = context_.selected_columns; + request.filter = context_.filter; + request.case_sensitive = context_.case_sensitive; + request.min_rows_requested = context_.min_rows_requested; + + if (context_.from_snapshot_id.has_value() && context_.to_snapshot_id.has_value()) { + request.start_snapshot_id = context_.from_snapshot_id; + request.end_snapshot_id = context_.to_snapshot_id; + } else if (context_.snapshot_id.has_value()) { + request.snapshot_id = context_.snapshot_id; + } + + if (!context_.columns_to_keep_stats.empty()) { + for (int32_t field_id : context_.columns_to_keep_stats) { + ICEBERG_ASSIGN_OR_RAISE(auto name, schema_->FindColumnNameById(field_id)); + if (name.has_value()) { + request.stats_fields.emplace_back(*name); + } + } + } + + ICEBERG_ASSIGN_OR_RAISE(auto path, + rest_context_.paths->Plan(rest_context_.identifier)); + ICEBERG_ASSIGN_OR_RAISE(auto request_json, ToJson(request)); + ICEBERG_ASSIGN_OR_RAISE(auto json_request, ToJsonString(request_json)); + ICEBERG_ASSIGN_OR_RAISE( + const auto response, + rest_context_.client->Post(path, json_request, /*headers=*/{}, + *PlanErrorHandler::Instance(), *rest_context_.session)); + ICEBERG_ASSIGN_OR_RAISE(auto json, FromJsonString(response.body())); + ICEBERG_ASSIGN_OR_RAISE(auto result, + PlanTableScanResponseFromJson(json, specs, *schema_)); + ICEBERG_RETURN_UNEXPECTED(result.Validate()); + + plan_id = result.plan_id; + + switch (result.plan_status) { + case PlanStatus::kCompleted: + return ResolveScanTasks(result.plan_tasks, result.file_scan_tasks, specs); + case PlanStatus::kSubmitted: + return FetchPlanningResult(plan_id, specs); + case PlanStatus::kFailed: + return IOError("Scan planning failed: {}", + result.error ? result.error->message : "unknown error"); + case PlanStatus::kCancelled: + return IOError("Scan planning was cancelled for plan_id={}", plan_id); + } + return IOError("Unexpected plan status"); +} + +Result>> RestTableScan::FetchPlanningResult( + const std::string& plan_id, + const std::unordered_map>& specs) const { + ICEBERG_ENDPOINT_CHECK(rest_context_.supported_endpoints, + Endpoint::FetchPlanningResult()); + + ICEBERG_ASSIGN_OR_RAISE(auto path, + rest_context_.paths->Plan(rest_context_.identifier, plan_id)); + + auto delay_ms = kMinSleepMs; + auto start = std::chrono::steady_clock::now(); + + for (int retry = 0; retry <= kMaxRetries; ++retry) { + ICEBERG_ASSIGN_OR_RAISE( + const auto response, + rest_context_.client->Get(path, /*params=*/{}, /*headers=*/{}, + *PlanErrorHandler::Instance(), *rest_context_.session)); + ICEBERG_ASSIGN_OR_RAISE(auto json, FromJsonString(response.body())); + ICEBERG_ASSIGN_OR_RAISE(auto result, + FetchPlanningResultResponseFromJson(json, specs, *schema_)); + ICEBERG_RETURN_UNEXPECTED(result.Validate()); + + switch (result.plan_status) { + case PlanStatus::kCompleted: + return ResolveScanTasks(result.plan_tasks, result.file_scan_tasks, specs); + case PlanStatus::kSubmitted: { + auto elapsed_ms = std::chrono::duration_cast( + std::chrono::steady_clock::now() - start) + .count(); + if (elapsed_ms >= kMaxWaitTimeMs) { + CancelPlanning(plan_id); + return IOError( + "Scan planning timed out after {}ms waiting for plan_id={}", elapsed_ms, + plan_id); + } + std::this_thread::sleep_for(std::chrono::milliseconds(delay_ms)); + delay_ms = std::min(delay_ms * 2, kMaxSleepMs); + continue; + } + case PlanStatus::kFailed: + CancelPlanning(plan_id); + return IOError("Scan planning failed: {}", + result.error ? result.error->message : "unknown error"); + case PlanStatus::kCancelled: + return IOError("Scan planning was cancelled for plan_id={}", plan_id); + } + } + + CancelPlanning(plan_id); + return IOError("Scan planning exceeded max retries ({}) for plan_id={}", + kMaxRetries, plan_id); +} + +Result>> RestTableScan::FetchScanTasks( + const std::string& plan_task, + const std::unordered_map>& specs) const { + ICEBERG_ENDPOINT_CHECK(rest_context_.supported_endpoints, Endpoint::FetchScanTasks()); + + ICEBERG_ASSIGN_OR_RAISE(auto path, + rest_context_.paths->FetchScanTasks(rest_context_.identifier)); + FetchScanTasksRequest request{.planTask = plan_task}; + ICEBERG_ASSIGN_OR_RAISE(auto json_request, ToJsonString(ToJson(request))); + ICEBERG_ASSIGN_OR_RAISE( + const auto response, + rest_context_.client->Post(path, json_request, /*headers=*/{}, + *PlanTaskErrorHandler::Instance(), + *rest_context_.session)); + ICEBERG_ASSIGN_OR_RAISE(auto json, FromJsonString(response.body())); + ICEBERG_ASSIGN_OR_RAISE(auto result, + FetchScanTasksResponseFromJson(json, specs, *schema_)); + ICEBERG_RETURN_UNEXPECTED(result.Validate()); + + return ResolveScanTasks(result.plan_tasks, result.file_scan_tasks, specs); +} + +Result>> RestTableScan::ResolveScanTasks( + const std::optional>& plan_tasks, + const std::optional>>& file_scan_tasks, + const std::unordered_map>& specs) const { + std::vector> result; + + if (file_scan_tasks.has_value()) { + result.insert(result.end(), file_scan_tasks->begin(), file_scan_tasks->end()); + } + + if (plan_tasks.has_value()) { + for (const auto& plan_task : *plan_tasks) { + ICEBERG_ASSIGN_OR_RAISE(auto tasks, FetchScanTasks(plan_task, specs)); + result.insert(result.end(), tasks.begin(), tasks.end()); + } + } + + return result; +} + +void RestTableScan::CancelPlanning(const std::string& plan_id) const { + if (plan_id.empty()) return; + if (!rest_context_.supported_endpoints.contains(Endpoint::CancelPlanning())) return; + + auto path = rest_context_.paths->Plan(rest_context_.identifier, plan_id); + if (!path.has_value()) return; + + // Best-effort: ignore errors. + std::ignore = rest_context_.client->Delete(*path, /*params=*/{}, /*headers=*/{}, + *PlanErrorHandler::Instance(), + *rest_context_.session); +} + +// RestTableScanBuilder + +RestTableScanBuilder::RestTableScanBuilder(std::shared_ptr metadata, + std::shared_ptr io, + RestScanContext rest_context) + : DataTableScanBuilder(std::move(metadata), std::move(io)), + rest_context_(std::move(rest_context)) {} + +Result> RestTableScanBuilder::Build() { + ICEBERG_RETURN_UNEXPECTED(CheckErrors()); + ICEBERG_RETURN_UNEXPECTED(context_.Validate()); + ICEBERG_ASSIGN_OR_RAISE(auto schema, ResolveSnapshotSchema()); + return RestTableScan::Make(metadata_, schema.get(), io_, std::move(context_), + rest_context_); +} + +} // namespace iceberg::rest diff --git a/src/iceberg/catalog/rest/rest_table_scan.h b/src/iceberg/catalog/rest/rest_table_scan.h new file mode 100644 index 000000000..149e8de91 --- /dev/null +++ b/src/iceberg/catalog/rest/rest_table_scan.h @@ -0,0 +1,116 @@ +/* + * 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 +#include +#include +#include +#include + +#include "iceberg/catalog/rest/endpoint.h" +#include "iceberg/catalog/rest/iceberg_rest_export.h" +#include "iceberg/result.h" +#include "iceberg/table_identifier.h" +#include "iceberg/table_scan.h" +#include "iceberg/type_fwd.h" + +/// \file iceberg/catalog/rest/rest_table_scan.h +/// REST-specific table scan that delegates scan planning to the REST catalog server. + +namespace iceberg::rest { + +class HttpClient; +class ResourcePaths; + +namespace auth { +class AuthSession; +} // namespace auth + +/// \brief HTTP context shared between RestTable and RestTableScan. +struct ICEBERG_REST_EXPORT RestScanContext { + std::shared_ptr client; + std::shared_ptr paths; + std::shared_ptr session; + std::unordered_set supported_endpoints; + TableIdentifier identifier; +}; + +/// \brief A DataTableScan that delegates PlanFiles() to the REST catalog server +/// via the scan planning endpoints (planTableScan / fetchPlanningResult / +/// cancelPlanning / fetchScanTasks). +class ICEBERG_REST_EXPORT RestTableScan : public DataTableScan { + public: + ~RestTableScan() override = default; + + static Result> Make( + std::shared_ptr metadata, std::shared_ptr schema, + std::shared_ptr io, internal::TableScanContext context, + RestScanContext rest_context); + + /// \brief Plans files via the REST scan planning endpoints. + Result>> PlanFiles() const override; + + private: + RestTableScan(std::shared_ptr metadata, std::shared_ptr schema, + std::shared_ptr io, internal::TableScanContext context, + RestScanContext rest_context); + + /// POST /plan → handle COMPLETED / SUBMITTED / FAILED / CANCELLED. + Result>> PlanTableScan( + std::string& plan_id, + const std::unordered_map>& specs) const; + + /// GET /plan/{plan_id} with exponential backoff until COMPLETED. + Result>> FetchPlanningResult( + const std::string& plan_id, + const std::unordered_map>& specs) const; + + /// POST /tasks/{plan_task_id} → fetch FileScanTasks for one opaque plan task token. + Result>> FetchScanTasks( + const std::string& plan_task, + const std::unordered_map>& specs) const; + + /// Flatten plan_tasks (opaque tokens) + file_scan_tasks into a single list. + Result>> ResolveScanTasks( + const std::optional>& plan_tasks, + const std::optional>>& file_scan_tasks, + const std::unordered_map>& specs) const; + + /// DELETE /plan/{plan_id}; best-effort, errors are silently ignored. + void CancelPlanning(const std::string& plan_id) const; + + RestScanContext rest_context_; +}; + +/// \brief Builder that produces a RestTableScan with the REST HTTP context injected. +class ICEBERG_REST_EXPORT RestTableScanBuilder : public DataTableScanBuilder { + public: + RestTableScanBuilder(std::shared_ptr metadata, std::shared_ptr io, + RestScanContext rest_context); + + /// \brief Resolves schema/context via parent logic then creates a RestTableScan. + Result> Build() override; + + private: + RestScanContext rest_context_; +}; + +} // namespace iceberg::rest diff --git a/src/iceberg/table_scan.h b/src/iceberg/table_scan.h index 7310d435b..f93fe5098 100644 --- a/src/iceberg/table_scan.h +++ b/src/iceberg/table_scan.h @@ -402,7 +402,7 @@ class ICEBERG_TEMPLATE_CLASS_EXPORT TableScanBuilder : public ErrorCollector { /// \brief Builds and returns a TableScan instance. /// \return A Result containing the TableScan or an error. - Result> Build(); + virtual Result> Build(); protected: TableScanBuilder(std::shared_ptr metadata, std::shared_ptr io, @@ -476,7 +476,7 @@ class ICEBERG_EXPORT DataTableScan : public TableScan { /// /// Collects PlanFilesStream() into a vector. /// \return A Result containing scan tasks or an error. - Result>> PlanFiles() const; + virtual Result>> PlanFiles() const; /// \brief Lazily plans scan tasks by resolving manifests and data files on demand. /// From 5f816605027e3d1267c64071471c9f37b0fa5861 Mon Sep 17 00:00:00 2001 From: Sandeep Gottimukkala Date: Sun, 13 Sep 2026 11:52:49 -0700 Subject: [PATCH 02/16] Address review comments and add tests for scan plan endpoints - Cancel server-side plan when ResolveScanTasks fails partway through - Propagate use_snapshot_schema from scan context to PlanTableScanRequest: true for UseSnapshot/AsOfTime/tag refs and incremental scans, false for branch refs and default scans - Gate RestTable creation on effective scan-planning-mode config (table config overrides client config, default is client); error if server mode is requested but endpoint is not advertised - Add ScanPlanningMode enum and ScanPlanningModeFrom() parser to RestCatalogProperties - Make HttpClient methods virtual and add HttpResponse::MakeForTesting() to support unit test mocking - Add tests: use_snapshot_schema in table_scan_test, ScanPlanningModeFrom parsing in catalog_properties_test, and RestTableScan HTTP flow tests in rest_table_scan_test --- .../catalog/rest/catalog_properties.cc | 11 + src/iceberg/catalog/rest/catalog_properties.h | 10 + src/iceberg/catalog/rest/http_client.cc | 9 + src/iceberg/catalog/rest/http_client.h | 42 +- src/iceberg/catalog/rest/rest_catalog.cc | 29 +- src/iceberg/catalog/rest/rest_table_scan.cc | 16 +- src/iceberg/table_scan.cc | 3 + src/iceberg/table_scan.h | 1 + src/iceberg/test/CMakeLists.txt | 2 + src/iceberg/test/catalog_properties_test.cc | 81 ++++ src/iceberg/test/rest_table_scan_test.cc | 403 ++++++++++++++++++ src/iceberg/test/table_scan_test.cc | 39 ++ 12 files changed, 622 insertions(+), 24 deletions(-) create mode 100644 src/iceberg/test/catalog_properties_test.cc create mode 100644 src/iceberg/test/rest_table_scan_test.cc diff --git a/src/iceberg/catalog/rest/catalog_properties.cc b/src/iceberg/catalog/rest/catalog_properties.cc index 0e417e6c3..91d72e8a4 100644 --- a/src/iceberg/catalog/rest/catalog_properties.cc +++ b/src/iceberg/catalog/rest/catalog_properties.cc @@ -20,6 +20,7 @@ #include "iceberg/catalog/rest/catalog_properties.h" #include +#include #include #include @@ -61,4 +62,14 @@ Result RestCatalogProperties::SnapshotLoadingMode() const { } } +Result> RestCatalogProperties::ScanPlanningModeFrom( + const std::unordered_map& config) { + auto it = config.find(kScanPlanningMode.key()); + if (it == config.end()) return std::nullopt; + std::string lower = StringUtils::ToLower(it->second); + if (lower == "client") return ScanPlanningMode::kClient; + if (lower == "server") return ScanPlanningMode::kServer; + return InvalidArgument("Invalid scan planning mode: '{}'.", it->second); +} + } // namespace iceberg::rest diff --git a/src/iceberg/catalog/rest/catalog_properties.h b/src/iceberg/catalog/rest/catalog_properties.h index d1ee0e9c4..63dcd9a95 100644 --- a/src/iceberg/catalog/rest/catalog_properties.h +++ b/src/iceberg/catalog/rest/catalog_properties.h @@ -34,6 +34,9 @@ namespace iceberg::rest { /// \brief Snapshot loading mode for REST catalog. enum class SnapshotMode : uint8_t { kAll, kRefs }; +/// \brief Scan planning mode for REST catalog. +enum class ScanPlanningMode : uint8_t { kClient, kServer }; + /// \brief Configuration class for a REST Catalog. class ICEBERG_REST_EXPORT RestCatalogProperties : public ConfigBase { @@ -58,6 +61,8 @@ class ICEBERG_REST_EXPORT RestCatalogProperties /// \brief Whether to report metrics to the REST catalog server (default: true). inline static Entry kMetricsReportingEnabled{ "rest-metrics-reporting-enabled", "true"}; + /// \brief The scan planning mode (client or server). + inline static Entry kScanPlanningMode{"scan-planning-mode", "client"}; /// \brief The prefix for HTTP headers. inline static constexpr std::string_view kHeaderPrefix = "header."; @@ -80,6 +85,11 @@ class ICEBERG_REST_EXPORT RestCatalogProperties /// "REFS", or an error if the value is invalid. Parsing is /// case-insensitive to match Java behavior. Result SnapshotLoadingMode() const; + + /// \brief Get the scan planning mode from the given config map, returning + /// std::nullopt if the key is absent. + static Result> ScanPlanningModeFrom( + const std::unordered_map& config); }; } // namespace iceberg::rest diff --git a/src/iceberg/catalog/rest/http_client.cc b/src/iceberg/catalog/rest/http_client.cc index 6661c5098..ec870b87d 100644 --- a/src/iceberg/catalog/rest/http_client.cc +++ b/src/iceberg/catalog/rest/http_client.cc @@ -62,6 +62,15 @@ std::unordered_map HttpResponse::headers() const { return impl_->headers(); } +HttpResponse HttpResponse::MakeForTesting(int32_t status_code, std::string body) { + cpr::Response cpr_response; + cpr_response.status_code = status_code; + cpr_response.text = std::move(body); + HttpResponse response; + response.impl_ = std::make_unique(std::move(cpr_response)); + return response; +} + namespace { /// \brief Default error type for unparseable REST responses. diff --git a/src/iceberg/catalog/rest/http_client.h b/src/iceberg/catalog/rest/http_client.h index ea9c10a39..52522425e 100644 --- a/src/iceberg/catalog/rest/http_client.h +++ b/src/iceberg/catalog/rest/http_client.h @@ -61,6 +61,9 @@ class ICEBERG_REST_EXPORT HttpResponse { /// \brief Get the headers of the response as a map. std::unordered_map headers() const; + /// \brief Create a response for use in unit tests. + static HttpResponse MakeForTesting(int32_t status_code, std::string body); + private: friend class HttpClient; class Impl; @@ -71,7 +74,7 @@ class ICEBERG_REST_EXPORT HttpResponse { class ICEBERG_REST_EXPORT HttpClient { public: explicit HttpClient(std::unordered_map default_headers = {}); - ~HttpClient(); + virtual ~HttpClient(); HttpClient(const HttpClient&) = delete; HttpClient& operator=(const HttpClient&) = delete; @@ -79,36 +82,37 @@ class ICEBERG_REST_EXPORT HttpClient { HttpClient& operator=(HttpClient&&) = delete; /// \brief Sends a GET request. - Result Get(const std::string& path, - const std::unordered_map& params, - const std::unordered_map& headers, - const ErrorHandler& error_handler, auth::AuthSession& session); + virtual Result Get( + const std::string& path, + const std::unordered_map& params, + const std::unordered_map& headers, + const ErrorHandler& error_handler, auth::AuthSession& session); /// \brief Sends a POST request. - Result Post(const std::string& path, const std::string& body, - const std::unordered_map& headers, - const ErrorHandler& error_handler, - auth::AuthSession& session); + virtual Result Post(const std::string& path, const std::string& body, + const std::unordered_map& headers, + const ErrorHandler& error_handler, + auth::AuthSession& session); /// \brief Sends a POST request with form data. - Result PostForm( + virtual Result PostForm( const std::string& path, const std::unordered_map& form_data, const std::unordered_map& headers, const ErrorHandler& error_handler, auth::AuthSession& session); /// \brief Sends a HEAD request. - Result Head(const std::string& path, - const std::unordered_map& headers, - const ErrorHandler& error_handler, - auth::AuthSession& session); + virtual Result Head( + const std::string& path, + const std::unordered_map& headers, + const ErrorHandler& error_handler, auth::AuthSession& session); /// \brief Sends a DELETE request. - Result Delete(const std::string& path, - const std::unordered_map& params, - const std::unordered_map& headers, - const ErrorHandler& error_handler, - auth::AuthSession& session); + virtual Result Delete( + const std::string& path, + const std::unordered_map& params, + const std::unordered_map& headers, + const ErrorHandler& error_handler, auth::AuthSession& session); private: std::unordered_map default_headers_; diff --git a/src/iceberg/catalog/rest/rest_catalog.cc b/src/iceberg/catalog/rest/rest_catalog.cc index 147404636..bd52c96f7 100644 --- a/src/iceberg/catalog/rest/rest_catalog.cc +++ b/src/iceberg/catalog/rest/rest_catalog.cc @@ -43,6 +43,8 @@ #include "iceberg/catalog/rest/rest_util.h" #include "iceberg/catalog/rest/types.h" #include "iceberg/json_serde_internal.h" +#include "iceberg/logging/logger.h" +#include "iceberg/logging/log_level.h" #include "iceberg/metrics/metrics_reporters.h" #include "iceberg/partition_spec.h" #include "iceberg/result.h" @@ -900,7 +902,32 @@ Result> RestCatalog::MakeTableFromLoadResult( auto table_catalog = std::make_shared( shared_from_this(), context, identifier, table_config, table_session, table_io); - if (supported_endpoints_.contains(Endpoint::PlanTableScan())) { + // Determine effective scan planning mode: table config overrides client config. + ICEBERG_ASSIGN_OR_RAISE(auto client_mode, + RestCatalogProperties::ScanPlanningModeFrom(config_.configs())); + ICEBERG_ASSIGN_OR_RAISE(auto server_mode, + RestCatalogProperties::ScanPlanningModeFrom(table_config)); + + if (client_mode.has_value() && server_mode.has_value() && + *client_mode != *server_mode) { + Log(LogLevel::kWarn, + "Scan planning mode mismatch for table {}: client config={}, server config={}. " + "Server config will take precedence.", + identifier.ToString(), + *client_mode == ScanPlanningMode::kClient ? "client" : "server", + *server_mode == ScanPlanningMode::kClient ? "client" : "server"); + } + + ScanPlanningMode effective_mode = + server_mode.value_or(client_mode.value_or(ScanPlanningMode::kClient)); + + if (effective_mode == ScanPlanningMode::kServer) { + if (!supported_endpoints_.contains(Endpoint::PlanTableScan())) { + return NotSupported( + "Server requires server-side scan planning for table {} but does not support " + "the PlanTableScan endpoint.", + identifier.ToString()); + } RestScanContext rest_ctx{ .client = client_, .paths = paths_, diff --git a/src/iceberg/catalog/rest/rest_table_scan.cc b/src/iceberg/catalog/rest/rest_table_scan.cc index 30dcc95df..277fbb630 100644 --- a/src/iceberg/catalog/rest/rest_table_scan.cc +++ b/src/iceberg/catalog/rest/rest_table_scan.cc @@ -100,8 +100,10 @@ Result>> RestTableScan::PlanTableScan( if (context_.from_snapshot_id.has_value() && context_.to_snapshot_id.has_value()) { request.start_snapshot_id = context_.from_snapshot_id; request.end_snapshot_id = context_.to_snapshot_id; + request.use_snapshot_schema = true; } else if (context_.snapshot_id.has_value()) { request.snapshot_id = context_.snapshot_id; + request.use_snapshot_schema = context_.use_snapshot_schema; } if (!context_.columns_to_keep_stats.empty()) { @@ -129,8 +131,11 @@ Result>> RestTableScan::PlanTableScan( plan_id = result.plan_id; switch (result.plan_status) { - case PlanStatus::kCompleted: - return ResolveScanTasks(result.plan_tasks, result.file_scan_tasks, specs); + case PlanStatus::kCompleted: { + auto tasks = ResolveScanTasks(result.plan_tasks, result.file_scan_tasks, specs); + if (!tasks.has_value()) CancelPlanning(plan_id); + return tasks; + } case PlanStatus::kSubmitted: return FetchPlanningResult(plan_id, specs); case PlanStatus::kFailed: @@ -165,8 +170,11 @@ Result>> RestTableScan::FetchPlanningR ICEBERG_RETURN_UNEXPECTED(result.Validate()); switch (result.plan_status) { - case PlanStatus::kCompleted: - return ResolveScanTasks(result.plan_tasks, result.file_scan_tasks, specs); + case PlanStatus::kCompleted: { + auto tasks = ResolveScanTasks(result.plan_tasks, result.file_scan_tasks, specs); + if (!tasks.has_value()) CancelPlanning(plan_id); + return tasks; + } case PlanStatus::kSubmitted: { auto elapsed_ms = std::chrono::duration_cast( std::chrono::steady_clock::now() - start) diff --git a/src/iceberg/table_scan.cc b/src/iceberg/table_scan.cc index 1c68d5341..7ce3b4d87 100644 --- a/src/iceberg/table_scan.cc +++ b/src/iceberg/table_scan.cc @@ -408,6 +408,7 @@ TableScanBuilder& TableScanBuilder::UseSnapshot(int64_t snap context_.snapshot_id.value()); ICEBERG_BUILDER_ASSIGN_OR_RETURN(std::ignore, metadata_->SnapshotById(snapshot_id)); context_.snapshot_id = snapshot_id; + context_.use_snapshot_schema = true; return *this; } @@ -415,6 +416,7 @@ template TableScanBuilder& TableScanBuilder::UseRef(const std::string& ref) { if (ref == SnapshotRef::kMainBranch) { context_.snapshot_id.reset(); + context_.use_snapshot_schema = false; return *this; } @@ -427,6 +429,7 @@ TableScanBuilder& TableScanBuilder::UseRef(const std::string const int64_t snapshot_id = iter->second->snapshot_id; ICEBERG_BUILDER_ASSIGN_OR_RETURN(std::ignore, metadata_->SnapshotById(snapshot_id)); context_.snapshot_id = snapshot_id; + context_.use_snapshot_schema = (iter->second->type() == SnapshotRefType::kTag); return *this; } diff --git a/src/iceberg/table_scan.h b/src/iceberg/table_scan.h index f93fe5098..4eb3e0fb2 100644 --- a/src/iceberg/table_scan.h +++ b/src/iceberg/table_scan.h @@ -234,6 +234,7 @@ struct TableScanContext { std::optional to_snapshot_id; std::string branch{}; std::optional min_rows_requested; + bool use_snapshot_schema{false}; OptionalExecutor plan_executor; std::string table_name; std::shared_ptr metrics_reporter; diff --git a/src/iceberg/test/CMakeLists.txt b/src/iceberg/test/CMakeLists.txt index f23bc9181..0a2166ed6 100644 --- a/src/iceberg/test/CMakeLists.txt +++ b/src/iceberg/test/CMakeLists.txt @@ -318,11 +318,13 @@ if(ICEBERG_BUILD_REST) add_rest_iceberg_test(rest_catalog_test SOURCES auth_manager_test.cc + catalog_properties_test.cc error_handlers_test.cc endpoint_test.cc rest_file_io_test.cc rest_json_serde_test.cc rest_metrics_reporter_test.cc + rest_table_scan_test.cc rest_util_test.cc) if(ICEBERG_SIGV4) diff --git a/src/iceberg/test/catalog_properties_test.cc b/src/iceberg/test/catalog_properties_test.cc new file mode 100644 index 000000000..47abe7682 --- /dev/null +++ b/src/iceberg/test/catalog_properties_test.cc @@ -0,0 +1,81 @@ +/* + * 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/catalog/rest/catalog_properties.h" + +#include + +#include "iceberg/test/matchers.h" + +namespace iceberg::rest { + +TEST(ScanPlanningModeTest, MissingKeyReturnsNullopt) { + std::unordered_map config; + auto result = RestCatalogProperties::ScanPlanningModeFrom(config); + ASSERT_THAT(result, IsOk()); + EXPECT_FALSE(result->has_value()); +} + +TEST(ScanPlanningModeTest, ClientLowercaseReturnsKClient) { + auto result = RestCatalogProperties::ScanPlanningModeFrom( + {{"scan-planning-mode", "client"}}); + ASSERT_THAT(result, IsOk()); + ASSERT_TRUE(result->has_value()); + EXPECT_EQ(**result, ScanPlanningMode::kClient); +} + +TEST(ScanPlanningModeTest, ServerLowercaseReturnsKServer) { + auto result = RestCatalogProperties::ScanPlanningModeFrom( + {{"scan-planning-mode", "server"}}); + ASSERT_THAT(result, IsOk()); + ASSERT_TRUE(result->has_value()); + EXPECT_EQ(**result, ScanPlanningMode::kServer); +} + +TEST(ScanPlanningModeTest, ClientUppercaseReturnsKClient) { + auto result = RestCatalogProperties::ScanPlanningModeFrom( + {{"scan-planning-mode", "CLIENT"}}); + ASSERT_THAT(result, IsOk()); + ASSERT_TRUE(result->has_value()); + EXPECT_EQ(**result, ScanPlanningMode::kClient); +} + +TEST(ScanPlanningModeTest, ServerUppercaseReturnsKServer) { + auto result = RestCatalogProperties::ScanPlanningModeFrom( + {{"scan-planning-mode", "SERVER"}}); + ASSERT_THAT(result, IsOk()); + ASSERT_TRUE(result->has_value()); + EXPECT_EQ(**result, ScanPlanningMode::kServer); +} + +TEST(ScanPlanningModeTest, InvalidValueReturnsError) { + auto result = RestCatalogProperties::ScanPlanningModeFrom( + {{"scan-planning-mode", "invalid"}}); + EXPECT_THAT(result, IsError(ErrorKind::kInvalidArgument)); +} + +TEST(ScanPlanningModeTest, OtherKeysAreIgnored) { + auto result = RestCatalogProperties::ScanPlanningModeFrom( + {{"other-key", "server"}, {"scan-planning-mode", "client"}}); + ASSERT_THAT(result, IsOk()); + ASSERT_TRUE(result->has_value()); + EXPECT_EQ(**result, ScanPlanningMode::kClient); +} + +} // namespace iceberg::rest diff --git a/src/iceberg/test/rest_table_scan_test.cc b/src/iceberg/test/rest_table_scan_test.cc new file mode 100644 index 000000000..89eab8087 --- /dev/null +++ b/src/iceberg/test/rest_table_scan_test.cc @@ -0,0 +1,403 @@ +/* + * 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/catalog/rest/rest_table_scan.h" + +#include +#include +#include +#include +#include + +#include +#include +#include + +#include "iceberg/catalog/rest/auth/auth_session.h" +#include "iceberg/catalog/rest/endpoint.h" +#include "iceberg/catalog/rest/error_handlers.h" +#include "iceberg/catalog/rest/http_client.h" +#include "iceberg/catalog/rest/resource_paths.h" +#include "iceberg/catalog/rest/rest_table.h" +#include "iceberg/file_io.h" +#include "iceberg/partition_spec.h" +#include "iceberg/schema.h" +#include "iceberg/snapshot.h" +#include "iceberg/table_identifier.h" +#include "iceberg/table_metadata.h" +#include "iceberg/table_scan.h" +#include "iceberg/test/matchers.h" +#include "iceberg/type.h" + +namespace iceberg::rest { + +using ::testing::_; +using ::testing::Return; + +// -------------------------------------------------------------------------- +// Mock HTTP client that overrides the virtual methods of HttpClient. +// The base class constructor creates a cpr::ConnectionPool, which is a +// lightweight allocation (no network connections are opened at construction). +// -------------------------------------------------------------------------- +class MockHttpClient : public HttpClient { + public: + MockHttpClient() : HttpClient({}) {} + + MOCK_METHOD(Result, Get, + (const std::string& path, + (const std::unordered_map&) params, + (const std::unordered_map&) headers, + const ErrorHandler& error_handler, auth::AuthSession& session), + (override)); + + MOCK_METHOD(Result, Post, + (const std::string& path, const std::string& body, + (const std::unordered_map&) headers, + const ErrorHandler& error_handler, auth::AuthSession& session), + (override)); + + MOCK_METHOD(Result, Delete, + (const std::string& path, + (const std::unordered_map&) params, + (const std::unordered_map&) headers, + const ErrorHandler& error_handler, auth::AuthSession& session), + (override)); +}; + +// -------------------------------------------------------------------------- +// Minimal FileIO stub (no real I/O needed for server-side scan planning tests) +// -------------------------------------------------------------------------- +class NoOpFileIO : public FileIO { + public: + Result ReadFile(const std::string&, + std::optional) override { + return IOError("NoOpFileIO"); + } + Status WriteFile(const std::string&, std::string_view) override { return {}; } + Status DeleteFile(const std::string&) override { return {}; } +}; + +// -------------------------------------------------------------------------- +// Test fixture shared by RestTableScan tests. +// -------------------------------------------------------------------------- +class RestTableScanTest : public ::testing::Test { + protected: + void SetUp() override { + schema_ = std::make_shared(std::vector{ + SchemaField::MakeRequired(1, "id", int32()), + SchemaField::MakeRequired(2, "data", string())}); + + auto spec = PartitionSpec::Unpartitioned(); + + constexpr int64_t kSnapshotId = 1000L; + auto snapshot = std::make_shared( + Snapshot{.snapshot_id = kSnapshotId, + .sequence_number = 1L, + .timestamp_ms = TimePointMsFromUnixMs(1609459200000L), + .manifest_list = "/tmp/manifest-list.avro", + .schema_id = schema_->schema_id()}); + + metadata_ = std::make_shared(TableMetadata{ + .format_version = 2, + .table_uuid = "test-uuid", + .location = "/tmp/table", + .last_sequence_number = 1L, + .last_updated_ms = TimePointMsFromUnixMs(1609459200000L), + .last_column_id = 2, + .schemas = {schema_}, + .current_schema_id = schema_->schema_id(), + .partition_specs = {spec}, + .default_spec_id = spec->spec_id(), + .last_partition_id = 999, + .current_snapshot_id = kSnapshotId, + .snapshots = {snapshot}, + .refs = {{"main", + std::make_shared(SnapshotRef{ + .snapshot_id = kSnapshotId, + .retention = SnapshotRef::Branch{}})}}}); + + file_io_ = std::make_shared(); + + mock_client_ = std::make_shared(); + + ICEBERG_UNWRAP_OR_FAIL(paths_, ResourcePaths::Make( + "http://test-server", /*prefix=*/"", + /*namespace_separator=*/"%1F")); + + session_ = auth::AuthSession::MakeDefault(/*headers=*/{}); + + identifier_ = TableIdentifier{Namespace{{"default"}}, "my_table"}; + + all_plan_endpoints_ = {Endpoint::PlanTableScan(), Endpoint::FetchPlanningResult(), + Endpoint::CancelPlanning(), Endpoint::FetchScanTasks()}; + } + + // Pass std::nullopt to get the full set of plan endpoints (default). + // Pass an explicit set (including empty) to use exactly that set. + RestScanContext MakeContext( + std::optional> endpoints = std::nullopt) { + auto effective = + endpoints.has_value() ? std::move(*endpoints) : all_plan_endpoints_; + return RestScanContext{ + .client = mock_client_, + .paths = paths_, + .session = session_, + .supported_endpoints = std::move(effective), + .identifier = identifier_, + }; + } + + Result> MakeScan(RestScanContext ctx) { + return RestTableScan::Make(metadata_, schema_, file_io_, + internal::TableScanContext{}, std::move(ctx)); + } + + std::shared_ptr schema_; + std::shared_ptr metadata_; + std::shared_ptr file_io_; + std::shared_ptr mock_client_; + std::shared_ptr paths_; + std::shared_ptr session_; + TableIdentifier identifier_; + std::unordered_set all_plan_endpoints_; +}; + +// -------------------------------------------------------------------------- +// PlanFiles: server returns COMPLETED immediately, no file scan tasks. +// -------------------------------------------------------------------------- +TEST_F(RestTableScanTest, PlanFilesCompleted) { + constexpr std::string_view kResponseBody = R"({"status":"completed"})"; + EXPECT_CALL(*mock_client_, Post(_, _, _, _, _)) + .WillOnce(Return(HttpResponse::MakeForTesting(200, std::string(kResponseBody)))); + + ICEBERG_UNWRAP_OR_FAIL(auto scan, MakeScan(MakeContext())); + ICEBERG_UNWRAP_OR_FAIL(auto tasks, scan->PlanFiles()); + EXPECT_TRUE(tasks.empty()); +} + +// -------------------------------------------------------------------------- +// PlanFiles: server returns COMPLETED with a non-empty plan-id (still valid). +// -------------------------------------------------------------------------- +TEST_F(RestTableScanTest, PlanFilesCompletedWithPlanId) { + constexpr std::string_view kResponseBody = + R"({"status":"completed","plan-id":"plan-abc"})"; + EXPECT_CALL(*mock_client_, Post(_, _, _, _, _)) + .WillOnce(Return(HttpResponse::MakeForTesting(200, std::string(kResponseBody)))); + + ICEBERG_UNWRAP_OR_FAIL(auto scan, MakeScan(MakeContext())); + ICEBERG_UNWRAP_OR_FAIL(auto tasks, scan->PlanFiles()); + EXPECT_TRUE(tasks.empty()); +} + +// -------------------------------------------------------------------------- +// PlanFiles: server returns SUBMITTED → poll returns COMPLETED. +// -------------------------------------------------------------------------- +TEST_F(RestTableScanTest, PlanFilesSubmittedThenCompleted) { + constexpr std::string_view kSubmittedBody = + R"({"status":"submitted","plan-id":"plan-poll-1"})"; + constexpr std::string_view kCompletedBody = R"({"status":"completed"})"; + + EXPECT_CALL(*mock_client_, Post(_, _, _, _, _)) + .WillOnce(Return(HttpResponse::MakeForTesting(200, std::string(kSubmittedBody)))); + EXPECT_CALL(*mock_client_, Get(_, _, _, _, _)) + .WillOnce(Return(HttpResponse::MakeForTesting(200, std::string(kCompletedBody)))); + + ICEBERG_UNWRAP_OR_FAIL(auto scan, MakeScan(MakeContext())); + ICEBERG_UNWRAP_OR_FAIL(auto tasks, scan->PlanFiles()); + EXPECT_TRUE(tasks.empty()); +} + +// -------------------------------------------------------------------------- +// PlanFiles: server returns FAILED → scan returns IOError. +// -------------------------------------------------------------------------- +TEST_F(RestTableScanTest, PlanFilesFailed) { + constexpr std::string_view kFailedBody = + R"({"status":"failed","error":{"message":"server error","type":"ServerError","code":500}})"; + EXPECT_CALL(*mock_client_, Post(_, _, _, _, _)) + .WillOnce(Return(HttpResponse::MakeForTesting(200, std::string(kFailedBody)))); + + ICEBERG_UNWRAP_OR_FAIL(auto scan, MakeScan(MakeContext())); + auto result = scan->PlanFiles(); + EXPECT_THAT(result, IsError(ErrorKind::kIOError)); +} + +// -------------------------------------------------------------------------- +// PlanFiles: PlanTableScan endpoint missing → NotSupported error. +// -------------------------------------------------------------------------- +TEST_F(RestTableScanTest, PlanFilesEndpointNotSupported) { + ICEBERG_UNWRAP_OR_FAIL( + auto scan, MakeScan(MakeContext(std::unordered_set{}))); + auto result = scan->PlanFiles(); + EXPECT_THAT(result, IsError(ErrorKind::kNotSupported)); +} + +// -------------------------------------------------------------------------- +// PlanFiles with plan-tasks: server returns COMPLETED with opaque task token, +// then FetchScanTasks is called and returns no file scan tasks. +// -------------------------------------------------------------------------- +TEST_F(RestTableScanTest, PlanFilesWithPlanTasks) { + constexpr std::string_view kPlanResponse = + R"({"status":"completed","plan-id":"plan-1","plan-tasks":["tok-1"]})"; + // FetchScanTasksResponse requires at least one of plan-tasks or file-scan-tasks present. + constexpr std::string_view kTasksResponse = R"({"file-scan-tasks":[]})"; + + EXPECT_CALL(*mock_client_, Post(_, _, _, _, _)) + .WillOnce(Return(HttpResponse::MakeForTesting(200, std::string(kPlanResponse)))) + .WillOnce(Return(HttpResponse::MakeForTesting(200, std::string(kTasksResponse)))); + + ICEBERG_UNWRAP_OR_FAIL(auto scan, MakeScan(MakeContext())); + ICEBERG_UNWRAP_OR_FAIL(auto tasks, scan->PlanFiles()); + EXPECT_TRUE(tasks.empty()); +} + +// -------------------------------------------------------------------------- +// Cancel is called when FetchScanTasks fails after a COMPLETED response that +// included plan-tasks. This mirrors the Java cancelPlan-on-close behavior. +// -------------------------------------------------------------------------- +TEST_F(RestTableScanTest, CancelCalledWhenFetchScanTasksFails) { + constexpr std::string_view kPlanResponse = + R"({"status":"completed","plan-id":"plan-cancel-1","plan-tasks":["tok-a"]})"; + + EXPECT_CALL(*mock_client_, Post(_, _, _, _, _)) + .WillOnce(Return(HttpResponse::MakeForTesting(200, std::string(kPlanResponse)))) + .WillOnce(Return(IOError("FetchScanTasks failed"))); + EXPECT_CALL(*mock_client_, Delete(_, _, _, _, _)) + .WillOnce(Return(HttpResponse::MakeForTesting(200, "{}"))); + + ICEBERG_UNWRAP_OR_FAIL(auto scan, MakeScan(MakeContext())); + auto result = scan->PlanFiles(); + EXPECT_THAT(result, IsError(ErrorKind::kIOError)); +} + +// -------------------------------------------------------------------------- +// Cancel is a no-op when plan_id is empty (server returned no plan-id). +// -------------------------------------------------------------------------- +TEST_F(RestTableScanTest, CancelIsNoOpWithEmptyPlanId) { + constexpr std::string_view kPlanResponse = + R"({"status":"completed","plan-tasks":["tok-b"]})"; + + EXPECT_CALL(*mock_client_, Post(_, _, _, _, _)) + .WillOnce(Return(HttpResponse::MakeForTesting(200, std::string(kPlanResponse)))) + .WillOnce(Return(IOError("FetchScanTasks failed"))); + EXPECT_CALL(*mock_client_, Delete(_, _, _, _, _)).Times(0); + + ICEBERG_UNWRAP_OR_FAIL(auto scan, MakeScan(MakeContext())); + auto result = scan->PlanFiles(); + EXPECT_THAT(result, IsError(ErrorKind::kIOError)); +} + +// -------------------------------------------------------------------------- +// Cancel is a no-op when CancelPlanning endpoint is not advertised. +// -------------------------------------------------------------------------- +TEST_F(RestTableScanTest, CancelIsNoOpWhenEndpointNotAdvertised) { + constexpr std::string_view kPlanResponse = + R"({"status":"completed","plan-id":"plan-2","plan-tasks":["tok-c"]})"; + + std::unordered_set endpoints_without_cancel = { + Endpoint::PlanTableScan(), Endpoint::FetchPlanningResult(), + Endpoint::FetchScanTasks()}; + + EXPECT_CALL(*mock_client_, Post(_, _, _, _, _)) + .WillOnce(Return(HttpResponse::MakeForTesting(200, std::string(kPlanResponse)))) + .WillOnce(Return(IOError("FetchScanTasks failed"))); + EXPECT_CALL(*mock_client_, Delete(_, _, _, _, _)).Times(0); + + ICEBERG_UNWRAP_OR_FAIL(auto scan, + MakeScan(MakeContext(endpoints_without_cancel))); + auto result = scan->PlanFiles(); + EXPECT_THAT(result, IsError(ErrorKind::kIOError)); +} + +// -------------------------------------------------------------------------- +// FetchPlanningResult: FetchPlanningResult endpoint missing → NotSupported. +// -------------------------------------------------------------------------- +TEST_F(RestTableScanTest, FetchPlanningResultEndpointNotSupported) { + constexpr std::string_view kSubmittedBody = + R"({"status":"submitted","plan-id":"plan-3"})"; + + std::unordered_set endpoints_without_fetch = { + Endpoint::PlanTableScan(), Endpoint::CancelPlanning(), + Endpoint::FetchScanTasks()}; + + EXPECT_CALL(*mock_client_, Post(_, _, _, _, _)) + .WillOnce(Return(HttpResponse::MakeForTesting(200, std::string(kSubmittedBody)))); + + ICEBERG_UNWRAP_OR_FAIL(auto scan, + MakeScan(MakeContext(endpoints_without_fetch))); + auto result = scan->PlanFiles(); + EXPECT_THAT(result, IsError(ErrorKind::kNotSupported)); +} + +// -------------------------------------------------------------------------- +// FetchScanTasks: endpoint missing → NotSupported. +// -------------------------------------------------------------------------- +TEST_F(RestTableScanTest, FetchScanTasksEndpointNotSupported) { + constexpr std::string_view kPlanResponse = + R"({"status":"completed","plan-id":"plan-4","plan-tasks":["tok-d"]})"; + + std::unordered_set endpoints_without_tasks = { + Endpoint::PlanTableScan(), Endpoint::FetchPlanningResult(), + Endpoint::CancelPlanning()}; + + EXPECT_CALL(*mock_client_, Post(_, _, _, _, _)) + .WillOnce(Return(HttpResponse::MakeForTesting(200, std::string(kPlanResponse)))); + EXPECT_CALL(*mock_client_, Delete(_, _, _, _, _)) + .WillOnce(Return(HttpResponse::MakeForTesting(200, "{}"))); + + ICEBERG_UNWRAP_OR_FAIL(auto scan, + MakeScan(MakeContext(endpoints_without_tasks))); + auto result = scan->PlanFiles(); + EXPECT_THAT(result, IsError(ErrorKind::kNotSupported)); +} + +// -------------------------------------------------------------------------- +// use_snapshot_schema: UseSnapshot() sets it to true in the builder context. +// RestTableScanBuilder propagates context from DataTableScanBuilder. +// -------------------------------------------------------------------------- +TEST_F(RestTableScanTest, UseSnapshotPropagatesUseSnapshotSchemaInContext) { + constexpr int64_t kSnapshotId = 1000L; + RestTableScanBuilder builder(metadata_, file_io_, MakeContext(std::nullopt)); + builder.UseSnapshot(kSnapshotId); + ICEBERG_UNWRAP_OR_FAIL(auto scan, builder.Build()); + EXPECT_TRUE(scan->context().use_snapshot_schema); +} + +// -------------------------------------------------------------------------- +// use_snapshot_schema: default scan does not set use_snapshot_schema. +// -------------------------------------------------------------------------- +TEST_F(RestTableScanTest, DefaultScanDoesNotSetUseSnapshotSchema) { + RestTableScanBuilder builder(metadata_, file_io_, MakeContext(std::nullopt)); + ICEBERG_UNWRAP_OR_FAIL(auto scan, builder.Build()); + EXPECT_FALSE(scan->context().use_snapshot_schema); +} + +// -------------------------------------------------------------------------- +// RestTable::NewScan returns a RestTableScanBuilder (not a plain builder). +// -------------------------------------------------------------------------- +TEST_F(RestTableScanTest, RestTableNewScanReturnsRestTableScanBuilder) { + ICEBERG_UNWRAP_OR_FAIL( + auto table, + RestTable::Make(identifier_, metadata_, "/tmp/metadata.json", file_io_, + /*catalog=*/nullptr, MakeContext(std::nullopt))); + ICEBERG_UNWRAP_OR_FAIL(auto builder, table->NewScan()); + auto* typed = dynamic_cast(builder.get()); + EXPECT_NE(typed, nullptr); +} + +} // namespace iceberg::rest diff --git a/src/iceberg/test/table_scan_test.cc b/src/iceberg/test/table_scan_test.cc index c9af765e9..88abbaef5 100644 --- a/src/iceberg/test/table_scan_test.cc +++ b/src/iceberg/test/table_scan_test.cc @@ -841,6 +841,45 @@ TEST_P(TableScanTest, SchemaWithSelectedColumnsAndFilter) { } } +// use_snapshot_schema propagation tests: verify the field is set correctly for +// UseSnapshot, UseRef (tag vs branch), and default/incremental scans. +TEST_P(TableScanTest, UseSnapshotSetsTrueUseSnapshotSchema) { + constexpr int64_t kSnapshotId = 1000L; + ICEBERG_UNWRAP_OR_FAIL(auto builder, + DataTableScanBuilder::Make(table_metadata_, file_io_)); + builder->UseSnapshot(kSnapshotId); + ICEBERG_UNWRAP_OR_FAIL(auto scan, builder->Build()); + EXPECT_TRUE(scan->context().use_snapshot_schema); +} + +TEST_P(TableScanTest, UseRefTagSetsTrueUseSnapshotSchema) { + constexpr int64_t kSnapshotId = 1000L; + table_metadata_->refs["v1.0"] = std::make_shared( + SnapshotRef{.snapshot_id = kSnapshotId, .retention = SnapshotRef::Tag{}}); + + ICEBERG_UNWRAP_OR_FAIL(auto builder, + DataTableScanBuilder::Make(table_metadata_, file_io_)); + builder->UseRef("v1.0"); + ICEBERG_UNWRAP_OR_FAIL(auto scan, builder->Build()); + EXPECT_TRUE(scan->context().use_snapshot_schema); +} + +TEST_P(TableScanTest, UseRefBranchSetsFalseUseSnapshotSchema) { + ICEBERG_UNWRAP_OR_FAIL(auto builder, + DataTableScanBuilder::Make(table_metadata_, file_io_)); + builder->UseRef("main"); + ICEBERG_UNWRAP_OR_FAIL(auto scan, builder->Build()); + EXPECT_FALSE(scan->context().use_snapshot_schema); +} + +TEST_P(TableScanTest, DefaultScanHasFalseUseSnapshotSchema) { + ICEBERG_UNWRAP_OR_FAIL(auto builder, + DataTableScanBuilder::Make(table_metadata_, file_io_)); + ICEBERG_UNWRAP_OR_FAIL(auto scan, builder->Build()); + EXPECT_FALSE(scan->context().use_snapshot_schema); +} + + INSTANTIATE_TEST_SUITE_P(TableScanVersions, TableScanTest, testing::Values(1, 2, 3)); } // namespace iceberg From 5e867b2dbe1aac94d2a1b80618adf85a55428523 Mon Sep 17 00:00:00 2001 From: Sandeep Gottimukkala Date: Sun, 13 Sep 2026 12:53:34 -0700 Subject: [PATCH 03/16] Fix API compatibility after rebase on main Upstream changed Table and DataTableScanBuilder constructors to require full_name/table_name and MetricsReporter parameters. Updated RestTable, RestTableScanBuilder, and their callers accordingly. --- src/iceberg/catalog/rest/rest_catalog.cc | 3 ++- src/iceberg/catalog/rest/rest_table.cc | 14 ++++++++++---- src/iceberg/catalog/rest/rest_table.h | 6 +++++- src/iceberg/catalog/rest/rest_table_scan.cc | 5 ++++- src/iceberg/catalog/rest/rest_table_scan.h | 3 +++ src/iceberg/test/rest_table_scan_test.cc | 9 ++++++--- 6 files changed, 30 insertions(+), 10 deletions(-) diff --git a/src/iceberg/catalog/rest/rest_catalog.cc b/src/iceberg/catalog/rest/rest_catalog.cc index bd52c96f7..8fe71fe15 100644 --- a/src/iceberg/catalog/rest/rest_catalog.cc +++ b/src/iceberg/catalog/rest/rest_catalog.cc @@ -937,7 +937,8 @@ Result> RestCatalog::MakeTableFromLoadResult( }; return RestTable::Make(identifier, std::move(result.metadata), std::move(result.metadata_location), std::move(table_io), - std::move(table_catalog), std::move(rest_ctx)); + std::move(table_catalog), RestTableName(name_, identifier), + reporter, std::move(rest_ctx)); } return Table::Make(identifier, std::move(result.metadata), diff --git a/src/iceberg/catalog/rest/rest_table.cc b/src/iceberg/catalog/rest/rest_table.cc index 00f8856d1..727444443 100644 --- a/src/iceberg/catalog/rest/rest_table.cc +++ b/src/iceberg/catalog/rest/rest_table.cc @@ -31,9 +31,12 @@ namespace iceberg::rest { RestTable::RestTable(TableIdentifier identifier, std::shared_ptr metadata, std::string metadata_location, std::shared_ptr io, - std::shared_ptr catalog, RestScanContext rest_context) + std::shared_ptr catalog, std::string full_name, + std::shared_ptr reporter, + RestScanContext rest_context) : Table(std::move(identifier), std::move(metadata), std::move(metadata_location), - std::move(io), std::move(catalog)), + std::move(io), std::move(catalog), std::move(full_name), + std::move(reporter)), rest_context_(std::move(rest_context)) {} RestTable::~RestTable() = default; @@ -43,6 +46,8 @@ Result> RestTable::Make(TableIdentifier identifier, std::string metadata_location, std::shared_ptr io, std::shared_ptr catalog, + std::string full_name, + std::shared_ptr reporter, RestScanContext rest_context) { if (metadata == nullptr) { return InvalidArgument("Metadata cannot be null"); @@ -50,11 +55,12 @@ Result> RestTable::Make(TableIdentifier identifier, return std::shared_ptr( new RestTable(std::move(identifier), std::move(metadata), std::move(metadata_location), std::move(io), std::move(catalog), - std::move(rest_context))); + std::move(full_name), std::move(reporter), std::move(rest_context))); } Result> RestTable::NewScan() const { - return std::make_unique(metadata_, io_, rest_context_); + return std::make_unique(metadata_, io_, full_name_, reporter_, + rest_context_); } } // namespace iceberg::rest diff --git a/src/iceberg/catalog/rest/rest_table.h b/src/iceberg/catalog/rest/rest_table.h index 8e9172b09..54208d26a 100644 --- a/src/iceberg/catalog/rest/rest_table.h +++ b/src/iceberg/catalog/rest/rest_table.h @@ -24,6 +24,7 @@ #include "iceberg/catalog/rest/iceberg_rest_export.h" #include "iceberg/catalog/rest/rest_table_scan.h" +#include "iceberg/metrics/metrics_reporter.h" #include "iceberg/result.h" #include "iceberg/table.h" #include "iceberg/type_fwd.h" @@ -42,6 +43,8 @@ class ICEBERG_REST_EXPORT RestTable final : public Table { std::string metadata_location, std::shared_ptr io, std::shared_ptr catalog, + std::string full_name, + std::shared_ptr reporter, RestScanContext rest_context); ~RestTable() override; @@ -53,7 +56,8 @@ class ICEBERG_REST_EXPORT RestTable final : public Table { private: RestTable(TableIdentifier identifier, std::shared_ptr metadata, std::string metadata_location, std::shared_ptr io, - std::shared_ptr catalog, RestScanContext rest_context); + std::shared_ptr catalog, std::string full_name, + std::shared_ptr reporter, RestScanContext rest_context); RestScanContext rest_context_; }; diff --git a/src/iceberg/catalog/rest/rest_table_scan.cc b/src/iceberg/catalog/rest/rest_table_scan.cc index 277fbb630..7201cfa35 100644 --- a/src/iceberg/catalog/rest/rest_table_scan.cc +++ b/src/iceberg/catalog/rest/rest_table_scan.cc @@ -262,8 +262,11 @@ void RestTableScan::CancelPlanning(const std::string& plan_id) const { RestTableScanBuilder::RestTableScanBuilder(std::shared_ptr metadata, std::shared_ptr io, + std::string table_name, + std::shared_ptr metrics_reporter, RestScanContext rest_context) - : DataTableScanBuilder(std::move(metadata), std::move(io)), + : DataTableScanBuilder(std::move(metadata), std::move(io), std::move(table_name), + std::move(metrics_reporter)), rest_context_(std::move(rest_context)) {} Result> RestTableScanBuilder::Build() { diff --git a/src/iceberg/catalog/rest/rest_table_scan.h b/src/iceberg/catalog/rest/rest_table_scan.h index 149e8de91..2bc28a20e 100644 --- a/src/iceberg/catalog/rest/rest_table_scan.h +++ b/src/iceberg/catalog/rest/rest_table_scan.h @@ -27,6 +27,7 @@ #include "iceberg/catalog/rest/endpoint.h" #include "iceberg/catalog/rest/iceberg_rest_export.h" +#include "iceberg/metrics/metrics_reporter.h" #include "iceberg/result.h" #include "iceberg/table_identifier.h" #include "iceberg/table_scan.h" @@ -104,6 +105,8 @@ class ICEBERG_REST_EXPORT RestTableScan : public DataTableScan { class ICEBERG_REST_EXPORT RestTableScanBuilder : public DataTableScanBuilder { public: RestTableScanBuilder(std::shared_ptr metadata, std::shared_ptr io, + std::string table_name, + std::shared_ptr metrics_reporter, RestScanContext rest_context); /// \brief Resolves schema/context via parent logic then creates a RestTableScan. diff --git a/src/iceberg/test/rest_table_scan_test.cc b/src/iceberg/test/rest_table_scan_test.cc index 89eab8087..749b12dcb 100644 --- a/src/iceberg/test/rest_table_scan_test.cc +++ b/src/iceberg/test/rest_table_scan_test.cc @@ -372,7 +372,8 @@ TEST_F(RestTableScanTest, FetchScanTasksEndpointNotSupported) { // -------------------------------------------------------------------------- TEST_F(RestTableScanTest, UseSnapshotPropagatesUseSnapshotSchemaInContext) { constexpr int64_t kSnapshotId = 1000L; - RestTableScanBuilder builder(metadata_, file_io_, MakeContext(std::nullopt)); + RestTableScanBuilder builder(metadata_, file_io_, "test.my_table", nullptr, + MakeContext(std::nullopt)); builder.UseSnapshot(kSnapshotId); ICEBERG_UNWRAP_OR_FAIL(auto scan, builder.Build()); EXPECT_TRUE(scan->context().use_snapshot_schema); @@ -382,7 +383,8 @@ TEST_F(RestTableScanTest, UseSnapshotPropagatesUseSnapshotSchemaInContext) { // use_snapshot_schema: default scan does not set use_snapshot_schema. // -------------------------------------------------------------------------- TEST_F(RestTableScanTest, DefaultScanDoesNotSetUseSnapshotSchema) { - RestTableScanBuilder builder(metadata_, file_io_, MakeContext(std::nullopt)); + RestTableScanBuilder builder(metadata_, file_io_, "test.my_table", nullptr, + MakeContext(std::nullopt)); ICEBERG_UNWRAP_OR_FAIL(auto scan, builder.Build()); EXPECT_FALSE(scan->context().use_snapshot_schema); } @@ -394,7 +396,8 @@ TEST_F(RestTableScanTest, RestTableNewScanReturnsRestTableScanBuilder) { ICEBERG_UNWRAP_OR_FAIL( auto table, RestTable::Make(identifier_, metadata_, "/tmp/metadata.json", file_io_, - /*catalog=*/nullptr, MakeContext(std::nullopt))); + /*catalog=*/nullptr, "test.my_table", nullptr, + MakeContext(std::nullopt))); ICEBERG_UNWRAP_OR_FAIL(auto builder, table->NewScan()); auto* typed = dynamic_cast(builder.get()); EXPECT_NE(typed, nullptr); From 592318b84ad111c54e323f046ce537d1500af6ef Mon Sep 17 00:00:00 2001 From: Sandeep Gottimukkala Date: Sun, 13 Sep 2026 14:25:50 -0700 Subject: [PATCH 04/16] feat(rest): support storage credentials in scan planning responses Parse storage-credentials from PlanTableScanResponse, FetchPlanningResultResponse, and FetchScanTasksResponse. When credentials are present, build a scan-scoped FileIO via MakeTableFileIO and expose it through RestTableScan::effective_io() for callers to use when reading the returned scan tasks. --- src/iceberg/catalog/rest/json_serde.cc | 24 +++ src/iceberg/catalog/rest/rest_catalog.cc | 4 +- src/iceberg/catalog/rest/rest_table_scan.cc | 58 ++++--- src/iceberg/catalog/rest/rest_table_scan.h | 22 ++- src/iceberg/catalog/rest/types.cc | 1 + src/iceberg/catalog/rest/types.h | 5 +- src/iceberg/test/rest_table_scan_test.cc | 169 +++++++++++++------- src/iceberg/test/table_scan_test.cc | 13 +- 8 files changed, 206 insertions(+), 90 deletions(-) diff --git a/src/iceberg/catalog/rest/json_serde.cc b/src/iceberg/catalog/rest/json_serde.cc index 3ce753f18..d6520878a 100644 --- a/src/iceberg/catalog/rest/json_serde.cc +++ b/src/iceberg/catalog/rest/json_serde.cc @@ -531,6 +531,15 @@ Result ScanTaskFieldsToJson( json[kFileScanTasks] = std::move(tasks_json); } + if (!response.storage_credentials.empty()) { + nlohmann::json creds_json = nlohmann::json::array(); + for (const auto& cred : response.storage_credentials) { + ICEBERG_ASSIGN_OR_RAISE(auto entry, StorageCredentialToJson(cred)); + creds_json.push_back(std::move(entry)); + } + json[kStorageCredentials] = std::move(creds_json); + } + return json; } @@ -571,6 +580,21 @@ Status ScanTaskFieldsFromJson( FileScanTasksFromJson(file_scan_tasks_json, response.delete_files, partition_specs_by_id, schema)); } + + // 4. storage_credentials + if (json.contains(kStorageCredentials)) { + ICEBERG_ASSIGN_OR_RAISE(auto creds_json, + GetJsonValue(json, kStorageCredentials)); + if (!creds_json.is_array()) { + return JsonParseError("Cannot parse storage credentials from non-array: {}", + SafeDumpJson(creds_json)); + } + for (const auto& entry : creds_json) { + ICEBERG_ASSIGN_OR_RAISE(auto cred, StorageCredentialFromJson(entry)); + response.storage_credentials.push_back(std::move(cred)); + } + } + return {}; } diff --git a/src/iceberg/catalog/rest/rest_catalog.cc b/src/iceberg/catalog/rest/rest_catalog.cc index 8fe71fe15..5a8bb2507 100644 --- a/src/iceberg/catalog/rest/rest_catalog.cc +++ b/src/iceberg/catalog/rest/rest_catalog.cc @@ -43,8 +43,8 @@ #include "iceberg/catalog/rest/rest_util.h" #include "iceberg/catalog/rest/types.h" #include "iceberg/json_serde_internal.h" -#include "iceberg/logging/logger.h" #include "iceberg/logging/log_level.h" +#include "iceberg/logging/logger.h" #include "iceberg/metrics/metrics_reporters.h" #include "iceberg/partition_spec.h" #include "iceberg/result.h" @@ -934,6 +934,8 @@ Result> RestCatalog::MakeTableFromLoadResult( .session = table_session, .supported_endpoints = supported_endpoints_, .identifier = identifier, + .catalog_config = config_.configs(), + .table_config = table_config, }; return RestTable::Make(identifier, std::move(result.metadata), std::move(result.metadata_location), std::move(table_io), diff --git a/src/iceberg/catalog/rest/rest_table_scan.cc b/src/iceberg/catalog/rest/rest_table_scan.cc index 7201cfa35..3e11c71dc 100644 --- a/src/iceberg/catalog/rest/rest_table_scan.cc +++ b/src/iceberg/catalog/rest/rest_table_scan.cc @@ -29,6 +29,7 @@ #include "iceberg/catalog/rest/http_client.h" #include "iceberg/catalog/rest/json_serde_internal.h" #include "iceberg/catalog/rest/resource_paths.h" +#include "iceberg/catalog/rest/rest_file_io.h" #include "iceberg/catalog/rest/types.h" #include "iceberg/json_serde_internal.h" #include "iceberg/partition_spec.h" @@ -115,8 +116,7 @@ Result>> RestTableScan::PlanTableScan( } } - ICEBERG_ASSIGN_OR_RAISE(auto path, - rest_context_.paths->Plan(rest_context_.identifier)); + ICEBERG_ASSIGN_OR_RAISE(auto path, rest_context_.paths->Plan(rest_context_.identifier)); ICEBERG_ASSIGN_OR_RAISE(auto request_json, ToJson(request)); ICEBERG_ASSIGN_OR_RAISE(auto json_request, ToJsonString(request_json)); ICEBERG_ASSIGN_OR_RAISE( @@ -132,6 +132,7 @@ Result>> RestTableScan::PlanTableScan( switch (result.plan_status) { case PlanStatus::kCompleted: { + ICEBERG_RETURN_UNEXPECTED(ApplyStorageCredentials(result.storage_credentials)); auto tasks = ResolveScanTasks(result.plan_tasks, result.file_scan_tasks, specs); if (!tasks.has_value()) CancelPlanning(plan_id); return tasks; @@ -140,7 +141,7 @@ Result>> RestTableScan::PlanTableScan( return FetchPlanningResult(plan_id, specs); case PlanStatus::kFailed: return IOError("Scan planning failed: {}", - result.error ? result.error->message : "unknown error"); + result.error ? result.error->message : "unknown error"); case PlanStatus::kCancelled: return IOError("Scan planning was cancelled for plan_id={}", plan_id); } @@ -171,6 +172,7 @@ Result>> RestTableScan::FetchPlanningR switch (result.plan_status) { case PlanStatus::kCompleted: { + ICEBERG_RETURN_UNEXPECTED(ApplyStorageCredentials(result.storage_credentials)); auto tasks = ResolveScanTasks(result.plan_tasks, result.file_scan_tasks, specs); if (!tasks.has_value()) CancelPlanning(plan_id); return tasks; @@ -181,9 +183,8 @@ Result>> RestTableScan::FetchPlanningR .count(); if (elapsed_ms >= kMaxWaitTimeMs) { CancelPlanning(plan_id); - return IOError( - "Scan planning timed out after {}ms waiting for plan_id={}", elapsed_ms, - plan_id); + return IOError("Scan planning timed out after {}ms waiting for plan_id={}", + elapsed_ms, plan_id); } std::this_thread::sleep_for(std::chrono::milliseconds(delay_ms)); delay_ms = std::min(delay_ms * 2, kMaxSleepMs); @@ -192,15 +193,15 @@ Result>> RestTableScan::FetchPlanningR case PlanStatus::kFailed: CancelPlanning(plan_id); return IOError("Scan planning failed: {}", - result.error ? result.error->message : "unknown error"); + result.error ? result.error->message : "unknown error"); case PlanStatus::kCancelled: return IOError("Scan planning was cancelled for plan_id={}", plan_id); } } CancelPlanning(plan_id); - return IOError("Scan planning exceeded max retries ({}) for plan_id={}", - kMaxRetries, plan_id); + return IOError("Scan planning exceeded max retries ({}) for plan_id={}", kMaxRetries, + plan_id); } Result>> RestTableScan::FetchScanTasks( @@ -212,15 +213,15 @@ Result>> RestTableScan::FetchScanTasks rest_context_.paths->FetchScanTasks(rest_context_.identifier)); FetchScanTasksRequest request{.planTask = plan_task}; ICEBERG_ASSIGN_OR_RAISE(auto json_request, ToJsonString(ToJson(request))); - ICEBERG_ASSIGN_OR_RAISE( - const auto response, - rest_context_.client->Post(path, json_request, /*headers=*/{}, - *PlanTaskErrorHandler::Instance(), - *rest_context_.session)); + ICEBERG_ASSIGN_OR_RAISE(const auto response, + rest_context_.client->Post(path, json_request, /*headers=*/{}, + *PlanTaskErrorHandler::Instance(), + *rest_context_.session)); ICEBERG_ASSIGN_OR_RAISE(auto json, FromJsonString(response.body())); ICEBERG_ASSIGN_OR_RAISE(auto result, FetchScanTasksResponseFromJson(json, specs, *schema_)); ICEBERG_RETURN_UNEXPECTED(result.Validate()); + ICEBERG_RETURN_UNEXPECTED(ApplyStorageCredentials(result.storage_credentials)); return ResolveScanTasks(result.plan_tasks, result.file_scan_tasks, specs); } @@ -253,18 +254,31 @@ void RestTableScan::CancelPlanning(const std::string& plan_id) const { if (!path.has_value()) return; // Best-effort: ignore errors. - std::ignore = rest_context_.client->Delete(*path, /*params=*/{}, /*headers=*/{}, - *PlanErrorHandler::Instance(), - *rest_context_.session); + std::ignore = + rest_context_.client->Delete(*path, /*params=*/{}, /*headers=*/{}, + *PlanErrorHandler::Instance(), *rest_context_.session); +} + +const std::shared_ptr& RestTableScan::effective_io() const { + return scan_io_ ? scan_io_ : io_; +} + +Status RestTableScan::ApplyStorageCredentials( + const std::vector& credentials) const { + if (credentials.empty()) return {}; + ICEBERG_ASSIGN_OR_RAISE( + auto io, MakeTableFileIO(rest_context_.catalog_config, rest_context_.table_config, + credentials)); + scan_io_ = std::move(io); + return {}; } // RestTableScanBuilder -RestTableScanBuilder::RestTableScanBuilder(std::shared_ptr metadata, - std::shared_ptr io, - std::string table_name, - std::shared_ptr metrics_reporter, - RestScanContext rest_context) +RestTableScanBuilder::RestTableScanBuilder( + std::shared_ptr metadata, std::shared_ptr io, + std::string table_name, std::shared_ptr metrics_reporter, + RestScanContext rest_context) : DataTableScanBuilder(std::move(metadata), std::move(io), std::move(table_name), std::move(metrics_reporter)), rest_context_(std::move(rest_context)) {} diff --git a/src/iceberg/catalog/rest/rest_table_scan.h b/src/iceberg/catalog/rest/rest_table_scan.h index 2bc28a20e..63a852303 100644 --- a/src/iceberg/catalog/rest/rest_table_scan.h +++ b/src/iceberg/catalog/rest/rest_table_scan.h @@ -29,6 +29,7 @@ #include "iceberg/catalog/rest/iceberg_rest_export.h" #include "iceberg/metrics/metrics_reporter.h" #include "iceberg/result.h" +#include "iceberg/storage_credential.h" #include "iceberg/table_identifier.h" #include "iceberg/table_scan.h" #include "iceberg/type_fwd.h" @@ -52,6 +53,11 @@ struct ICEBERG_REST_EXPORT RestScanContext { std::shared_ptr session; std::unordered_set supported_endpoints; TableIdentifier identifier; + /// Catalog-level config, used with table_config to build a scan-scoped FileIO + /// when the server vends storage credentials in a planning response. + std::unordered_map catalog_config; + /// Table-level config merged with catalog_config for scan-scoped FileIO creation. + std::unordered_map table_config; }; /// \brief A DataTableScan that delegates PlanFiles() to the REST catalog server @@ -69,6 +75,13 @@ class ICEBERG_REST_EXPORT RestTableScan : public DataTableScan { /// \brief Plans files via the REST scan planning endpoints. Result>> PlanFiles() const override; + /// \brief Returns the FileIO to use when reading scan results. + /// + /// If the server vended storage credentials during planning, returns a FileIO + /// initialised with those credentials; otherwise returns the table's FileIO. + /// Must be called after PlanFiles(). + const std::shared_ptr& effective_io() const; + private: RestTableScan(std::shared_ptr metadata, std::shared_ptr schema, std::shared_ptr io, internal::TableScanContext context, @@ -98,14 +111,19 @@ class ICEBERG_REST_EXPORT RestTableScan : public DataTableScan { /// DELETE /plan/{plan_id}; best-effort, errors are silently ignored. void CancelPlanning(const std::string& plan_id) const; + /// Builds a scan-scoped FileIO from vended credentials and caches it in scan_io_. + /// No-op if credentials is empty. + Status ApplyStorageCredentials(const std::vector& credentials) const; + RestScanContext rest_context_; + mutable std::shared_ptr scan_io_; }; /// \brief Builder that produces a RestTableScan with the REST HTTP context injected. class ICEBERG_REST_EXPORT RestTableScanBuilder : public DataTableScanBuilder { public: - RestTableScanBuilder(std::shared_ptr metadata, std::shared_ptr io, - std::string table_name, + RestTableScanBuilder(std::shared_ptr metadata, + std::shared_ptr io, std::string table_name, std::shared_ptr metrics_reporter, RestScanContext rest_context); diff --git a/src/iceberg/catalog/rest/types.cc b/src/iceberg/catalog/rest/types.cc index 84fba9a7c..ef20f44da 100644 --- a/src/iceberg/catalog/rest/types.cc +++ b/src/iceberg/catalog/rest/types.cc @@ -210,6 +210,7 @@ bool OptionalSharedPtrVectorEqual( template bool ScanTaskFieldsEqual(const Response& lhs, const Response& rhs) { return lhs.plan_tasks == rhs.plan_tasks && + lhs.storage_credentials == rhs.storage_credentials && SharedPtrVectorEqual(lhs.delete_files, rhs.delete_files) && OptionalSharedPtrVectorEqual(lhs.file_scan_tasks, rhs.file_scan_tasks, FileScanTaskEqual); diff --git a/src/iceberg/catalog/rest/types.h b/src/iceberg/catalog/rest/types.h index 20a59fa59..ad58127c7 100644 --- a/src/iceberg/catalog/rest/types.h +++ b/src/iceberg/catalog/rest/types.h @@ -337,7 +337,7 @@ struct ICEBERG_REST_EXPORT PlanTableScanResponse { PlanStatus plan_status = PlanStatus::kCompleted; std::string plan_id; std::optional error; - // TODO(sandeepg): Add storage credentials and bind scan FileIO to them. + std::vector storage_credentials; Status Validate() const; @@ -352,7 +352,7 @@ struct ICEBERG_REST_EXPORT FetchPlanningResultResponse { std::vector> delete_files; PlanStatus plan_status = PlanStatus::kCompleted; std::optional error; - // TODO(sandeepg): Add storage credentials and bind scan FileIO to them. + std::vector storage_credentials; Status Validate() const; @@ -373,6 +373,7 @@ struct ICEBERG_REST_EXPORT FetchScanTasksResponse { std::optional> plan_tasks; std::optional>> file_scan_tasks; std::vector> delete_files; + std::vector storage_credentials; Status Validate() const; diff --git a/src/iceberg/test/rest_table_scan_test.cc b/src/iceberg/test/rest_table_scan_test.cc index 749b12dcb..9c5efd929 100644 --- a/src/iceberg/test/rest_table_scan_test.cc +++ b/src/iceberg/test/rest_table_scan_test.cc @@ -61,21 +61,21 @@ class MockHttpClient : public HttpClient { MOCK_METHOD(Result, Get, (const std::string& path, - (const std::unordered_map&) params, - (const std::unordered_map&) headers, + (const std::unordered_map&)params, + (const std::unordered_map&)headers, const ErrorHandler& error_handler, auth::AuthSession& session), (override)); MOCK_METHOD(Result, Post, (const std::string& path, const std::string& body, - (const std::unordered_map&) headers, + (const std::unordered_map&)headers, const ErrorHandler& error_handler, auth::AuthSession& session), (override)); MOCK_METHOD(Result, Delete, (const std::string& path, - (const std::unordered_map&) params, - (const std::unordered_map&) headers, + (const std::unordered_map&)params, + (const std::unordered_map&)headers, const ErrorHandler& error_handler, auth::AuthSession& session), (override)); }; @@ -85,8 +85,7 @@ class MockHttpClient : public HttpClient { // -------------------------------------------------------------------------- class NoOpFileIO : public FileIO { public: - Result ReadFile(const std::string&, - std::optional) override { + Result ReadFile(const std::string&, std::optional) override { return IOError("NoOpFileIO"); } Status WriteFile(const std::string&, std::string_view) override { return {}; } @@ -99,9 +98,9 @@ class NoOpFileIO : public FileIO { class RestTableScanTest : public ::testing::Test { protected: void SetUp() override { - schema_ = std::make_shared(std::vector{ - SchemaField::MakeRequired(1, "id", int32()), - SchemaField::MakeRequired(2, "data", string())}); + schema_ = std::make_shared( + std::vector{SchemaField::MakeRequired(1, "id", int32()), + SchemaField::MakeRequired(2, "data", string())}); auto spec = PartitionSpec::Unpartitioned(); @@ -113,36 +112,35 @@ class RestTableScanTest : public ::testing::Test { .manifest_list = "/tmp/manifest-list.avro", .schema_id = schema_->schema_id()}); - metadata_ = std::make_shared(TableMetadata{ - .format_version = 2, - .table_uuid = "test-uuid", - .location = "/tmp/table", - .last_sequence_number = 1L, - .last_updated_ms = TimePointMsFromUnixMs(1609459200000L), - .last_column_id = 2, - .schemas = {schema_}, - .current_schema_id = schema_->schema_id(), - .partition_specs = {spec}, - .default_spec_id = spec->spec_id(), - .last_partition_id = 999, - .current_snapshot_id = kSnapshotId, - .snapshots = {snapshot}, - .refs = {{"main", - std::make_shared(SnapshotRef{ - .snapshot_id = kSnapshotId, - .retention = SnapshotRef::Branch{}})}}}); + metadata_ = std::make_shared( + TableMetadata{.format_version = 2, + .table_uuid = "test-uuid", + .location = "/tmp/table", + .last_sequence_number = 1L, + .last_updated_ms = TimePointMsFromUnixMs(1609459200000L), + .last_column_id = 2, + .schemas = {schema_}, + .current_schema_id = schema_->schema_id(), + .partition_specs = {spec}, + .default_spec_id = spec->spec_id(), + .last_partition_id = 999, + .current_snapshot_id = kSnapshotId, + .snapshots = {snapshot}, + .refs = {{"main", std::make_shared(SnapshotRef{ + .snapshot_id = kSnapshotId, + .retention = SnapshotRef::Branch{}})}}}); file_io_ = std::make_shared(); mock_client_ = std::make_shared(); - ICEBERG_UNWRAP_OR_FAIL(paths_, ResourcePaths::Make( - "http://test-server", /*prefix=*/"", - /*namespace_separator=*/"%1F")); + ICEBERG_UNWRAP_OR_FAIL(paths_, + ResourcePaths::Make("http://test-server", /*prefix=*/"", + /*namespace_separator=*/"%1F")); session_ = auth::AuthSession::MakeDefault(/*headers=*/{}); - identifier_ = TableIdentifier{Namespace{{"default"}}, "my_table"}; + identifier_ = TableIdentifier{.ns = Namespace{{"default"}}, .name = "my_table"}; all_plan_endpoints_ = {Endpoint::PlanTableScan(), Endpoint::FetchPlanningResult(), Endpoint::CancelPlanning(), Endpoint::FetchScanTasks()}; @@ -152,8 +150,7 @@ class RestTableScanTest : public ::testing::Test { // Pass an explicit set (including empty) to use exactly that set. RestScanContext MakeContext( std::optional> endpoints = std::nullopt) { - auto effective = - endpoints.has_value() ? std::move(*endpoints) : all_plan_endpoints_; + auto effective = endpoints.has_value() ? std::move(*endpoints) : all_plan_endpoints_; return RestScanContext{ .client = mock_client_, .paths = paths_, @@ -164,8 +161,8 @@ class RestTableScanTest : public ::testing::Test { } Result> MakeScan(RestScanContext ctx) { - return RestTableScan::Make(metadata_, schema_, file_io_, - internal::TableScanContext{}, std::move(ctx)); + return RestTableScan::Make(metadata_, schema_, file_io_, internal::TableScanContext{}, + std::move(ctx)); } std::shared_ptr schema_; @@ -241,8 +238,8 @@ TEST_F(RestTableScanTest, PlanFilesFailed) { // PlanFiles: PlanTableScan endpoint missing → NotSupported error. // -------------------------------------------------------------------------- TEST_F(RestTableScanTest, PlanFilesEndpointNotSupported) { - ICEBERG_UNWRAP_OR_FAIL( - auto scan, MakeScan(MakeContext(std::unordered_set{}))); + ICEBERG_UNWRAP_OR_FAIL(auto scan, + MakeScan(MakeContext(std::unordered_set{}))); auto result = scan->PlanFiles(); EXPECT_THAT(result, IsError(ErrorKind::kNotSupported)); } @@ -254,7 +251,8 @@ TEST_F(RestTableScanTest, PlanFilesEndpointNotSupported) { TEST_F(RestTableScanTest, PlanFilesWithPlanTasks) { constexpr std::string_view kPlanResponse = R"({"status":"completed","plan-id":"plan-1","plan-tasks":["tok-1"]})"; - // FetchScanTasksResponse requires at least one of plan-tasks or file-scan-tasks present. + // FetchScanTasksResponse requires at least one of plan-tasks or file-scan-tasks + // present. constexpr std::string_view kTasksResponse = R"({"file-scan-tasks":[]})"; EXPECT_CALL(*mock_client_, Post(_, _, _, _, _)) @@ -318,8 +316,7 @@ TEST_F(RestTableScanTest, CancelIsNoOpWhenEndpointNotAdvertised) { .WillOnce(Return(IOError("FetchScanTasks failed"))); EXPECT_CALL(*mock_client_, Delete(_, _, _, _, _)).Times(0); - ICEBERG_UNWRAP_OR_FAIL(auto scan, - MakeScan(MakeContext(endpoints_without_cancel))); + ICEBERG_UNWRAP_OR_FAIL(auto scan, MakeScan(MakeContext(endpoints_without_cancel))); auto result = scan->PlanFiles(); EXPECT_THAT(result, IsError(ErrorKind::kIOError)); } @@ -332,14 +329,12 @@ TEST_F(RestTableScanTest, FetchPlanningResultEndpointNotSupported) { R"({"status":"submitted","plan-id":"plan-3"})"; std::unordered_set endpoints_without_fetch = { - Endpoint::PlanTableScan(), Endpoint::CancelPlanning(), - Endpoint::FetchScanTasks()}; + Endpoint::PlanTableScan(), Endpoint::CancelPlanning(), Endpoint::FetchScanTasks()}; EXPECT_CALL(*mock_client_, Post(_, _, _, _, _)) .WillOnce(Return(HttpResponse::MakeForTesting(200, std::string(kSubmittedBody)))); - ICEBERG_UNWRAP_OR_FAIL(auto scan, - MakeScan(MakeContext(endpoints_without_fetch))); + ICEBERG_UNWRAP_OR_FAIL(auto scan, MakeScan(MakeContext(endpoints_without_fetch))); auto result = scan->PlanFiles(); EXPECT_THAT(result, IsError(ErrorKind::kNotSupported)); } @@ -351,17 +346,16 @@ TEST_F(RestTableScanTest, FetchScanTasksEndpointNotSupported) { constexpr std::string_view kPlanResponse = R"({"status":"completed","plan-id":"plan-4","plan-tasks":["tok-d"]})"; - std::unordered_set endpoints_without_tasks = { - Endpoint::PlanTableScan(), Endpoint::FetchPlanningResult(), - Endpoint::CancelPlanning()}; + std::unordered_set endpoints_without_tasks = {Endpoint::PlanTableScan(), + Endpoint::FetchPlanningResult(), + Endpoint::CancelPlanning()}; EXPECT_CALL(*mock_client_, Post(_, _, _, _, _)) .WillOnce(Return(HttpResponse::MakeForTesting(200, std::string(kPlanResponse)))); EXPECT_CALL(*mock_client_, Delete(_, _, _, _, _)) .WillOnce(Return(HttpResponse::MakeForTesting(200, "{}"))); - ICEBERG_UNWRAP_OR_FAIL(auto scan, - MakeScan(MakeContext(endpoints_without_tasks))); + ICEBERG_UNWRAP_OR_FAIL(auto scan, MakeScan(MakeContext(endpoints_without_tasks))); auto result = scan->PlanFiles(); EXPECT_THAT(result, IsError(ErrorKind::kNotSupported)); } @@ -389,15 +383,82 @@ TEST_F(RestTableScanTest, DefaultScanDoesNotSetUseSnapshotSchema) { EXPECT_FALSE(scan->context().use_snapshot_schema); } +// -------------------------------------------------------------------------- +// Storage credentials in COMPLETED response: effective_io() returns a +// credential-scoped IO, not the original table IO. +// -------------------------------------------------------------------------- +TEST_F(RestTableScanTest, StorageCredentialsInPlanResponseUpdatesEffectiveIO) { + constexpr std::string_view kResponseBody = R"({ + "status": "completed", + "storage-credentials": [ + {"prefix": "s3://bucket/prefix", "config": {"key": "value"}} + ] + })"; + EXPECT_CALL(*mock_client_, Post(_, _, _, _, _)) + .WillOnce(Return(HttpResponse::MakeForTesting(200, std::string(kResponseBody)))); + + ICEBERG_UNWRAP_OR_FAIL(auto scan, MakeScan(MakeContext())); + ICEBERG_UNWRAP_OR_FAIL(auto tasks, scan->PlanFiles()); + EXPECT_TRUE(tasks.empty()); + + auto* rest_scan = dynamic_cast(scan.get()); + ASSERT_NE(rest_scan, nullptr); + // effective_io() must return a credential-scoped IO, not the original file_io_. + EXPECT_NE(rest_scan->effective_io().get(), file_io_.get()); +} + +// -------------------------------------------------------------------------- +// No storage credentials: effective_io() falls back to the table's FileIO. +// -------------------------------------------------------------------------- +TEST_F(RestTableScanTest, NoStorageCredentialsEffectiveIoFallsBackToTableIO) { + constexpr std::string_view kResponseBody = R"({"status":"completed"})"; + EXPECT_CALL(*mock_client_, Post(_, _, _, _, _)) + .WillOnce(Return(HttpResponse::MakeForTesting(200, std::string(kResponseBody)))); + + ICEBERG_UNWRAP_OR_FAIL(auto scan, MakeScan(MakeContext())); + ICEBERG_UNWRAP_OR_FAIL(auto tasks, scan->PlanFiles()); + EXPECT_TRUE(tasks.empty()); + + auto* rest_scan = dynamic_cast(scan.get()); + ASSERT_NE(rest_scan, nullptr); + EXPECT_EQ(rest_scan->effective_io().get(), file_io_.get()); +} + +// -------------------------------------------------------------------------- +// Storage credentials returned in FetchScanTasksResponse also update +// effective_io(). +// -------------------------------------------------------------------------- +TEST_F(RestTableScanTest, StorageCredentialsInFetchScanTasksResponseUpdatesEffectiveIO) { + constexpr std::string_view kPlanResponse = + R"({"status":"completed","plan-id":"plan-cred","plan-tasks":["tok-cred"]})"; + constexpr std::string_view kTasksResponse = R"({ + "file-scan-tasks": [], + "storage-credentials": [ + {"prefix": "s3://bucket/prefix", "config": {"key": "value"}} + ] + })"; + + EXPECT_CALL(*mock_client_, Post(_, _, _, _, _)) + .WillOnce(Return(HttpResponse::MakeForTesting(200, std::string(kPlanResponse)))) + .WillOnce(Return(HttpResponse::MakeForTesting(200, std::string(kTasksResponse)))); + + ICEBERG_UNWRAP_OR_FAIL(auto scan, MakeScan(MakeContext())); + ICEBERG_UNWRAP_OR_FAIL(auto tasks, scan->PlanFiles()); + EXPECT_TRUE(tasks.empty()); + + auto* rest_scan = dynamic_cast(scan.get()); + ASSERT_NE(rest_scan, nullptr); + EXPECT_NE(rest_scan->effective_io().get(), file_io_.get()); +} + // -------------------------------------------------------------------------- // RestTable::NewScan returns a RestTableScanBuilder (not a plain builder). // -------------------------------------------------------------------------- TEST_F(RestTableScanTest, RestTableNewScanReturnsRestTableScanBuilder) { ICEBERG_UNWRAP_OR_FAIL( - auto table, - RestTable::Make(identifier_, metadata_, "/tmp/metadata.json", file_io_, - /*catalog=*/nullptr, "test.my_table", nullptr, - MakeContext(std::nullopt))); + auto table, RestTable::Make(identifier_, metadata_, "/tmp/metadata.json", file_io_, + /*catalog=*/nullptr, "test.my_table", nullptr, + MakeContext(std::nullopt))); ICEBERG_UNWRAP_OR_FAIL(auto builder, table->NewScan()); auto* typed = dynamic_cast(builder.get()); EXPECT_NE(typed, nullptr); diff --git a/src/iceberg/test/table_scan_test.cc b/src/iceberg/test/table_scan_test.cc index 88abbaef5..8deada17d 100644 --- a/src/iceberg/test/table_scan_test.cc +++ b/src/iceberg/test/table_scan_test.cc @@ -845,8 +845,7 @@ TEST_P(TableScanTest, SchemaWithSelectedColumnsAndFilter) { // UseSnapshot, UseRef (tag vs branch), and default/incremental scans. TEST_P(TableScanTest, UseSnapshotSetsTrueUseSnapshotSchema) { constexpr int64_t kSnapshotId = 1000L; - ICEBERG_UNWRAP_OR_FAIL(auto builder, - DataTableScanBuilder::Make(table_metadata_, file_io_)); + ICEBERG_UNWRAP_OR_FAIL(auto builder, MakeScanBuilder(table_metadata_)); builder->UseSnapshot(kSnapshotId); ICEBERG_UNWRAP_OR_FAIL(auto scan, builder->Build()); EXPECT_TRUE(scan->context().use_snapshot_schema); @@ -857,29 +856,25 @@ TEST_P(TableScanTest, UseRefTagSetsTrueUseSnapshotSchema) { table_metadata_->refs["v1.0"] = std::make_shared( SnapshotRef{.snapshot_id = kSnapshotId, .retention = SnapshotRef::Tag{}}); - ICEBERG_UNWRAP_OR_FAIL(auto builder, - DataTableScanBuilder::Make(table_metadata_, file_io_)); + ICEBERG_UNWRAP_OR_FAIL(auto builder, MakeScanBuilder(table_metadata_)); builder->UseRef("v1.0"); ICEBERG_UNWRAP_OR_FAIL(auto scan, builder->Build()); EXPECT_TRUE(scan->context().use_snapshot_schema); } TEST_P(TableScanTest, UseRefBranchSetsFalseUseSnapshotSchema) { - ICEBERG_UNWRAP_OR_FAIL(auto builder, - DataTableScanBuilder::Make(table_metadata_, file_io_)); + ICEBERG_UNWRAP_OR_FAIL(auto builder, MakeScanBuilder(table_metadata_)); builder->UseRef("main"); ICEBERG_UNWRAP_OR_FAIL(auto scan, builder->Build()); EXPECT_FALSE(scan->context().use_snapshot_schema); } TEST_P(TableScanTest, DefaultScanHasFalseUseSnapshotSchema) { - ICEBERG_UNWRAP_OR_FAIL(auto builder, - DataTableScanBuilder::Make(table_metadata_, file_io_)); + ICEBERG_UNWRAP_OR_FAIL(auto builder, MakeScanBuilder(table_metadata_)); ICEBERG_UNWRAP_OR_FAIL(auto scan, builder->Build()); EXPECT_FALSE(scan->context().use_snapshot_schema); } - INSTANTIATE_TEST_SUITE_P(TableScanVersions, TableScanTest, testing::Values(1, 2, 3)); } // namespace iceberg From 92ba0d76746e6825e8d2eda96a0300932435dac8 Mon Sep 17 00:00:00 2001 From: Sandeep Gottimukkala Date: Sun, 13 Sep 2026 15:58:17 -0700 Subject: [PATCH 05/16] style: apply clang-format to fix CI pre-commit failures --- src/iceberg/catalog/rest/http_client.h | 14 ++++++-------- src/iceberg/catalog/rest/rest_table.cc | 16 ++++++---------- src/iceberg/catalog/rest/rest_table.h | 13 +++++-------- src/iceberg/test/catalog_properties_test.cc | 20 ++++++++++---------- 4 files changed, 27 insertions(+), 36 deletions(-) diff --git a/src/iceberg/catalog/rest/http_client.h b/src/iceberg/catalog/rest/http_client.h index 52522425e..d42798fae 100644 --- a/src/iceberg/catalog/rest/http_client.h +++ b/src/iceberg/catalog/rest/http_client.h @@ -83,16 +83,15 @@ class ICEBERG_REST_EXPORT HttpClient { /// \brief Sends a GET request. virtual Result Get( - const std::string& path, - const std::unordered_map& params, + const std::string& path, const std::unordered_map& params, const std::unordered_map& headers, const ErrorHandler& error_handler, auth::AuthSession& session); /// \brief Sends a POST request. - virtual Result Post(const std::string& path, const std::string& body, - const std::unordered_map& headers, - const ErrorHandler& error_handler, - auth::AuthSession& session); + virtual Result Post( + const std::string& path, const std::string& body, + const std::unordered_map& headers, + const ErrorHandler& error_handler, auth::AuthSession& session); /// \brief Sends a POST request with form data. virtual Result PostForm( @@ -109,8 +108,7 @@ class ICEBERG_REST_EXPORT HttpClient { /// \brief Sends a DELETE request. virtual Result Delete( - const std::string& path, - const std::unordered_map& params, + const std::string& path, const std::unordered_map& params, const std::unordered_map& headers, const ErrorHandler& error_handler, auth::AuthSession& session); diff --git a/src/iceberg/catalog/rest/rest_table.cc b/src/iceberg/catalog/rest/rest_table.cc index 727444443..6fb330670 100644 --- a/src/iceberg/catalog/rest/rest_table.cc +++ b/src/iceberg/catalog/rest/rest_table.cc @@ -35,20 +35,16 @@ RestTable::RestTable(TableIdentifier identifier, std::shared_ptr std::shared_ptr reporter, RestScanContext rest_context) : Table(std::move(identifier), std::move(metadata), std::move(metadata_location), - std::move(io), std::move(catalog), std::move(full_name), - std::move(reporter)), + std::move(io), std::move(catalog), std::move(full_name), std::move(reporter)), rest_context_(std::move(rest_context)) {} RestTable::~RestTable() = default; -Result> RestTable::Make(TableIdentifier identifier, - std::shared_ptr metadata, - std::string metadata_location, - std::shared_ptr io, - std::shared_ptr catalog, - std::string full_name, - std::shared_ptr reporter, - RestScanContext rest_context) { +Result> RestTable::Make( + TableIdentifier identifier, std::shared_ptr metadata, + std::string metadata_location, std::shared_ptr io, + std::shared_ptr catalog, std::string full_name, + std::shared_ptr reporter, RestScanContext rest_context) { if (metadata == nullptr) { return InvalidArgument("Metadata cannot be null"); } diff --git a/src/iceberg/catalog/rest/rest_table.h b/src/iceberg/catalog/rest/rest_table.h index 54208d26a..fb23ca66b 100644 --- a/src/iceberg/catalog/rest/rest_table.h +++ b/src/iceberg/catalog/rest/rest_table.h @@ -38,14 +38,11 @@ namespace iceberg::rest { /// PlanFiles() to the REST catalog server's scan planning endpoints. class ICEBERG_REST_EXPORT RestTable final : public Table { public: - static Result> Make(TableIdentifier identifier, - std::shared_ptr metadata, - std::string metadata_location, - std::shared_ptr io, - std::shared_ptr catalog, - std::string full_name, - std::shared_ptr reporter, - RestScanContext rest_context); + static Result> Make( + TableIdentifier identifier, std::shared_ptr metadata, + std::string metadata_location, std::shared_ptr io, + std::shared_ptr catalog, std::string full_name, + std::shared_ptr reporter, RestScanContext rest_context); ~RestTable() override; diff --git a/src/iceberg/test/catalog_properties_test.cc b/src/iceberg/test/catalog_properties_test.cc index 47abe7682..5f22e60d4 100644 --- a/src/iceberg/test/catalog_properties_test.cc +++ b/src/iceberg/test/catalog_properties_test.cc @@ -33,40 +33,40 @@ TEST(ScanPlanningModeTest, MissingKeyReturnsNullopt) { } TEST(ScanPlanningModeTest, ClientLowercaseReturnsKClient) { - auto result = RestCatalogProperties::ScanPlanningModeFrom( - {{"scan-planning-mode", "client"}}); + auto result = + RestCatalogProperties::ScanPlanningModeFrom({{"scan-planning-mode", "client"}}); ASSERT_THAT(result, IsOk()); ASSERT_TRUE(result->has_value()); EXPECT_EQ(**result, ScanPlanningMode::kClient); } TEST(ScanPlanningModeTest, ServerLowercaseReturnsKServer) { - auto result = RestCatalogProperties::ScanPlanningModeFrom( - {{"scan-planning-mode", "server"}}); + auto result = + RestCatalogProperties::ScanPlanningModeFrom({{"scan-planning-mode", "server"}}); ASSERT_THAT(result, IsOk()); ASSERT_TRUE(result->has_value()); EXPECT_EQ(**result, ScanPlanningMode::kServer); } TEST(ScanPlanningModeTest, ClientUppercaseReturnsKClient) { - auto result = RestCatalogProperties::ScanPlanningModeFrom( - {{"scan-planning-mode", "CLIENT"}}); + auto result = + RestCatalogProperties::ScanPlanningModeFrom({{"scan-planning-mode", "CLIENT"}}); ASSERT_THAT(result, IsOk()); ASSERT_TRUE(result->has_value()); EXPECT_EQ(**result, ScanPlanningMode::kClient); } TEST(ScanPlanningModeTest, ServerUppercaseReturnsKServer) { - auto result = RestCatalogProperties::ScanPlanningModeFrom( - {{"scan-planning-mode", "SERVER"}}); + auto result = + RestCatalogProperties::ScanPlanningModeFrom({{"scan-planning-mode", "SERVER"}}); ASSERT_THAT(result, IsOk()); ASSERT_TRUE(result->has_value()); EXPECT_EQ(**result, ScanPlanningMode::kServer); } TEST(ScanPlanningModeTest, InvalidValueReturnsError) { - auto result = RestCatalogProperties::ScanPlanningModeFrom( - {{"scan-planning-mode", "invalid"}}); + auto result = + RestCatalogProperties::ScanPlanningModeFrom({{"scan-planning-mode", "invalid"}}); EXPECT_THAT(result, IsError(ErrorKind::kInvalidArgument)); } From 25ebc656d62e398228c7e536be32bf5f6997e491 Mon Sep 17 00:00:00 2001 From: Sandeep Gottimukkala Date: Sun, 13 Sep 2026 16:01:55 -0700 Subject: [PATCH 06/16] fix: export TableScanContext so Validate() is linkable from iceberg_rest RestTableScanBuilder::Build() calls context_.Validate() across the iceberg_rest/iceberg library boundary. Without ICEBERG_EXPORT on TableScanContext the symbol is hidden in the shared library and the linker fails on arm64. --- src/iceberg/table_scan.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/iceberg/table_scan.h b/src/iceberg/table_scan.h index 4eb3e0fb2..7d4765aed 100644 --- a/src/iceberg/table_scan.h +++ b/src/iceberg/table_scan.h @@ -219,7 +219,7 @@ class ICEBERG_EXPORT DeletedDataFileScanTask : public ChangelogScanTask { namespace internal { // Internal table scan context used by different scan implementations. -struct TableScanContext { +struct ICEBERG_EXPORT TableScanContext { std::optional snapshot_id; std::shared_ptr filter; bool ignore_residuals{false}; From b9e3aafc0d7f96c18ccf7a61abd39f398bfbd57c Mon Sep 17 00:00:00 2001 From: Sandeep Gottimukkala Date: Sun, 13 Sep 2026 16:04:13 -0700 Subject: [PATCH 07/16] fix: explicitly default TableScanBuilder move ctor for Windows DLL export MSVC does not export the implicitly-generated move constructor of a template class instantiation. RestTableScanBuilder (introduced in iceberg_rest) is exported with ICEBERG_REST_EXPORT, so its compiler- generated move constructor must call the base TableScanBuilder move constructor as an imported symbol. Explicitly defaulting it makes it part of the explicit template instantiation and therefore exported. --- src/iceberg/table_scan.h | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/iceberg/table_scan.h b/src/iceberg/table_scan.h index 7d4765aed..1b298cd33 100644 --- a/src/iceberg/table_scan.h +++ b/src/iceberg/table_scan.h @@ -410,6 +410,9 @@ class ICEBERG_TEMPLATE_CLASS_EXPORT TableScanBuilder : public ErrorCollector { std::string table_name, std::shared_ptr metrics_reporter); + TableScanBuilder(TableScanBuilder&&) = default; + TableScanBuilder& operator=(TableScanBuilder&&) = default; + // Return the schema bound to the specified snapshot. Result>> ResolveSnapshotSchema(); Status ResolveColumnStatsSelection(); From 5de03445d2930ad0ae9f641046a56461d4b19a70 Mon Sep 17 00:00:00 2001 From: Sandeep Gottimukkala Date: Mon, 28 Sep 2026 20:20:15 -0700 Subject: [PATCH 08/16] feat(rest): server-side planning for incremental append scans, PlanFilesStream, and virtual io() - Add RestIncrementalAppendScan and RestIncrementalAppendScanBuilder; override RestTable::NewIncrementalAppendScan() to delegate PlanFiles() to the REST server via the same scan planning endpoints. IncrementalChangelogScan is left local: the planTableScan response only carries FileScanTask objects, not the per-snapshot operation metadata needed to reconstruct ChangelogScanTask entries. Refactor shared HTTP planning logic into free functions (ExecuteScanPlan, FetchPlanningResult, FetchScanTasks, ResolveScanTasks, ApplyStorageCredentials, CancelPlanning) used by both scan types. - Make DataTableScan::PlanFilesStream() virtual and implement it in RestTableScan with a lazy RestFileScanTaskStream that drives FetchScanTasks per plan_task token. The base-class PlanFiles() delegates to PlanFilesStream()->ToVector(), so the server planning path is used for both the eager and streaming APIs. - Make TableScan::io() virtual and override it in RestTableScan to return the credential-scoped FileIO when the server vends storage credentials, without requiring a downcast. Remove effective_io(). A shared ScanIoSlot (shared_ptr>) is held by both the scan and the lazy stream so credentials vended by any FetchScanTasks response are visible through io() even after the stream is consumed. Update tests accordingly. --- src/iceberg/catalog/rest/rest_table.cc | 6 + src/iceberg/catalog/rest/rest_table.h | 4 + src/iceberg/catalog/rest/rest_table_scan.cc | 503 ++++++++++++++------ src/iceberg/catalog/rest/rest_table_scan.h | 90 ++-- src/iceberg/table_scan.h | 4 +- src/iceberg/test/rest_table_scan_test.cc | 21 +- 6 files changed, 436 insertions(+), 192 deletions(-) diff --git a/src/iceberg/catalog/rest/rest_table.cc b/src/iceberg/catalog/rest/rest_table.cc index 6fb330670..dd2b43b96 100644 --- a/src/iceberg/catalog/rest/rest_table.cc +++ b/src/iceberg/catalog/rest/rest_table.cc @@ -59,4 +59,10 @@ Result> RestTable::NewScan() const { rest_context_); } +Result> +RestTable::NewIncrementalAppendScan() const { + return std::make_unique(metadata_, io_, full_name_, + reporter_, rest_context_); +} + } // namespace iceberg::rest diff --git a/src/iceberg/catalog/rest/rest_table.h b/src/iceberg/catalog/rest/rest_table.h index fb23ca66b..f47dbe5b3 100644 --- a/src/iceberg/catalog/rest/rest_table.h +++ b/src/iceberg/catalog/rest/rest_table.h @@ -50,6 +50,10 @@ class ICEBERG_REST_EXPORT RestTable final : public Table { /// REST catalog server. Result> NewScan() const override; + /// \brief Returns a RestIncrementalAppendScanBuilder that delegates to the server. + Result> NewIncrementalAppendScan() + const override; + private: RestTable(TableIdentifier identifier, std::shared_ptr metadata, std::string metadata_location, std::shared_ptr io, diff --git a/src/iceberg/catalog/rest/rest_table_scan.cc b/src/iceberg/catalog/rest/rest_table_scan.cc index 3e11c71dc..5a346b03e 100644 --- a/src/iceberg/catalog/rest/rest_table_scan.cc +++ b/src/iceberg/catalog/rest/rest_table_scan.cc @@ -35,6 +35,7 @@ #include "iceberg/partition_spec.h" #include "iceberg/result.h" #include "iceberg/schema.h" +#include "iceberg/snapshot.h" #include "iceberg/table_metadata.h" #include "iceberg/util/macros.h" @@ -47,6 +48,12 @@ constexpr int64_t kMaxSleepMs = 60'000; constexpr int kMaxRetries = 10; constexpr int64_t kMaxWaitTimeMs = 5 * 60 * 1'000; +using SpecsById = std::unordered_map>; +// A shared slot that lets the scan and the stream share a single FileIO reference. +// Credentials vended by any lazy FetchScanTasks response are written into *slot, +// making them visible to RestTableScan::io() even after the stream is consumed. +using ScanIoSlot = std::shared_ptr>; + #define ICEBERG_ENDPOINT_CHECK(endpoints, endpoint) \ do { \ if (!endpoints.contains(endpoint)) { \ @@ -54,108 +61,83 @@ constexpr int64_t kMaxWaitTimeMs = 5 * 60 * 1'000; } \ } while (0) -} // namespace - -// RestTableScan - -RestTableScan::RestTableScan(std::shared_ptr metadata, - std::shared_ptr schema, std::shared_ptr io, - internal::TableScanContext context, - RestScanContext rest_context) - : DataTableScan(std::move(metadata), std::move(schema), std::move(io), - std::move(context)), - rest_context_(std::move(rest_context)) {} +// --------------------------------------------------------------------------- +// Shared HTTP scan planning helpers used by all REST scan implementations. +// --------------------------------------------------------------------------- -Result> RestTableScan::Make( - std::shared_ptr metadata, std::shared_ptr schema, - std::shared_ptr io, internal::TableScanContext context, - RestScanContext rest_context) { - ICEBERG_PRECHECK(metadata != nullptr, "Table metadata cannot be null"); - ICEBERG_PRECHECK(schema != nullptr, "Schema cannot be null"); - ICEBERG_PRECHECK(io != nullptr, "FileIO cannot be null"); - return std::unique_ptr( - new RestTableScan(std::move(metadata), std::move(schema), std::move(io), - std::move(context), std::move(rest_context))); +Status ApplyStorageCredentials(const RestScanContext& ctx, + const std::vector& credentials, + std::shared_ptr& scan_io) { + if (credentials.empty()) return {}; + ICEBERG_ASSIGN_OR_RAISE( + auto io, MakeTableFileIO(ctx.catalog_config, ctx.table_config, credentials)); + scan_io = std::move(io); + return {}; } -Result>> RestTableScan::PlanFiles() const { - TableMetadataCache metadata_cache(metadata_.get()); - ICEBERG_ASSIGN_OR_RAISE(auto specs_by_id, metadata_cache.GetPartitionSpecsById()); +void CancelPlanning(const RestScanContext& ctx, const std::string& plan_id) { + if (plan_id.empty()) return; + if (!ctx.supported_endpoints.contains(Endpoint::CancelPlanning())) return; + + auto path = ctx.paths->Plan(ctx.identifier, plan_id); + if (!path.has_value()) return; - std::string plan_id; - return PlanTableScan(plan_id, specs_by_id); + std::ignore = ctx.client->Delete(*path, /*params=*/{}, /*headers=*/{}, + *PlanErrorHandler::Instance(), *ctx.session); } -Result>> RestTableScan::PlanTableScan( - std::string& plan_id, - const std::unordered_map>& specs) const { - ICEBERG_ENDPOINT_CHECK(rest_context_.supported_endpoints, Endpoint::PlanTableScan()); +Result>> FetchScanTasks( + const RestScanContext& ctx, const Schema& schema, const std::string& plan_task, + const SpecsById& specs, std::shared_ptr& scan_io); - // Build request from scan context - PlanTableScanRequest request; - request.select = context_.selected_columns; - request.filter = context_.filter; - request.case_sensitive = context_.case_sensitive; - request.min_rows_requested = context_.min_rows_requested; +Result>> ResolveScanTasks( + const RestScanContext& ctx, const Schema& schema, + const std::optional>& plan_tasks, + const std::optional>>& file_scan_tasks, + const SpecsById& specs, std::shared_ptr& scan_io) { + std::vector> result; - if (context_.from_snapshot_id.has_value() && context_.to_snapshot_id.has_value()) { - request.start_snapshot_id = context_.from_snapshot_id; - request.end_snapshot_id = context_.to_snapshot_id; - request.use_snapshot_schema = true; - } else if (context_.snapshot_id.has_value()) { - request.snapshot_id = context_.snapshot_id; - request.use_snapshot_schema = context_.use_snapshot_schema; + if (file_scan_tasks.has_value()) { + result.insert(result.end(), file_scan_tasks->begin(), file_scan_tasks->end()); } - if (!context_.columns_to_keep_stats.empty()) { - for (int32_t field_id : context_.columns_to_keep_stats) { - ICEBERG_ASSIGN_OR_RAISE(auto name, schema_->FindColumnNameById(field_id)); - if (name.has_value()) { - request.stats_fields.emplace_back(*name); - } + if (plan_tasks.has_value()) { + for (const auto& token : *plan_tasks) { + ICEBERG_ASSIGN_OR_RAISE(auto tasks, FetchScanTasks(ctx, schema, token, specs, scan_io)); + result.insert(result.end(), tasks.begin(), tasks.end()); } } - ICEBERG_ASSIGN_OR_RAISE(auto path, rest_context_.paths->Plan(rest_context_.identifier)); - ICEBERG_ASSIGN_OR_RAISE(auto request_json, ToJson(request)); - ICEBERG_ASSIGN_OR_RAISE(auto json_request, ToJsonString(request_json)); + return result; +} + +Result>> FetchScanTasks( + const RestScanContext& ctx, const Schema& schema, const std::string& plan_task, + const SpecsById& specs, std::shared_ptr& scan_io) { + ICEBERG_ENDPOINT_CHECK(ctx.supported_endpoints, Endpoint::FetchScanTasks()); + + ICEBERG_ASSIGN_OR_RAISE(auto path, ctx.paths->FetchScanTasks(ctx.identifier)); + FetchScanTasksRequest request{.planTask = plan_task}; + ICEBERG_ASSIGN_OR_RAISE(auto json_request, ToJsonString(ToJson(request))); ICEBERG_ASSIGN_OR_RAISE( const auto response, - rest_context_.client->Post(path, json_request, /*headers=*/{}, - *PlanErrorHandler::Instance(), *rest_context_.session)); + ctx.client->Post(path, json_request, /*headers=*/{}, + *PlanTaskErrorHandler::Instance(), *ctx.session)); ICEBERG_ASSIGN_OR_RAISE(auto json, FromJsonString(response.body())); - ICEBERG_ASSIGN_OR_RAISE(auto result, - PlanTableScanResponseFromJson(json, specs, *schema_)); + ICEBERG_ASSIGN_OR_RAISE(auto result, FetchScanTasksResponseFromJson(json, specs, schema)); ICEBERG_RETURN_UNEXPECTED(result.Validate()); + ICEBERG_RETURN_UNEXPECTED(ApplyStorageCredentials(ctx, result.storage_credentials, scan_io)); - plan_id = result.plan_id; - - switch (result.plan_status) { - case PlanStatus::kCompleted: { - ICEBERG_RETURN_UNEXPECTED(ApplyStorageCredentials(result.storage_credentials)); - auto tasks = ResolveScanTasks(result.plan_tasks, result.file_scan_tasks, specs); - if (!tasks.has_value()) CancelPlanning(plan_id); - return tasks; - } - case PlanStatus::kSubmitted: - return FetchPlanningResult(plan_id, specs); - case PlanStatus::kFailed: - return IOError("Scan planning failed: {}", - result.error ? result.error->message : "unknown error"); - case PlanStatus::kCancelled: - return IOError("Scan planning was cancelled for plan_id={}", plan_id); - } - return IOError("Unexpected plan status"); + return ResolveScanTasks(ctx, schema, result.plan_tasks, result.file_scan_tasks, specs, + scan_io); } -Result>> RestTableScan::FetchPlanningResult( - const std::string& plan_id, - const std::unordered_map>& specs) const { - ICEBERG_ENDPOINT_CHECK(rest_context_.supported_endpoints, - Endpoint::FetchPlanningResult()); +Result>> FetchPlanningResult( + const RestScanContext& ctx, const Schema& schema, const std::string& plan_id, + const SpecsById& specs, std::shared_ptr& scan_io) { + ICEBERG_ENDPOINT_CHECK(ctx.supported_endpoints, Endpoint::FetchPlanningResult()); - ICEBERG_ASSIGN_OR_RAISE(auto path, - rest_context_.paths->Plan(rest_context_.identifier, plan_id)); + ICEBERG_ASSIGN_OR_RAISE(auto path, ctx.paths->Plan(ctx.identifier, plan_id)); auto delay_ms = kMinSleepMs; auto start = std::chrono::steady_clock::now(); @@ -163,18 +145,21 @@ Result>> RestTableScan::FetchPlanningR for (int retry = 0; retry <= kMaxRetries; ++retry) { ICEBERG_ASSIGN_OR_RAISE( const auto response, - rest_context_.client->Get(path, /*params=*/{}, /*headers=*/{}, - *PlanErrorHandler::Instance(), *rest_context_.session)); + ctx.client->Get(path, /*params=*/{}, /*headers=*/{}, + *PlanErrorHandler::Instance(), *ctx.session)); ICEBERG_ASSIGN_OR_RAISE(auto json, FromJsonString(response.body())); ICEBERG_ASSIGN_OR_RAISE(auto result, - FetchPlanningResultResponseFromJson(json, specs, *schema_)); + FetchPlanningResultResponseFromJson(json, specs, schema)); ICEBERG_RETURN_UNEXPECTED(result.Validate()); switch (result.plan_status) { case PlanStatus::kCompleted: { - ICEBERG_RETURN_UNEXPECTED(ApplyStorageCredentials(result.storage_credentials)); - auto tasks = ResolveScanTasks(result.plan_tasks, result.file_scan_tasks, specs); - if (!tasks.has_value()) CancelPlanning(plan_id); + ICEBERG_RETURN_UNEXPECTED( + ApplyStorageCredentials(ctx, result.storage_credentials, scan_io)); + auto tasks = + ResolveScanTasks(ctx, schema, result.plan_tasks, result.file_scan_tasks, specs, + scan_io); + if (!tasks.has_value()) CancelPlanning(ctx, plan_id); return tasks; } case PlanStatus::kSubmitted: { @@ -182,7 +167,7 @@ Result>> RestTableScan::FetchPlanningR std::chrono::steady_clock::now() - start) .count(); if (elapsed_ms >= kMaxWaitTimeMs) { - CancelPlanning(plan_id); + CancelPlanning(ctx, plan_id); return IOError("Scan planning timed out after {}ms waiting for plan_id={}", elapsed_ms, plan_id); } @@ -191,7 +176,7 @@ Result>> RestTableScan::FetchPlanningR continue; } case PlanStatus::kFailed: - CancelPlanning(plan_id); + CancelPlanning(ctx, plan_id); return IOError("Scan planning failed: {}", result.error ? result.error->message : "unknown error"); case PlanStatus::kCancelled: @@ -199,81 +184,234 @@ Result>> RestTableScan::FetchPlanningR } } - CancelPlanning(plan_id); + CancelPlanning(ctx, plan_id); return IOError("Scan planning exceeded max retries ({}) for plan_id={}", kMaxRetries, plan_id); } -Result>> RestTableScan::FetchScanTasks( - const std::string& plan_task, - const std::unordered_map>& specs) const { - ICEBERG_ENDPOINT_CHECK(rest_context_.supported_endpoints, Endpoint::FetchScanTasks()); +/// A lazy stream that drives FetchScanTasks calls on demand. +/// +/// The eager POST /plan (or poll until COMPLETED) is done before this stream is +/// constructed. The stream then yields the directly-returned file_scan_tasks first, +/// then fetches each plan_task token lazily one at a time as Next() is called. +/// +/// scan_io_slot is shared with the owning RestTableScan so that credentials vended +/// by any FetchScanTasks response are visible through RestTableScan::io() even after +/// the stream has been consumed. +class RestFileScanTaskStream final : public FileScanTaskStream { + public: + RestFileScanTaskStream(RestScanContext ctx, std::shared_ptr schema, + std::string plan_id, + std::vector> initial_tasks, + std::vector plan_task_tokens, SpecsById specs, + ScanIoSlot scan_io_slot) + : ctx_(std::move(ctx)), + schema_(std::move(schema)), + plan_id_(std::move(plan_id)), + buffer_(std::move(initial_tasks)), + plan_task_tokens_(std::move(plan_task_tokens)), + specs_(std::move(specs)), + scan_io_slot_(std::move(scan_io_slot)) {} + + ~RestFileScanTaskStream() override { + if (!consumed_) CancelPlanning(ctx_, plan_id_); + } - ICEBERG_ASSIGN_OR_RAISE(auto path, - rest_context_.paths->FetchScanTasks(rest_context_.identifier)); - FetchScanTasksRequest request{.planTask = plan_task}; - ICEBERG_ASSIGN_OR_RAISE(auto json_request, ToJsonString(ToJson(request))); - ICEBERG_ASSIGN_OR_RAISE(const auto response, - rest_context_.client->Post(path, json_request, /*headers=*/{}, - *PlanTaskErrorHandler::Instance(), - *rest_context_.session)); + protected: + Result>> NextImpl() override { + while (true) { + if (buffer_pos_ < buffer_.size()) { + return buffer_[buffer_pos_++]; + } + if (token_pos_ >= plan_task_tokens_.size()) { + consumed_ = true; + return std::nullopt; + } + const auto& token = plan_task_tokens_[token_pos_++]; + ICEBERG_ASSIGN_OR_RAISE( + buffer_, FetchScanTasks(ctx_, *schema_, token, specs_, *scan_io_slot_)); + buffer_pos_ = 0; + } + } + + private: + RestScanContext ctx_; + std::shared_ptr schema_; + std::string plan_id_; + std::vector> buffer_; + size_t buffer_pos_ = 0; + std::vector plan_task_tokens_; + size_t token_pos_ = 0; + SpecsById specs_; + ScanIoSlot scan_io_slot_; + bool consumed_ = false; +}; + +/// POST /plan (polling if SUBMITTED), apply credentials, return a lazy stream. +/// +/// scan_io_slot is shared with the caller (RestTableScan) so that credentials +/// vended by FetchScanTasks responses update RestTableScan::io() in place. +Result ExecuteScanPlanStream(const RestScanContext& ctx, + const Schema& schema, + std::shared_ptr schema_ptr, + PlanTableScanRequest request, + const SpecsById& specs, + ScanIoSlot scan_io_slot) { + ICEBERG_ENDPOINT_CHECK(ctx.supported_endpoints, Endpoint::PlanTableScan()); + + ICEBERG_ASSIGN_OR_RAISE(auto path, ctx.paths->Plan(ctx.identifier)); + ICEBERG_ASSIGN_OR_RAISE(auto request_json, ToJson(request)); + ICEBERG_ASSIGN_OR_RAISE(auto json_request, ToJsonString(request_json)); + ICEBERG_ASSIGN_OR_RAISE( + const auto response, + ctx.client->Post(path, json_request, /*headers=*/{}, *PlanErrorHandler::Instance(), + *ctx.session)); ICEBERG_ASSIGN_OR_RAISE(auto json, FromJsonString(response.body())); - ICEBERG_ASSIGN_OR_RAISE(auto result, - FetchScanTasksResponseFromJson(json, specs, *schema_)); + ICEBERG_ASSIGN_OR_RAISE(auto result, PlanTableScanResponseFromJson(json, specs, schema)); ICEBERG_RETURN_UNEXPECTED(result.Validate()); - ICEBERG_RETURN_UNEXPECTED(ApplyStorageCredentials(result.storage_credentials)); - return ResolveScanTasks(result.plan_tasks, result.file_scan_tasks, specs); -} + const std::string plan_id = result.plan_id; -Result>> RestTableScan::ResolveScanTasks( - const std::optional>& plan_tasks, - const std::optional>>& file_scan_tasks, - const std::unordered_map>& specs) const { - std::vector> result; + if (result.plan_status == PlanStatus::kSubmitted) { + // Poll until COMPLETED, eagerly collecting all tasks into the buffer. + std::string mutable_plan_id = plan_id; + ICEBERG_ASSIGN_OR_RAISE( + auto tasks, FetchPlanningResult(ctx, schema, mutable_plan_id, specs, *scan_io_slot)); + return std::make_unique(ctx, schema_ptr, mutable_plan_id, + std::move(tasks), {}, specs, scan_io_slot); + } - if (file_scan_tasks.has_value()) { - result.insert(result.end(), file_scan_tasks->begin(), file_scan_tasks->end()); + if (result.plan_status == PlanStatus::kFailed) { + return IOError("Scan planning failed: {}", + result.error ? result.error->message : "unknown error"); + } + if (result.plan_status == PlanStatus::kCancelled) { + return IOError("Scan planning was cancelled for plan_id={}", plan_id); } - if (plan_tasks.has_value()) { - for (const auto& plan_task : *plan_tasks) { - ICEBERG_ASSIGN_OR_RAISE(auto tasks, FetchScanTasks(plan_task, specs)); - result.insert(result.end(), tasks.begin(), tasks.end()); - } + // kCompleted: apply credentials from the initial response, then build the lazy stream. + ICEBERG_RETURN_UNEXPECTED(ApplyStorageCredentials(ctx, result.storage_credentials, *scan_io_slot)); + + std::vector> initial_tasks; + if (result.file_scan_tasks.has_value()) { + initial_tasks = std::move(*result.file_scan_tasks); + } + std::vector plan_task_tokens; + if (result.plan_tasks.has_value()) { + plan_task_tokens = std::move(*result.plan_tasks); } - return result; + return std::make_unique( + ctx, std::move(schema_ptr), plan_id, std::move(initial_tasks), + std::move(plan_task_tokens), specs, std::move(scan_io_slot)); } -void RestTableScan::CancelPlanning(const std::string& plan_id) const { - if (plan_id.empty()) return; - if (!rest_context_.supported_endpoints.contains(Endpoint::CancelPlanning())) return; +/// Eager batch planning: used by RestIncrementalAppendScan which has no stream path. +Result>> ExecuteScanPlan( + const RestScanContext& ctx, const Schema& schema, PlanTableScanRequest request, + const SpecsById& specs, std::shared_ptr& scan_io) { + ICEBERG_ENDPOINT_CHECK(ctx.supported_endpoints, Endpoint::PlanTableScan()); - auto path = rest_context_.paths->Plan(rest_context_.identifier, plan_id); - if (!path.has_value()) return; + ICEBERG_ASSIGN_OR_RAISE(auto path, ctx.paths->Plan(ctx.identifier)); + ICEBERG_ASSIGN_OR_RAISE(auto request_json, ToJson(request)); + ICEBERG_ASSIGN_OR_RAISE(auto json_request, ToJsonString(request_json)); + ICEBERG_ASSIGN_OR_RAISE( + const auto response, + ctx.client->Post(path, json_request, /*headers=*/{}, *PlanErrorHandler::Instance(), + *ctx.session)); + ICEBERG_ASSIGN_OR_RAISE(auto json, FromJsonString(response.body())); + ICEBERG_ASSIGN_OR_RAISE(auto result, PlanTableScanResponseFromJson(json, specs, schema)); + ICEBERG_RETURN_UNEXPECTED(result.Validate()); + + const std::string plan_id = result.plan_id; + + switch (result.plan_status) { + case PlanStatus::kCompleted: { + ICEBERG_RETURN_UNEXPECTED( + ApplyStorageCredentials(ctx, result.storage_credentials, scan_io)); + auto tasks = + ResolveScanTasks(ctx, schema, result.plan_tasks, result.file_scan_tasks, specs, + scan_io); + if (!tasks.has_value()) CancelPlanning(ctx, plan_id); + return tasks; + } + case PlanStatus::kSubmitted: + return FetchPlanningResult(ctx, schema, plan_id, specs, scan_io); + case PlanStatus::kFailed: + return IOError("Scan planning failed: {}", + result.error ? result.error->message : "unknown error"); + case PlanStatus::kCancelled: + return IOError("Scan planning was cancelled for plan_id={}", plan_id); + } + return IOError("Unexpected plan status"); +} + +} // namespace + +// --------------------------------------------------------------------------- +// RestTableScan +// --------------------------------------------------------------------------- + +RestTableScan::RestTableScan(std::shared_ptr metadata, + std::shared_ptr schema, std::shared_ptr io, + internal::TableScanContext context, + RestScanContext rest_context) + : DataTableScan(std::move(metadata), std::move(schema), std::move(io), + std::move(context)), + rest_context_(std::move(rest_context)), + scan_io_slot_(std::make_shared>()) {} - // Best-effort: ignore errors. - std::ignore = - rest_context_.client->Delete(*path, /*params=*/{}, /*headers=*/{}, - *PlanErrorHandler::Instance(), *rest_context_.session); +Result> RestTableScan::Make( + std::shared_ptr metadata, std::shared_ptr schema, + std::shared_ptr io, internal::TableScanContext context, + RestScanContext rest_context) { + ICEBERG_PRECHECK(metadata != nullptr, "Table metadata cannot be null"); + ICEBERG_PRECHECK(schema != nullptr, "Schema cannot be null"); + ICEBERG_PRECHECK(io != nullptr, "FileIO cannot be null"); + return std::unique_ptr( + new RestTableScan(std::move(metadata), std::move(schema), std::move(io), + std::move(context), std::move(rest_context))); } -const std::shared_ptr& RestTableScan::effective_io() const { - return scan_io_ ? scan_io_ : io_; +Result RestTableScan::PlanFilesStream() const { + TableMetadataCache metadata_cache(metadata_.get()); + ICEBERG_ASSIGN_OR_RAISE(auto specs, metadata_cache.GetPartitionSpecsById()); + + PlanTableScanRequest request; + request.select = context_.selected_columns.value_or(std::vector{}); + request.filter = context_.filter; + request.case_sensitive = context_.case_sensitive; + request.min_rows_requested = context_.min_rows_requested; + + if (context_.from_snapshot_id.has_value() && context_.to_snapshot_id.has_value()) { + request.start_snapshot_id = context_.from_snapshot_id; + request.end_snapshot_id = context_.to_snapshot_id; + request.use_snapshot_schema = true; + } else if (context_.snapshot_id.has_value()) { + request.snapshot_id = context_.snapshot_id; + request.use_snapshot_schema = context_.use_snapshot_schema; + } + + if (!context_.columns_to_keep_stats.empty()) { + for (int32_t field_id : context_.columns_to_keep_stats) { + ICEBERG_ASSIGN_OR_RAISE(auto name, schema_->FindColumnNameById(field_id)); + if (name.has_value()) { + request.stats_fields.emplace_back(*name); + } + } + } + + return ExecuteScanPlanStream(rest_context_, *schema_, schema_, std::move(request), specs, + scan_io_slot_); } -Status RestTableScan::ApplyStorageCredentials( - const std::vector& credentials) const { - if (credentials.empty()) return {}; - ICEBERG_ASSIGN_OR_RAISE( - auto io, MakeTableFileIO(rest_context_.catalog_config, rest_context_.table_config, - credentials)); - scan_io_ = std::move(io); - return {}; +const std::shared_ptr& RestTableScan::io() const { + return *scan_io_slot_ ? *scan_io_slot_ : io_; } +// --------------------------------------------------------------------------- // RestTableScanBuilder +// --------------------------------------------------------------------------- RestTableScanBuilder::RestTableScanBuilder( std::shared_ptr metadata, std::shared_ptr io, @@ -291,4 +429,91 @@ Result> RestTableScanBuilder::Build() { rest_context_); } +// --------------------------------------------------------------------------- +// RestIncrementalAppendScan +// --------------------------------------------------------------------------- + +RestIncrementalAppendScan::RestIncrementalAppendScan( + std::shared_ptr metadata, std::shared_ptr schema, + std::shared_ptr io, internal::TableScanContext context, + RestScanContext rest_context) + : IncrementalAppendScan(std::move(metadata), std::move(schema), std::move(io), + std::move(context)), + rest_context_(std::move(rest_context)) {} + +Result> RestIncrementalAppendScan::Make( + std::shared_ptr metadata, std::shared_ptr schema, + std::shared_ptr io, internal::TableScanContext context, + RestScanContext rest_context) { + ICEBERG_PRECHECK(metadata != nullptr, "Table metadata cannot be null"); + ICEBERG_PRECHECK(schema != nullptr, "Schema cannot be null"); + ICEBERG_PRECHECK(io != nullptr, "FileIO cannot be null"); + return std::unique_ptr(new RestIncrementalAppendScan( + std::move(metadata), std::move(schema), std::move(io), std::move(context), + std::move(rest_context))); +} + +Result>> RestIncrementalAppendScan::PlanFiles() + const { + TableMetadataCache metadata_cache(metadata_.get()); + ICEBERG_ASSIGN_OR_RAISE(auto specs, metadata_cache.GetPartitionSpecsById()); + + PlanTableScanRequest request; + request.select = context_.selected_columns.value_or(std::vector{}); + request.filter = context_.filter; + request.case_sensitive = context_.case_sensitive; + request.min_rows_requested = context_.min_rows_requested; + + // Resolve end snapshot: use to_snapshot_id if set, else current table snapshot. + if (context_.to_snapshot_id.has_value()) { + request.end_snapshot_id = context_.to_snapshot_id; + } else { + ICEBERG_ASSIGN_OR_RAISE(auto snapshot, metadata_->Snapshot()); + if (!snapshot) return {}; + request.end_snapshot_id = snapshot->snapshot_id; + } + + // Resolve start snapshot (exclusive): respect from_snapshot_id_inclusive. + if (context_.from_snapshot_id.has_value()) { + if (context_.from_snapshot_id_inclusive) { + ICEBERG_ASSIGN_OR_RAISE(auto from_snap, + metadata_->SnapshotById(*context_.from_snapshot_id)); + request.start_snapshot_id = from_snap->parent_snapshot_id; + } else { + request.start_snapshot_id = context_.from_snapshot_id; + } + } + + if (!context_.columns_to_keep_stats.empty()) { + for (int32_t field_id : context_.columns_to_keep_stats) { + ICEBERG_ASSIGN_OR_RAISE(auto name, schema_->FindColumnNameById(field_id)); + if (name.has_value()) { + request.stats_fields.emplace_back(*name); + } + } + } + + return ExecuteScanPlan(rest_context_, *schema_, std::move(request), specs, scan_io_); +} + +// --------------------------------------------------------------------------- +// RestIncrementalAppendScanBuilder +// --------------------------------------------------------------------------- + +RestIncrementalAppendScanBuilder::RestIncrementalAppendScanBuilder( + std::shared_ptr metadata, std::shared_ptr io, + std::string table_name, std::shared_ptr metrics_reporter, + RestScanContext rest_context) + : IncrementalAppendScanBuilder(std::move(metadata), std::move(io), + std::move(table_name), std::move(metrics_reporter)), + rest_context_(std::move(rest_context)) {} + +Result> RestIncrementalAppendScanBuilder::Build() { + ICEBERG_RETURN_UNEXPECTED(CheckErrors()); + ICEBERG_RETURN_UNEXPECTED(context_.Validate()); + ICEBERG_ASSIGN_OR_RAISE(auto schema, ResolveSnapshotSchema()); + return RestIncrementalAppendScan::Make(metadata_, schema.get(), io_, std::move(context_), + rest_context_); +} + } // namespace iceberg::rest diff --git a/src/iceberg/catalog/rest/rest_table_scan.h b/src/iceberg/catalog/rest/rest_table_scan.h index 63a852303..8df976959 100644 --- a/src/iceberg/catalog/rest/rest_table_scan.h +++ b/src/iceberg/catalog/rest/rest_table_scan.h @@ -35,7 +35,7 @@ #include "iceberg/type_fwd.h" /// \file iceberg/catalog/rest/rest_table_scan.h -/// REST-specific table scan that delegates scan planning to the REST catalog server. +/// REST-specific table scans that delegate scan planning to the REST catalog server. namespace iceberg::rest { @@ -46,7 +46,7 @@ namespace auth { class AuthSession; } // namespace auth -/// \brief HTTP context shared between RestTable and RestTableScan. +/// \brief HTTP context shared between RestTable and REST scan classes. struct ICEBERG_REST_EXPORT RestScanContext { std::shared_ptr client; std::shared_ptr paths; @@ -60,7 +60,7 @@ struct ICEBERG_REST_EXPORT RestScanContext { std::unordered_map table_config; }; -/// \brief A DataTableScan that delegates PlanFiles() to the REST catalog server +/// \brief A DataTableScan that delegates PlanFilesStream() to the REST catalog server /// via the scan planning endpoints (planTableScan / fetchPlanningResult / /// cancelPlanning / fetchScanTasks). class ICEBERG_REST_EXPORT RestTableScan : public DataTableScan { @@ -72,51 +72,24 @@ class ICEBERG_REST_EXPORT RestTableScan : public DataTableScan { std::shared_ptr io, internal::TableScanContext context, RestScanContext rest_context); - /// \brief Plans files via the REST scan planning endpoints. - Result>> PlanFiles() const override; + /// \brief Plans files lazily via the REST scan planning endpoints. + Result PlanFilesStream() const override; - /// \brief Returns the FileIO to use when reading scan results. + /// \brief Returns the effective FileIO for reading scan results. /// /// If the server vended storage credentials during planning, returns a FileIO /// initialised with those credentials; otherwise returns the table's FileIO. - /// Must be called after PlanFiles(). - const std::shared_ptr& effective_io() const; + const std::shared_ptr& io() const override; private: RestTableScan(std::shared_ptr metadata, std::shared_ptr schema, std::shared_ptr io, internal::TableScanContext context, RestScanContext rest_context); - /// POST /plan → handle COMPLETED / SUBMITTED / FAILED / CANCELLED. - Result>> PlanTableScan( - std::string& plan_id, - const std::unordered_map>& specs) const; - - /// GET /plan/{plan_id} with exponential backoff until COMPLETED. - Result>> FetchPlanningResult( - const std::string& plan_id, - const std::unordered_map>& specs) const; - - /// POST /tasks/{plan_task_id} → fetch FileScanTasks for one opaque plan task token. - Result>> FetchScanTasks( - const std::string& plan_task, - const std::unordered_map>& specs) const; - - /// Flatten plan_tasks (opaque tokens) + file_scan_tasks into a single list. - Result>> ResolveScanTasks( - const std::optional>& plan_tasks, - const std::optional>>& file_scan_tasks, - const std::unordered_map>& specs) const; - - /// DELETE /plan/{plan_id}; best-effort, errors are silently ignored. - void CancelPlanning(const std::string& plan_id) const; - - /// Builds a scan-scoped FileIO from vended credentials and caches it in scan_io_. - /// No-op if credentials is empty. - Status ApplyStorageCredentials(const std::vector& credentials) const; - RestScanContext rest_context_; - mutable std::shared_ptr scan_io_; + /// Shared slot so credentials vended by any lazy FetchScanTasks response are + /// visible through io() even after the stream has been consumed. + mutable std::shared_ptr> scan_io_slot_; }; /// \brief Builder that produces a RestTableScan with the REST HTTP context injected. @@ -134,4 +107,47 @@ class ICEBERG_REST_EXPORT RestTableScanBuilder : public DataTableScanBuilder { RestScanContext rest_context_; }; +/// \brief An IncrementalAppendScan that delegates PlanFiles() to the REST catalog server. +/// +/// IncrementalChangelogScan is not delegated because the REST planTableScan response +/// only carries FileScanTask objects; reconstructing ChangelogScanTask entries +/// (AddedRowsScanTask vs DeletedDataFileScanTask) requires per-snapshot operation +/// metadata that the server does not return. Changelog scans always plan locally. +class ICEBERG_REST_EXPORT RestIncrementalAppendScan : public IncrementalAppendScan { + public: + ~RestIncrementalAppendScan() override = default; + + static Result> Make( + std::shared_ptr metadata, std::shared_ptr schema, + std::shared_ptr io, internal::TableScanContext context, + RestScanContext rest_context); + + /// \brief Plans files via the REST scan planning endpoints. + Result>> PlanFiles() const override; + + private: + RestIncrementalAppendScan(std::shared_ptr metadata, + std::shared_ptr schema, std::shared_ptr io, + internal::TableScanContext context, + RestScanContext rest_context); + + RestScanContext rest_context_; + mutable std::shared_ptr scan_io_; +}; + +/// \brief Builder that produces a RestIncrementalAppendScan. +class ICEBERG_REST_EXPORT RestIncrementalAppendScanBuilder + : public IncrementalAppendScanBuilder { + public: + RestIncrementalAppendScanBuilder(std::shared_ptr metadata, + std::shared_ptr io, std::string table_name, + std::shared_ptr metrics_reporter, + RestScanContext rest_context); + + Result> Build() override; + + private: + RestScanContext rest_context_; +}; + } // namespace iceberg::rest diff --git a/src/iceberg/table_scan.h b/src/iceberg/table_scan.h index 1b298cd33..0939d7dd8 100644 --- a/src/iceberg/table_scan.h +++ b/src/iceberg/table_scan.h @@ -442,7 +442,7 @@ class ICEBERG_EXPORT TableScan { const internal::TableScanContext& context() const; /// \brief Returns the file I/O instance used for reading files. - const std::shared_ptr& io() const; + virtual const std::shared_ptr& io() const; /// \brief Returns this scan's filter expression. const std::shared_ptr& filter() const; @@ -488,7 +488,7 @@ class ICEBERG_EXPORT DataTableScan : public TableScan { /// can outlive this scan. An executor configured through PlanWith() is borrowed and /// must remain alive until the stream is destroyed, as later Next() calls may submit /// work to it. - Result PlanFilesStream() const; + virtual Result PlanFilesStream() const; protected: using TableScan::TableScan; diff --git a/src/iceberg/test/rest_table_scan_test.cc b/src/iceberg/test/rest_table_scan_test.cc index 9c5efd929..ed9a000b6 100644 --- a/src/iceberg/test/rest_table_scan_test.cc +++ b/src/iceberg/test/rest_table_scan_test.cc @@ -384,7 +384,7 @@ TEST_F(RestTableScanTest, DefaultScanDoesNotSetUseSnapshotSchema) { } // -------------------------------------------------------------------------- -// Storage credentials in COMPLETED response: effective_io() returns a +// Storage credentials in COMPLETED response: io() returns a // credential-scoped IO, not the original table IO. // -------------------------------------------------------------------------- TEST_F(RestTableScanTest, StorageCredentialsInPlanResponseUpdatesEffectiveIO) { @@ -401,14 +401,12 @@ TEST_F(RestTableScanTest, StorageCredentialsInPlanResponseUpdatesEffectiveIO) { ICEBERG_UNWRAP_OR_FAIL(auto tasks, scan->PlanFiles()); EXPECT_TRUE(tasks.empty()); - auto* rest_scan = dynamic_cast(scan.get()); - ASSERT_NE(rest_scan, nullptr); - // effective_io() must return a credential-scoped IO, not the original file_io_. - EXPECT_NE(rest_scan->effective_io().get(), file_io_.get()); + // io() must return a credential-scoped IO without requiring a downcast. + EXPECT_NE(scan->io().get(), file_io_.get()); } // -------------------------------------------------------------------------- -// No storage credentials: effective_io() falls back to the table's FileIO. +// No storage credentials: io() falls back to the table's FileIO. // -------------------------------------------------------------------------- TEST_F(RestTableScanTest, NoStorageCredentialsEffectiveIoFallsBackToTableIO) { constexpr std::string_view kResponseBody = R"({"status":"completed"})"; @@ -419,14 +417,11 @@ TEST_F(RestTableScanTest, NoStorageCredentialsEffectiveIoFallsBackToTableIO) { ICEBERG_UNWRAP_OR_FAIL(auto tasks, scan->PlanFiles()); EXPECT_TRUE(tasks.empty()); - auto* rest_scan = dynamic_cast(scan.get()); - ASSERT_NE(rest_scan, nullptr); - EXPECT_EQ(rest_scan->effective_io().get(), file_io_.get()); + EXPECT_EQ(scan->io().get(), file_io_.get()); } // -------------------------------------------------------------------------- -// Storage credentials returned in FetchScanTasksResponse also update -// effective_io(). +// Storage credentials returned in FetchScanTasksResponse also update io(). // -------------------------------------------------------------------------- TEST_F(RestTableScanTest, StorageCredentialsInFetchScanTasksResponseUpdatesEffectiveIO) { constexpr std::string_view kPlanResponse = @@ -446,9 +441,7 @@ TEST_F(RestTableScanTest, StorageCredentialsInFetchScanTasksResponseUpdatesEffec ICEBERG_UNWRAP_OR_FAIL(auto tasks, scan->PlanFiles()); EXPECT_TRUE(tasks.empty()); - auto* rest_scan = dynamic_cast(scan.get()); - ASSERT_NE(rest_scan, nullptr); - EXPECT_NE(rest_scan->effective_io().get(), file_io_.get()); + EXPECT_NE(scan->io().get(), file_io_.get()); } // -------------------------------------------------------------------------- From 0614e84516cc7d9164f0ab026c465f37a779c0ff Mon Sep 17 00:00:00 2001 From: Sandeep Gottimukkala Date: Mon, 28 Sep 2026 20:33:04 -0700 Subject: [PATCH 09/16] fix: remove unnecessary virtual from DataTableScan::PlanFiles() MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit PlanFiles() was made virtual in this PR to support a RestTableScan override. That override has since been replaced by RestTableScan::PlanFilesStream(), so PlanFiles() no longer needs to be virtual — callers dispatch through the virtual PlanFilesStream() hook instead. --- src/iceberg/table_scan.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/iceberg/table_scan.h b/src/iceberg/table_scan.h index 0939d7dd8..0190ad01f 100644 --- a/src/iceberg/table_scan.h +++ b/src/iceberg/table_scan.h @@ -480,7 +480,7 @@ class ICEBERG_EXPORT DataTableScan : public TableScan { /// /// Collects PlanFilesStream() into a vector. /// \return A Result containing scan tasks or an error. - virtual Result>> PlanFiles() const; + Result>> PlanFiles() const; /// \brief Lazily plans scan tasks by resolving manifests and data files on demand. /// From 7ab2637df9df3196f70dd26381e958e1d760b24d Mon Sep 17 00:00:00 2001 From: Sandeep Gottimukkala Date: Mon, 28 Sep 2026 20:55:16 -0700 Subject: [PATCH 10/16] test(rest): add tests for PlanFilesStream, RestIncrementalAppendScan, and virtual io() MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - PlanFilesStream: COMPLETED, lazy token fetching, cancel-on-partial-consume, SUBMITTED→poll→COMPLETED, FAILED - RestIncrementalAppendScan: COMPLETED, plan-tasks, SUBMITTED→poll→COMPLETED, FAILED, endpoint-not-supported, no-current-snapshot returns empty, explicit to_snapshot_id, exclusive from_snapshot_id, inclusive from_snapshot_id with and without a parent snapshot - RestTable::NewIncrementalAppendScan returns RestIncrementalAppendScanBuilder --- src/iceberg/test/rest_table_scan_test.cc | 307 +++++++++++++++++++++++ 1 file changed, 307 insertions(+) diff --git a/src/iceberg/test/rest_table_scan_test.cc b/src/iceberg/test/rest_table_scan_test.cc index ed9a000b6..bffa729bc 100644 --- a/src/iceberg/test/rest_table_scan_test.cc +++ b/src/iceberg/test/rest_table_scan_test.cc @@ -457,4 +457,311 @@ TEST_F(RestTableScanTest, RestTableNewScanReturnsRestTableScanBuilder) { EXPECT_NE(typed, nullptr); } +// ========================================================================== +// PlanFilesStream tests +// ========================================================================== + +// -------------------------------------------------------------------------- +// PlanFilesStream: COMPLETED immediately, stream yields no tasks. +// -------------------------------------------------------------------------- +TEST_F(RestTableScanTest, PlanFilesStreamCompleted) { + constexpr std::string_view kResponseBody = R"({"status":"completed"})"; + EXPECT_CALL(*mock_client_, Post(_, _, _, _, _)) + .WillOnce(Return(HttpResponse::MakeForTesting(200, std::string(kResponseBody)))); + + ICEBERG_UNWRAP_OR_FAIL(auto scan, MakeScan(MakeContext())); + ICEBERG_UNWRAP_OR_FAIL(auto stream, scan->PlanFilesStream()); + ICEBERG_UNWRAP_OR_FAIL(auto tasks, stream->ToVector()); + EXPECT_TRUE(tasks.empty()); +} + +// -------------------------------------------------------------------------- +// PlanFilesStream: COMPLETED with two plan-task tokens; each fetched lazily +// as Next() is called, not all at once on POST /plan. +// -------------------------------------------------------------------------- +TEST_F(RestTableScanTest, PlanFilesStreamLazilyFetchesPlanTasks) { + constexpr std::string_view kPlanResponse = + R"({"status":"completed","plan-id":"plan-stream","plan-tasks":["tok-s1","tok-s2"]})"; + constexpr std::string_view kTasksResponse = R"({"file-scan-tasks":[]})"; + + EXPECT_CALL(*mock_client_, Post(_, _, _, _, _)) + .WillOnce(Return(HttpResponse::MakeForTesting(200, std::string(kPlanResponse)))) + .WillOnce(Return(HttpResponse::MakeForTesting(200, std::string(kTasksResponse)))) + .WillOnce(Return(HttpResponse::MakeForTesting(200, std::string(kTasksResponse)))); + + ICEBERG_UNWRAP_OR_FAIL(auto scan, MakeScan(MakeContext())); + ICEBERG_UNWRAP_OR_FAIL(auto stream, scan->PlanFilesStream()); + ICEBERG_UNWRAP_OR_FAIL(auto tasks, stream->ToVector()); + EXPECT_TRUE(tasks.empty()); +} + +// -------------------------------------------------------------------------- +// PlanFilesStream: stream destroyed before full consumption → DELETE /plan. +// -------------------------------------------------------------------------- +TEST_F(RestTableScanTest, PlanFilesStreamCancelOnPartialConsumption) { + constexpr std::string_view kPlanResponse = + R"({"status":"completed","plan-id":"plan-partial","plan-tasks":["tok-p1"]})"; + + EXPECT_CALL(*mock_client_, Post(_, _, _, _, _)) + .WillOnce(Return(HttpResponse::MakeForTesting(200, std::string(kPlanResponse)))); + EXPECT_CALL(*mock_client_, Delete(_, _, _, _, _)) + .WillOnce(Return(HttpResponse::MakeForTesting(200, "{}"))); + + ICEBERG_UNWRAP_OR_FAIL(auto scan, MakeScan(MakeContext())); + { + ICEBERG_UNWRAP_OR_FAIL(auto stream, scan->PlanFilesStream()); + // Destroy without consuming — destructor must call DELETE /plan. + } +} + +// -------------------------------------------------------------------------- +// PlanFilesStream: SUBMITTED → poll until COMPLETED, stream yields all tasks. +// -------------------------------------------------------------------------- +TEST_F(RestTableScanTest, PlanFilesStreamSubmittedThenCompleted) { + constexpr std::string_view kSubmittedBody = + R"({"status":"submitted","plan-id":"plan-poll-stream"})"; + constexpr std::string_view kCompletedBody = R"({"status":"completed"})"; + + EXPECT_CALL(*mock_client_, Post(_, _, _, _, _)) + .WillOnce(Return(HttpResponse::MakeForTesting(200, std::string(kSubmittedBody)))); + EXPECT_CALL(*mock_client_, Get(_, _, _, _, _)) + .WillOnce(Return(HttpResponse::MakeForTesting(200, std::string(kCompletedBody)))); + + ICEBERG_UNWRAP_OR_FAIL(auto scan, MakeScan(MakeContext())); + ICEBERG_UNWRAP_OR_FAIL(auto stream, scan->PlanFilesStream()); + ICEBERG_UNWRAP_OR_FAIL(auto tasks, stream->ToVector()); + EXPECT_TRUE(tasks.empty()); +} + +// -------------------------------------------------------------------------- +// PlanFilesStream: FAILED → stream returns an error. +// -------------------------------------------------------------------------- +TEST_F(RestTableScanTest, PlanFilesStreamFailed) { + constexpr std::string_view kFailedBody = + R"({"status":"failed","error":{"message":"server error","type":"ServerError","code":500}})"; + EXPECT_CALL(*mock_client_, Post(_, _, _, _, _)) + .WillOnce(Return(HttpResponse::MakeForTesting(200, std::string(kFailedBody)))); + + ICEBERG_UNWRAP_OR_FAIL(auto scan, MakeScan(MakeContext())); + auto result = scan->PlanFilesStream(); + EXPECT_THAT(result, IsError(ErrorKind::kIOError)); +} + +// ========================================================================== +// RestIncrementalAppendScan tests +// ========================================================================== + +class RestIncrementalAppendScanTest : public RestTableScanTest { + protected: + // Creates a RestIncrementalAppendScan with the given context and optional + // snapshot range. + Result> MakeIncrementalScan( + RestScanContext ctx, std::optional from_snapshot_id = std::nullopt, + bool from_inclusive = false, + std::optional to_snapshot_id = std::nullopt) { + RestIncrementalAppendScanBuilder builder(metadata_, file_io_, "test.my_table", + nullptr, std::move(ctx)); + if (from_snapshot_id.has_value()) { + builder.FromSnapshot(*from_snapshot_id, from_inclusive); + } + if (to_snapshot_id.has_value()) { + builder.ToSnapshot(*to_snapshot_id); + } + return builder.Build(); + } +}; + +// -------------------------------------------------------------------------- +// PlanFiles: server returns COMPLETED immediately, no tasks. +// -------------------------------------------------------------------------- +TEST_F(RestIncrementalAppendScanTest, PlanFilesCompleted) { + constexpr std::string_view kResponseBody = R"({"status":"completed"})"; + EXPECT_CALL(*mock_client_, Post(_, _, _, _, _)) + .WillOnce(Return(HttpResponse::MakeForTesting(200, std::string(kResponseBody)))); + + ICEBERG_UNWRAP_OR_FAIL(auto scan, MakeIncrementalScan(MakeContext())); + ICEBERG_UNWRAP_OR_FAIL(auto tasks, scan->PlanFiles()); + EXPECT_TRUE(tasks.empty()); +} + +// -------------------------------------------------------------------------- +// PlanFiles: COMPLETED with a plan-task token; FetchScanTasks is called. +// -------------------------------------------------------------------------- +TEST_F(RestIncrementalAppendScanTest, PlanFilesWithPlanTasks) { + constexpr std::string_view kPlanResponse = + R"({"status":"completed","plan-id":"incr-plan-1","plan-tasks":["tok-incr-1"]})"; + constexpr std::string_view kTasksResponse = R"({"file-scan-tasks":[]})"; + + EXPECT_CALL(*mock_client_, Post(_, _, _, _, _)) + .WillOnce(Return(HttpResponse::MakeForTesting(200, std::string(kPlanResponse)))) + .WillOnce(Return(HttpResponse::MakeForTesting(200, std::string(kTasksResponse)))); + + ICEBERG_UNWRAP_OR_FAIL(auto scan, MakeIncrementalScan(MakeContext())); + ICEBERG_UNWRAP_OR_FAIL(auto tasks, scan->PlanFiles()); + EXPECT_TRUE(tasks.empty()); +} + +// -------------------------------------------------------------------------- +// PlanFiles: SUBMITTED → poll → COMPLETED. +// -------------------------------------------------------------------------- +TEST_F(RestIncrementalAppendScanTest, PlanFilesSubmittedThenCompleted) { + constexpr std::string_view kSubmittedBody = + R"({"status":"submitted","plan-id":"incr-poll-1"})"; + constexpr std::string_view kCompletedBody = R"({"status":"completed"})"; + + EXPECT_CALL(*mock_client_, Post(_, _, _, _, _)) + .WillOnce(Return(HttpResponse::MakeForTesting(200, std::string(kSubmittedBody)))); + EXPECT_CALL(*mock_client_, Get(_, _, _, _, _)) + .WillOnce(Return(HttpResponse::MakeForTesting(200, std::string(kCompletedBody)))); + + ICEBERG_UNWRAP_OR_FAIL(auto scan, MakeIncrementalScan(MakeContext())); + ICEBERG_UNWRAP_OR_FAIL(auto tasks, scan->PlanFiles()); + EXPECT_TRUE(tasks.empty()); +} + +// -------------------------------------------------------------------------- +// PlanFiles: FAILED → IOError. +// -------------------------------------------------------------------------- +TEST_F(RestIncrementalAppendScanTest, PlanFilesFailed) { + constexpr std::string_view kFailedBody = + R"({"status":"failed","error":{"message":"server error","type":"ServerError","code":500}})"; + EXPECT_CALL(*mock_client_, Post(_, _, _, _, _)) + .WillOnce(Return(HttpResponse::MakeForTesting(200, std::string(kFailedBody)))); + + ICEBERG_UNWRAP_OR_FAIL(auto scan, MakeIncrementalScan(MakeContext())); + auto result = scan->PlanFiles(); + EXPECT_THAT(result, IsError(ErrorKind::kIOError)); +} + +// -------------------------------------------------------------------------- +// PlanFiles: PlanTableScan endpoint missing → NotSupported. +// -------------------------------------------------------------------------- +TEST_F(RestIncrementalAppendScanTest, PlanFilesEndpointNotSupported) { + ICEBERG_UNWRAP_OR_FAIL(auto scan, + MakeIncrementalScan(MakeContext(std::unordered_set{}))); + auto result = scan->PlanFiles(); + EXPECT_THAT(result, IsError(ErrorKind::kNotSupported)); +} + +// -------------------------------------------------------------------------- +// No current snapshot → PlanFiles returns empty without calling the server. +// -------------------------------------------------------------------------- +TEST_F(RestIncrementalAppendScanTest, PlanFilesEmptyWhenNoCurrentSnapshot) { + // Build metadata with no current snapshot. + auto spec = PartitionSpec::Unpartitioned(); + auto empty_metadata = std::make_shared(TableMetadata{ + .format_version = 2, + .table_uuid = "no-snap-uuid", + .location = "/tmp/table", + .last_sequence_number = 0L, + .last_updated_ms = TimePointMsFromUnixMs(1609459200000L), + .last_column_id = 2, + .schemas = {schema_}, + .current_schema_id = schema_->schema_id(), + .partition_specs = {spec}, + .default_spec_id = spec->spec_id(), + .last_partition_id = 999, + }); + + EXPECT_CALL(*mock_client_, Post(_, _, _, _, _)).Times(0); + + // Use Make() directly to bypass builder validation that requires a snapshot. + ICEBERG_UNWRAP_OR_FAIL( + auto scan, RestIncrementalAppendScan::Make(empty_metadata, schema_, file_io_, + internal::TableScanContext{}, + MakeContext())); + ICEBERG_UNWRAP_OR_FAIL(auto tasks, scan->PlanFiles()); + EXPECT_TRUE(tasks.empty()); +} + +// -------------------------------------------------------------------------- +// Explicit to_snapshot_id: request uses that snapshot, not current. +// -------------------------------------------------------------------------- +TEST_F(RestIncrementalAppendScanTest, PlanFilesWithExplicitToSnapshotId) { + constexpr int64_t kToSnapshotId = 1000L; + constexpr std::string_view kResponseBody = R"({"status":"completed"})"; + EXPECT_CALL(*mock_client_, Post(_, _, _, _, _)) + .WillOnce(Return(HttpResponse::MakeForTesting(200, std::string(kResponseBody)))); + + ICEBERG_UNWRAP_OR_FAIL( + auto scan, MakeIncrementalScan(MakeContext(), std::nullopt, false, kToSnapshotId)); + ICEBERG_UNWRAP_OR_FAIL(auto tasks, scan->PlanFiles()); + EXPECT_TRUE(tasks.empty()); +} + +// -------------------------------------------------------------------------- +// Exclusive from_snapshot_id: passed directly as start_snapshot_id. +// -------------------------------------------------------------------------- +TEST_F(RestIncrementalAppendScanTest, PlanFilesWithFromSnapshotIdExclusive) { + constexpr int64_t kFromSnapshotId = 999L; + constexpr std::string_view kResponseBody = R"({"status":"completed"})"; + EXPECT_CALL(*mock_client_, Post(_, _, _, _, _)) + .WillOnce(Return(HttpResponse::MakeForTesting(200, std::string(kResponseBody)))); + + ICEBERG_UNWRAP_OR_FAIL( + auto scan, MakeIncrementalScan(MakeContext(), kFromSnapshotId, /*inclusive=*/false)); + ICEBERG_UNWRAP_OR_FAIL(auto tasks, scan->PlanFiles()); + EXPECT_TRUE(tasks.empty()); +} + +// -------------------------------------------------------------------------- +// Inclusive from_snapshot_id: parent snapshot is used as start_snapshot_id. +// The fixture snapshot (id=1000) has no parent, so start_snapshot_id = nullopt. +// -------------------------------------------------------------------------- +TEST_F(RestIncrementalAppendScanTest, PlanFilesWithFromSnapshotIdInclusiveNoParent) { + constexpr int64_t kFromSnapshotId = 1000L; + constexpr std::string_view kResponseBody = R"({"status":"completed"})"; + EXPECT_CALL(*mock_client_, Post(_, _, _, _, _)) + .WillOnce(Return(HttpResponse::MakeForTesting(200, std::string(kResponseBody)))); + + ICEBERG_UNWRAP_OR_FAIL( + auto scan, MakeIncrementalScan(MakeContext(), kFromSnapshotId, /*inclusive=*/true)); + ICEBERG_UNWRAP_OR_FAIL(auto tasks, scan->PlanFiles()); + EXPECT_TRUE(tasks.empty()); +} + +// -------------------------------------------------------------------------- +// Inclusive from_snapshot_id with a parent: parent's id used as start. +// -------------------------------------------------------------------------- +TEST_F(RestIncrementalAppendScanTest, PlanFilesWithFromSnapshotIdInclusiveWithParent) { + constexpr int64_t kParentSnapshotId = 900L; + constexpr int64_t kChildSnapshotId = 1001L; + + // Add a second snapshot with a parent to the metadata. + auto child_snapshot = std::make_shared( + Snapshot{.snapshot_id = kChildSnapshotId, + .parent_snapshot_id = kParentSnapshotId, + .sequence_number = 2L, + .timestamp_ms = TimePointMsFromUnixMs(1609459260000L), + .manifest_list = "/tmp/manifest-list-2.avro", + .schema_id = schema_->schema_id()}); + metadata_->snapshots.push_back(child_snapshot); + + constexpr std::string_view kResponseBody = R"({"status":"completed"})"; + EXPECT_CALL(*mock_client_, Post(_, _, _, _, _)) + .WillOnce(Return(HttpResponse::MakeForTesting(200, std::string(kResponseBody)))); + + ICEBERG_UNWRAP_OR_FAIL(auto scan, MakeIncrementalScan(MakeContext(), kChildSnapshotId, + /*inclusive=*/true)); + ICEBERG_UNWRAP_OR_FAIL(auto tasks, scan->PlanFiles()); + EXPECT_TRUE(tasks.empty()); +} + +// ========================================================================== +// RestTable::NewIncrementalAppendScan +// ========================================================================== + +// -------------------------------------------------------------------------- +// RestTable::NewIncrementalAppendScan returns a RestIncrementalAppendScanBuilder. +// -------------------------------------------------------------------------- +TEST_F(RestTableScanTest, RestTableNewIncrementalAppendScanReturnsRestBuilder) { + ICEBERG_UNWRAP_OR_FAIL( + auto table, RestTable::Make(identifier_, metadata_, "/tmp/metadata.json", file_io_, + /*catalog=*/nullptr, "test.my_table", nullptr, + MakeContext(std::nullopt))); + ICEBERG_UNWRAP_OR_FAIL(auto builder, table->NewIncrementalAppendScan()); + auto* typed = dynamic_cast(builder.get()); + EXPECT_NE(typed, nullptr); +} + } // namespace iceberg::rest From 28992b7dd3e7cfca9ccc7bb2ae5bb51e770c2a8a Mon Sep 17 00:00:00 2001 From: Sandeep Gottimukkala Date: Mon, 28 Sep 2026 21:02:30 -0700 Subject: [PATCH 11/16] fix(rest): call CancelPlanning on all error paths after plan_id is known FetchPlanningResult returned early on GET failure, JSON parse error, response deserialization error, Validate() failure, and ApplyStorageCredentials failure without cancelling, leaving the server holding plan resources. ExecuteScanPlanStream and ExecuteScanPlan had the same gap for the kFailed status and ApplyStorageCredentials failure in the kCompleted branch. Replace ICEBERG_ASSIGN_OR_RAISE / ICEBERG_RETURN_UNEXPECTED with explicit checks in FetchPlanningResult's poll loop, and add CancelPlanning before every error return in ExecuteScanPlanStream and ExecuteScanPlan where plan_id is known. --- src/iceberg/catalog/rest/rest_table_scan.cc | 56 +++++++++++++++------ 1 file changed, 41 insertions(+), 15 deletions(-) diff --git a/src/iceberg/catalog/rest/rest_table_scan.cc b/src/iceberg/catalog/rest/rest_table_scan.cc index 5a346b03e..a7ae751f0 100644 --- a/src/iceberg/catalog/rest/rest_table_scan.cc +++ b/src/iceberg/catalog/rest/rest_table_scan.cc @@ -143,23 +143,42 @@ Result>> FetchPlanningResult( auto start = std::chrono::steady_clock::now(); for (int retry = 0; retry <= kMaxRetries; ++retry) { - ICEBERG_ASSIGN_OR_RAISE( - const auto response, - ctx.client->Get(path, /*params=*/{}, /*headers=*/{}, - *PlanErrorHandler::Instance(), *ctx.session)); - ICEBERG_ASSIGN_OR_RAISE(auto json, FromJsonString(response.body())); - ICEBERG_ASSIGN_OR_RAISE(auto result, - FetchPlanningResultResponseFromJson(json, specs, schema)); - ICEBERG_RETURN_UNEXPECTED(result.Validate()); + auto response_or = ctx.client->Get(path, /*params=*/{}, /*headers=*/{}, + *PlanErrorHandler::Instance(), *ctx.session); + if (!response_or) { + CancelPlanning(ctx, plan_id); + return std::unexpected(response_or.error()); + } + + auto json_or = FromJsonString(response_or->body()); + if (!json_or) { + CancelPlanning(ctx, plan_id); + return std::unexpected(json_or.error()); + } + + auto result_or = FetchPlanningResultResponseFromJson(*json_or, specs, schema); + if (!result_or) { + CancelPlanning(ctx, plan_id); + return std::unexpected(result_or.error()); + } + + if (auto s = result_or->Validate(); !s) { + CancelPlanning(ctx, plan_id); + return std::unexpected(s.error()); + } + + auto& result = *result_or; switch (result.plan_status) { case PlanStatus::kCompleted: { - ICEBERG_RETURN_UNEXPECTED( - ApplyStorageCredentials(ctx, result.storage_credentials, scan_io)); + if (auto s = ApplyStorageCredentials(ctx, result.storage_credentials, scan_io); !s) { + CancelPlanning(ctx, plan_id); + return std::unexpected(s.error()); + } auto tasks = ResolveScanTasks(ctx, schema, result.plan_tasks, result.file_scan_tasks, specs, scan_io); - if (!tasks.has_value()) CancelPlanning(ctx, plan_id); + if (!tasks) CancelPlanning(ctx, plan_id); return tasks; } case PlanStatus::kSubmitted: { @@ -282,6 +301,7 @@ Result ExecuteScanPlanStream(const RestScanContext& ctx, } if (result.plan_status == PlanStatus::kFailed) { + CancelPlanning(ctx, plan_id); return IOError("Scan planning failed: {}", result.error ? result.error->message : "unknown error"); } @@ -290,7 +310,10 @@ Result ExecuteScanPlanStream(const RestScanContext& ctx, } // kCompleted: apply credentials from the initial response, then build the lazy stream. - ICEBERG_RETURN_UNEXPECTED(ApplyStorageCredentials(ctx, result.storage_credentials, *scan_io_slot)); + if (auto s = ApplyStorageCredentials(ctx, result.storage_credentials, *scan_io_slot); !s) { + CancelPlanning(ctx, plan_id); + return std::unexpected(s.error()); + } std::vector> initial_tasks; if (result.file_scan_tasks.has_value()) { @@ -327,17 +350,20 @@ Result>> ExecuteScanPlan( switch (result.plan_status) { case PlanStatus::kCompleted: { - ICEBERG_RETURN_UNEXPECTED( - ApplyStorageCredentials(ctx, result.storage_credentials, scan_io)); + if (auto s = ApplyStorageCredentials(ctx, result.storage_credentials, scan_io); !s) { + CancelPlanning(ctx, plan_id); + return std::unexpected(s.error()); + } auto tasks = ResolveScanTasks(ctx, schema, result.plan_tasks, result.file_scan_tasks, specs, scan_io); - if (!tasks.has_value()) CancelPlanning(ctx, plan_id); + if (!tasks) CancelPlanning(ctx, plan_id); return tasks; } case PlanStatus::kSubmitted: return FetchPlanningResult(ctx, schema, plan_id, specs, scan_io); case PlanStatus::kFailed: + CancelPlanning(ctx, plan_id); return IOError("Scan planning failed: {}", result.error ? result.error->message : "unknown error"); case PlanStatus::kCancelled: From ea8a901b2d8d6373be89a48c1fa3249d9d4924d9 Mon Sep 17 00:00:00 2001 From: Sandeep Gottimukkala Date: Mon, 28 Sep 2026 21:04:59 -0700 Subject: [PATCH 12/16] fix(rest): reset scan IO slot at the start of each planning call If PlanFilesStream()/PlanFiles() is called more than once and only the first response vends storage credentials, the second plan would inherit the stale credential-scoped FileIO from the first. Reset the slot to null before each planning call so that io() falls back to the table IO when the server does not vend credentials in the new response. --- src/iceberg/catalog/rest/rest_table_scan.cc | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/iceberg/catalog/rest/rest_table_scan.cc b/src/iceberg/catalog/rest/rest_table_scan.cc index a7ae751f0..dbb805655 100644 --- a/src/iceberg/catalog/rest/rest_table_scan.cc +++ b/src/iceberg/catalog/rest/rest_table_scan.cc @@ -400,6 +400,7 @@ Result> RestTableScan::Make( } Result RestTableScan::PlanFilesStream() const { + *scan_io_slot_ = nullptr; // reset so stale credentials from a prior plan are not reused TableMetadataCache metadata_cache(metadata_.get()); ICEBERG_ASSIGN_OR_RAISE(auto specs, metadata_cache.GetPartitionSpecsById()); @@ -481,6 +482,7 @@ Result> RestIncrementalAppendScan::Make( Result>> RestIncrementalAppendScan::PlanFiles() const { + scan_io_ = nullptr; // reset so stale credentials from a prior plan are not reused TableMetadataCache metadata_cache(metadata_.get()); ICEBERG_ASSIGN_OR_RAISE(auto specs, metadata_cache.GetPartitionSpecsById()); From cc4b96b3dd2dd69aaed6dfbbb93c4828acd4d416 Mon Sep 17 00:00:00 2001 From: Sandeep Gottimukkala Date: Mon, 28 Sep 2026 22:26:24 -0700 Subject: [PATCH 13/16] test(rest): add body-inspecting matchers and tests for cancel/credential fixes - Add JsonBodyHas/JsonBodyLacks GMock matchers that parse the POST body so request field propagation is verified against actual server JSON, not just internal context state - Update UseSnapshot and DefaultScan tests to call PlanFiles() and match "snapshot-id"/"use-snapshot-schema" in the POST body - Update incremental scan snapshot routing tests to match "start-snapshot-id"/"end-snapshot-id" values in the POST body - Add PlanFilesStreamFailedWithPlanIdCancels: verifies DELETE is called when ExecuteScanPlanStream receives FAILED with a plan-id - Add CancelCalledOnFetchPlanningResultGetError: verifies DELETE is called when FetchPlanningResult GET fails after SUBMITTED - Add SecondPlanFilesStreamCallClearsStaleCredentials: verifies scan_io_slot_ is reset so second plan response with no credentials reverts io() to table IO - Add PlanFilesFailedWithPlanIdCancels for RestIncrementalAppendScan - Add SecondPlanFilesCallClearsStaleCredentials for RestIncrementalAppendScan --- src/iceberg/test/rest_table_scan_test.cc | 212 +++++++++++++++++++++-- 1 file changed, 199 insertions(+), 13 deletions(-) diff --git a/src/iceberg/test/rest_table_scan_test.cc b/src/iceberg/test/rest_table_scan_test.cc index bffa729bc..1bad8dd10 100644 --- a/src/iceberg/test/rest_table_scan_test.cc +++ b/src/iceberg/test/rest_table_scan_test.cc @@ -50,6 +50,42 @@ namespace iceberg::rest { using ::testing::_; using ::testing::Return; +// Matches a JSON string body where `key` has exactly `expected_value`. +MATCHER_P2(JsonBodyHas, key, expected_value, "") { + try { + auto json = nlohmann::json::parse(arg); + if (!json.contains(key)) { + *result_listener << "JSON body missing key \"" << key << "\""; + return false; + } + nlohmann::json expected = expected_value; + if (json.at(key) != expected) { + *result_listener << "JSON[\"" << key << "\"] = " << json.at(key) + << ", expected " << expected; + return false; + } + return true; + } catch (...) { + *result_listener << "failed to parse JSON body"; + return false; + } +} + +// Matches a JSON string body that does NOT contain `key`. +MATCHER_P(JsonBodyLacks, key, "") { + try { + auto json = nlohmann::json::parse(arg); + if (json.contains(key)) { + *result_listener << "JSON body unexpectedly contains key \"" << key << "\""; + return false; + } + return true; + } catch (...) { + *result_listener << "failed to parse JSON body"; + return false; + } +} + // -------------------------------------------------------------------------- // Mock HTTP client that overrides the virtual methods of HttpClient. // The base class constructor creates a cpr::ConnectionPool, which is a @@ -361,26 +397,45 @@ TEST_F(RestTableScanTest, FetchScanTasksEndpointNotSupported) { } // -------------------------------------------------------------------------- -// use_snapshot_schema: UseSnapshot() sets it to true in the builder context. -// RestTableScanBuilder propagates context from DataTableScanBuilder. +// UseSnapshot(): the POST body sent to the server must contain both +// "snapshot-id" and "use-snapshot-schema": true. // -------------------------------------------------------------------------- TEST_F(RestTableScanTest, UseSnapshotPropagatesUseSnapshotSchemaInContext) { constexpr int64_t kSnapshotId = 1000L; + constexpr std::string_view kResponseBody = R"({"status":"completed"})"; + + EXPECT_CALL(*mock_client_, + Post(_, testing::AllOf(JsonBodyHas("snapshot-id", kSnapshotId), + JsonBodyHas("use-snapshot-schema", true)), + _, _, _)) + .WillOnce(Return(HttpResponse::MakeForTesting(200, std::string(kResponseBody)))); + RestTableScanBuilder builder(metadata_, file_io_, "test.my_table", nullptr, MakeContext(std::nullopt)); builder.UseSnapshot(kSnapshotId); ICEBERG_UNWRAP_OR_FAIL(auto scan, builder.Build()); - EXPECT_TRUE(scan->context().use_snapshot_schema); + ICEBERG_UNWRAP_OR_FAIL(auto tasks, scan->PlanFiles()); + EXPECT_TRUE(tasks.empty()); } // -------------------------------------------------------------------------- -// use_snapshot_schema: default scan does not set use_snapshot_schema. +// Default scan: POST body must have "use-snapshot-schema": false and no +// "snapshot-id" field. // -------------------------------------------------------------------------- TEST_F(RestTableScanTest, DefaultScanDoesNotSetUseSnapshotSchema) { + constexpr std::string_view kResponseBody = R"({"status":"completed"})"; + + EXPECT_CALL(*mock_client_, + Post(_, testing::AllOf(JsonBodyHas("use-snapshot-schema", false), + JsonBodyLacks("snapshot-id")), + _, _, _)) + .WillOnce(Return(HttpResponse::MakeForTesting(200, std::string(kResponseBody)))); + RestTableScanBuilder builder(metadata_, file_io_, "test.my_table", nullptr, MakeContext(std::nullopt)); ICEBERG_UNWRAP_OR_FAIL(auto scan, builder.Build()); - EXPECT_FALSE(scan->context().use_snapshot_schema); + ICEBERG_UNWRAP_OR_FAIL(auto tasks, scan->PlanFiles()); + EXPECT_TRUE(tasks.empty()); } // -------------------------------------------------------------------------- @@ -547,6 +602,75 @@ TEST_F(RestTableScanTest, PlanFilesStreamFailed) { EXPECT_THAT(result, IsError(ErrorKind::kIOError)); } +// -------------------------------------------------------------------------- +// PlanFilesStream: FAILED with plan-id → DELETE /plan called before returning +// the error (tests the ExecuteScanPlanStream kFailed cancel fix). +// -------------------------------------------------------------------------- +TEST_F(RestTableScanTest, PlanFilesStreamFailedWithPlanIdCancels) { + constexpr std::string_view kFailedBody = + R"({"status":"failed","plan-id":"plan-fail-stream","error":{"message":"server error","type":"ServerError","code":500}})"; + + EXPECT_CALL(*mock_client_, Post(_, _, _, _, _)) + .WillOnce(Return(HttpResponse::MakeForTesting(200, std::string(kFailedBody)))); + EXPECT_CALL(*mock_client_, Delete(_, _, _, _, _)) + .WillOnce(Return(HttpResponse::MakeForTesting(200, "{}"))); + + ICEBERG_UNWRAP_OR_FAIL(auto scan, MakeScan(MakeContext())); + auto result = scan->PlanFilesStream(); + EXPECT_THAT(result, IsError(ErrorKind::kIOError)); +} + +// -------------------------------------------------------------------------- +// FetchPlanningResult: GET fails after SUBMITTED → DELETE /plan called before +// returning the error (tests the FetchPlanningResult error-path cancel fix). +// -------------------------------------------------------------------------- +TEST_F(RestTableScanTest, CancelCalledOnFetchPlanningResultGetError) { + constexpr std::string_view kSubmittedBody = + R"({"status":"submitted","plan-id":"plan-fetch-err"})"; + + EXPECT_CALL(*mock_client_, Post(_, _, _, _, _)) + .WillOnce(Return(HttpResponse::MakeForTesting(200, std::string(kSubmittedBody)))); + EXPECT_CALL(*mock_client_, Get(_, _, _, _, _)) + .WillOnce(Return(IOError("network failure"))); + EXPECT_CALL(*mock_client_, Delete(_, _, _, _, _)) + .WillOnce(Return(HttpResponse::MakeForTesting(200, "{}"))); + + ICEBERG_UNWRAP_OR_FAIL(auto scan, MakeScan(MakeContext())); + auto result = scan->PlanFiles(); + EXPECT_THAT(result, IsError(ErrorKind::kIOError)); +} + +// -------------------------------------------------------------------------- +// Stale credentials: second PlanFilesStream call resets scan_io_slot_ so that +// credentials from the first plan response do not persist into the second. +// -------------------------------------------------------------------------- +TEST_F(RestTableScanTest, SecondPlanFilesStreamCallClearsStaleCredentials) { + constexpr std::string_view kFirstResponse = R"({ + "status": "completed", + "storage-credentials": [ + {"prefix": "s3://bucket/prefix", "config": {"key": "value"}} + ] + })"; + constexpr std::string_view kSecondResponse = R"({"status":"completed"})"; + + EXPECT_CALL(*mock_client_, Post(_, _, _, _, _)) + .WillOnce(Return(HttpResponse::MakeForTesting(200, std::string(kFirstResponse)))) + .WillOnce(Return(HttpResponse::MakeForTesting(200, std::string(kSecondResponse)))); + + ICEBERG_UNWRAP_OR_FAIL(auto scan, MakeScan(MakeContext())); + + // First call: server vends credentials → io() returns a credential-scoped IO. + ICEBERG_UNWRAP_OR_FAIL(auto tasks1, scan->PlanFiles()); + EXPECT_TRUE(tasks1.empty()); + EXPECT_NE(scan->io().get(), file_io_.get()); + + // Second call: no credentials returned → io() must revert to the table IO, + // not retain the credentials from the first plan. + ICEBERG_UNWRAP_OR_FAIL(auto tasks2, scan->PlanFiles()); + EXPECT_TRUE(tasks2.empty()); + EXPECT_EQ(scan->io().get(), file_io_.get()); +} + // ========================================================================== // RestIncrementalAppendScan tests // ========================================================================== @@ -675,12 +799,13 @@ TEST_F(RestIncrementalAppendScanTest, PlanFilesEmptyWhenNoCurrentSnapshot) { } // -------------------------------------------------------------------------- -// Explicit to_snapshot_id: request uses that snapshot, not current. +// Explicit to_snapshot_id: POST body must contain "end-snapshot-id" set to +// the given value, not the current table snapshot. // -------------------------------------------------------------------------- TEST_F(RestIncrementalAppendScanTest, PlanFilesWithExplicitToSnapshotId) { constexpr int64_t kToSnapshotId = 1000L; constexpr std::string_view kResponseBody = R"({"status":"completed"})"; - EXPECT_CALL(*mock_client_, Post(_, _, _, _, _)) + EXPECT_CALL(*mock_client_, Post(_, JsonBodyHas("end-snapshot-id", kToSnapshotId), _, _, _)) .WillOnce(Return(HttpResponse::MakeForTesting(200, std::string(kResponseBody)))); ICEBERG_UNWRAP_OR_FAIL( @@ -690,12 +815,17 @@ TEST_F(RestIncrementalAppendScanTest, PlanFilesWithExplicitToSnapshotId) { } // -------------------------------------------------------------------------- -// Exclusive from_snapshot_id: passed directly as start_snapshot_id. +// Exclusive from_snapshot_id: POST body must pass from_snapshot_id directly +// as "start-snapshot-id" (exclusive), and current snapshot as "end-snapshot-id". // -------------------------------------------------------------------------- TEST_F(RestIncrementalAppendScanTest, PlanFilesWithFromSnapshotIdExclusive) { constexpr int64_t kFromSnapshotId = 999L; + constexpr int64_t kCurrentSnapshotId = 1000L; constexpr std::string_view kResponseBody = R"({"status":"completed"})"; - EXPECT_CALL(*mock_client_, Post(_, _, _, _, _)) + EXPECT_CALL(*mock_client_, + Post(_, testing::AllOf(JsonBodyHas("start-snapshot-id", kFromSnapshotId), + JsonBodyHas("end-snapshot-id", kCurrentSnapshotId)), + _, _, _)) .WillOnce(Return(HttpResponse::MakeForTesting(200, std::string(kResponseBody)))); ICEBERG_UNWRAP_OR_FAIL( @@ -706,12 +836,16 @@ TEST_F(RestIncrementalAppendScanTest, PlanFilesWithFromSnapshotIdExclusive) { // -------------------------------------------------------------------------- // Inclusive from_snapshot_id: parent snapshot is used as start_snapshot_id. -// The fixture snapshot (id=1000) has no parent, so start_snapshot_id = nullopt. +// The fixture snapshot (id=1000) has no parent, so "start-snapshot-id" is +// absent from the POST body. // -------------------------------------------------------------------------- TEST_F(RestIncrementalAppendScanTest, PlanFilesWithFromSnapshotIdInclusiveNoParent) { constexpr int64_t kFromSnapshotId = 1000L; constexpr std::string_view kResponseBody = R"({"status":"completed"})"; - EXPECT_CALL(*mock_client_, Post(_, _, _, _, _)) + EXPECT_CALL(*mock_client_, + Post(_, testing::AllOf(JsonBodyLacks("start-snapshot-id"), + JsonBodyHas("end-snapshot-id", kFromSnapshotId)), + _, _, _)) .WillOnce(Return(HttpResponse::MakeForTesting(200, std::string(kResponseBody)))); ICEBERG_UNWRAP_OR_FAIL( @@ -721,11 +855,60 @@ TEST_F(RestIncrementalAppendScanTest, PlanFilesWithFromSnapshotIdInclusiveNoPare } // -------------------------------------------------------------------------- -// Inclusive from_snapshot_id with a parent: parent's id used as start. +// PlanFiles: FAILED with plan-id → DELETE /plan called before returning the +// error (tests the ExecuteScanPlan kFailed cancel fix). +// -------------------------------------------------------------------------- +TEST_F(RestIncrementalAppendScanTest, PlanFilesFailedWithPlanIdCancels) { + constexpr std::string_view kFailedBody = + R"({"status":"failed","plan-id":"plan-fail-incr","error":{"message":"server error","type":"ServerError","code":500}})"; + + EXPECT_CALL(*mock_client_, Post(_, _, _, _, _)) + .WillOnce(Return(HttpResponse::MakeForTesting(200, std::string(kFailedBody)))); + EXPECT_CALL(*mock_client_, Delete(_, _, _, _, _)) + .WillOnce(Return(HttpResponse::MakeForTesting(200, "{}"))); + + ICEBERG_UNWRAP_OR_FAIL(auto scan, MakeIncrementalScan(MakeContext())); + auto result = scan->PlanFiles(); + EXPECT_THAT(result, IsError(ErrorKind::kIOError)); +} + +// -------------------------------------------------------------------------- +// Stale credentials: second PlanFiles call resets scan_io_ so that credentials +// from the first plan response do not persist into the second. +// -------------------------------------------------------------------------- +TEST_F(RestIncrementalAppendScanTest, SecondPlanFilesCallClearsStaleCredentials) { + constexpr std::string_view kFirstResponse = R"({ + "status": "completed", + "storage-credentials": [ + {"prefix": "s3://bucket/prefix", "config": {"key": "value"}} + ] + })"; + constexpr std::string_view kSecondResponse = R"({"status":"completed"})"; + + EXPECT_CALL(*mock_client_, Post(_, _, _, _, _)) + .WillOnce(Return(HttpResponse::MakeForTesting(200, std::string(kFirstResponse)))) + .WillOnce(Return(HttpResponse::MakeForTesting(200, std::string(kSecondResponse)))); + + ICEBERG_UNWRAP_OR_FAIL(auto scan, MakeIncrementalScan(MakeContext())); + + // First call: server vends credentials. + ICEBERG_UNWRAP_OR_FAIL(auto tasks1, scan->PlanFiles()); + EXPECT_TRUE(tasks1.empty()); + + // Second call: no credentials. Verifies scan_io_ was cleared so stale + // credentials from the first plan do not bleed into the second request. + ICEBERG_UNWRAP_OR_FAIL(auto tasks2, scan->PlanFiles()); + EXPECT_TRUE(tasks2.empty()); +} + +// -------------------------------------------------------------------------- +// Inclusive from_snapshot_id with a parent: POST body must use the parent's id +// as "start-snapshot-id" and the current snapshot as "end-snapshot-id". // -------------------------------------------------------------------------- TEST_F(RestIncrementalAppendScanTest, PlanFilesWithFromSnapshotIdInclusiveWithParent) { constexpr int64_t kParentSnapshotId = 900L; constexpr int64_t kChildSnapshotId = 1001L; + constexpr int64_t kCurrentSnapshotId = 1000L; // Add a second snapshot with a parent to the metadata. auto child_snapshot = std::make_shared( @@ -738,7 +921,10 @@ TEST_F(RestIncrementalAppendScanTest, PlanFilesWithFromSnapshotIdInclusiveWithPa metadata_->snapshots.push_back(child_snapshot); constexpr std::string_view kResponseBody = R"({"status":"completed"})"; - EXPECT_CALL(*mock_client_, Post(_, _, _, _, _)) + EXPECT_CALL(*mock_client_, + Post(_, testing::AllOf(JsonBodyHas("start-snapshot-id", kParentSnapshotId), + JsonBodyHas("end-snapshot-id", kCurrentSnapshotId)), + _, _, _)) .WillOnce(Return(HttpResponse::MakeForTesting(200, std::string(kResponseBody)))); ICEBERG_UNWRAP_OR_FAIL(auto scan, MakeIncrementalScan(MakeContext(), kChildSnapshotId, From c0d99f990c9a27fc47d99e975e1c7faf8780d7ac Mon Sep 17 00:00:00 2001 From: Sandeep Gottimukkala Date: Mon, 28 Sep 2026 22:59:25 -0700 Subject: [PATCH 14/16] test(rest): replace misleading stream comment with true laziness and partial-consume tests - Rename PlanFilesStreamLazilyFetchesPlanTasks to PlanFilesStreamFetchesEachTokenSeparately; verify task content and count rather than empty responses, since the old comment implied laziness was being tested when only call-count was verified - Add PlanFilesStreamFetchesTokenOnlyWhenBufferExhausted: uses a counter incremented inside WillOnce lambdas to assert the second FetchScanTasks POST is not made until the first token buffer is fully drained - Add PlanFilesStreamNextReturnsErrorOnFetchFailure: calls Next() directly and asserts it returns IOError when FetchScanTasks fails - Add PlanFilesStreamCancelAfterPartialConsumption: consumes one task then destroys the stream, verifying DELETE called with a token still pending - Retain PlanFilesStreamCancelOnPartialConsumption with updated comment clarifying it tests destroy-before-any-Next --- src/iceberg/test/rest_table_scan_test.cc | 149 +++++++++++++++++++++-- 1 file changed, 139 insertions(+), 10 deletions(-) diff --git a/src/iceberg/test/rest_table_scan_test.cc b/src/iceberg/test/rest_table_scan_test.cc index 1bad8dd10..6da511733 100644 --- a/src/iceberg/test/rest_table_scan_test.cc +++ b/src/iceberg/test/rest_table_scan_test.cc @@ -531,31 +531,160 @@ TEST_F(RestTableScanTest, PlanFilesStreamCompleted) { } // -------------------------------------------------------------------------- -// PlanFilesStream: COMPLETED with two plan-task tokens; each fetched lazily -// as Next() is called, not all at once on POST /plan. +// PlanFilesStream: two plan-task tokens each trigger a separate FetchScanTasks +// POST, one per token, and the combined task set is returned. // -------------------------------------------------------------------------- -TEST_F(RestTableScanTest, PlanFilesStreamLazilyFetchesPlanTasks) { +TEST_F(RestTableScanTest, PlanFilesStreamFetchesEachTokenSeparately) { constexpr std::string_view kPlanResponse = R"({"status":"completed","plan-id":"plan-stream","plan-tasks":["tok-s1","tok-s2"]})"; - constexpr std::string_view kTasksResponse = R"({"file-scan-tasks":[]})"; + constexpr std::string_view kTask1Response = R"({ + "file-scan-tasks": [ + {"data-file":{"content":"data","file-path":"s3://b/f1.parquet", + "file-format":"PARQUET","spec-id":0,"partition":[],"file-size-in-bytes":1,"record-count":1}} + ] + })"; + constexpr std::string_view kTask2Response = R"({ + "file-scan-tasks": [ + {"data-file":{"content":"data","file-path":"s3://b/f2.parquet", + "file-format":"PARQUET","spec-id":0,"partition":[],"file-size-in-bytes":1,"record-count":1}} + ] + })"; EXPECT_CALL(*mock_client_, Post(_, _, _, _, _)) .WillOnce(Return(HttpResponse::MakeForTesting(200, std::string(kPlanResponse)))) - .WillOnce(Return(HttpResponse::MakeForTesting(200, std::string(kTasksResponse)))) - .WillOnce(Return(HttpResponse::MakeForTesting(200, std::string(kTasksResponse)))); + .WillOnce(Return(HttpResponse::MakeForTesting(200, std::string(kTask1Response)))) + .WillOnce(Return(HttpResponse::MakeForTesting(200, std::string(kTask2Response)))); ICEBERG_UNWRAP_OR_FAIL(auto scan, MakeScan(MakeContext())); ICEBERG_UNWRAP_OR_FAIL(auto stream, scan->PlanFilesStream()); ICEBERG_UNWRAP_OR_FAIL(auto tasks, stream->ToVector()); - EXPECT_TRUE(tasks.empty()); + ASSERT_EQ(tasks.size(), 2u); + EXPECT_EQ(tasks[0]->data_file()->file_path, "s3://b/f1.parquet"); + EXPECT_EQ(tasks[1]->data_file()->file_path, "s3://b/f2.parquet"); +} + +// -------------------------------------------------------------------------- +// PlanFilesStream: the second FetchScanTasks POST is not made until the first +// token's buffer is exhausted. Verified by counting POST calls between Next() +// invocations. +// -------------------------------------------------------------------------- +TEST_F(RestTableScanTest, PlanFilesStreamFetchesTokenOnlyWhenBufferExhausted) { + constexpr std::string_view kPlanResponse = + R"({"status":"completed","plan-id":"plan-lazy","plan-tasks":["tok-1","tok-2"]})"; + // tok-1 returns 2 tasks; tok-2 must not be fetched until both are consumed. + constexpr std::string_view kTwoTasksResponse = R"({ + "file-scan-tasks": [ + {"data-file":{"content":"data","file-path":"s3://b/f1.parquet", + "file-format":"PARQUET","spec-id":0,"partition":[],"file-size-in-bytes":1,"record-count":1}}, + {"data-file":{"content":"data","file-path":"s3://b/f2.parquet", + "file-format":"PARQUET","spec-id":0,"partition":[],"file-size-in-bytes":1,"record-count":1}} + ] + })"; + constexpr std::string_view kOneTaskResponse = R"({ + "file-scan-tasks": [ + {"data-file":{"content":"data","file-path":"s3://b/f3.parquet", + "file-format":"PARQUET","spec-id":0,"partition":[],"file-size-in-bytes":1,"record-count":1}} + ] + })"; + + int fetch_count = 0; + EXPECT_CALL(*mock_client_, Post(_, _, _, _, _)) + .WillOnce(Return(HttpResponse::MakeForTesting(200, std::string(kPlanResponse)))) + .WillOnce([&](auto&&...) -> Result { + ++fetch_count; + return HttpResponse::MakeForTesting(200, std::string(kTwoTasksResponse)); + }) + .WillOnce([&](auto&&...) -> Result { + ++fetch_count; + return HttpResponse::MakeForTesting(200, std::string(kOneTaskResponse)); + }); + + ICEBERG_UNWRAP_OR_FAIL(auto scan, MakeScan(MakeContext())); + ICEBERG_UNWRAP_OR_FAIL(auto stream, scan->PlanFilesStream()); + + // No FetchScanTasks call yet — stream has not been driven. + EXPECT_EQ(fetch_count, 0); + + // First Next(): fetches tok-1 (2 tasks buffered), returns f1. + ICEBERG_UNWRAP_OR_FAIL(auto t1, stream->Next()); + ASSERT_TRUE(t1.has_value()); + EXPECT_EQ(fetch_count, 1); + EXPECT_EQ((*t1)->data_file()->file_path, "s3://b/f1.parquet"); + + // Second Next(): served from buffer; tok-2 not fetched yet. + ICEBERG_UNWRAP_OR_FAIL(auto t2, stream->Next()); + ASSERT_TRUE(t2.has_value()); + EXPECT_EQ(fetch_count, 1); + EXPECT_EQ((*t2)->data_file()->file_path, "s3://b/f2.parquet"); + + // Third Next(): buffer exhausted, fetches tok-2, returns f3. + ICEBERG_UNWRAP_OR_FAIL(auto t3, stream->Next()); + ASSERT_TRUE(t3.has_value()); + EXPECT_EQ(fetch_count, 2); + EXPECT_EQ((*t3)->data_file()->file_path, "s3://b/f3.parquet"); + + // Fourth Next(): all tokens consumed, stream terminates. + ICEBERG_UNWRAP_OR_FAIL(auto end, stream->Next()); + EXPECT_FALSE(end.has_value()); +} + +// -------------------------------------------------------------------------- +// PlanFilesStream: Next() propagates a FetchScanTasks error and DELETE /plan +// is called via the stream destructor since consumed_ is never set. +// -------------------------------------------------------------------------- +TEST_F(RestTableScanTest, PlanFilesStreamNextReturnsErrorOnFetchFailure) { + constexpr std::string_view kPlanResponse = + R"({"status":"completed","plan-id":"plan-next-err","plan-tasks":["tok-err"]})"; + + EXPECT_CALL(*mock_client_, Post(_, _, _, _, _)) + .WillOnce(Return(HttpResponse::MakeForTesting(200, std::string(kPlanResponse)))) + .WillOnce(Return(IOError("FetchScanTasks network error"))); + EXPECT_CALL(*mock_client_, Delete(_, _, _, _, _)) + .WillOnce(Return(HttpResponse::MakeForTesting(200, "{}"))); + + ICEBERG_UNWRAP_OR_FAIL(auto scan, MakeScan(MakeContext())); + ICEBERG_UNWRAP_OR_FAIL(auto stream, scan->PlanFilesStream()); + auto result = stream->Next(); + EXPECT_THAT(result, IsError(ErrorKind::kIOError)); +} + +// -------------------------------------------------------------------------- +// PlanFilesStream: stream destroyed after consuming the first task but before +// the second token is fetched → DELETE /plan called by the destructor. +// -------------------------------------------------------------------------- +TEST_F(RestTableScanTest, PlanFilesStreamCancelAfterPartialConsumption) { + constexpr std::string_view kPlanResponse = + R"({"status":"completed","plan-id":"plan-partial","plan-tasks":["tok-p1","tok-p2"]})"; + constexpr std::string_view kTaskResponse = R"({ + "file-scan-tasks": [ + {"data-file":{"content":"data","file-path":"s3://b/fp1.parquet", + "file-format":"PARQUET","spec-id":0,"partition":[],"file-size-in-bytes":1,"record-count":1}} + ] + })"; + + EXPECT_CALL(*mock_client_, Post(_, _, _, _, _)) + .WillOnce(Return(HttpResponse::MakeForTesting(200, std::string(kPlanResponse)))) + .WillOnce(Return(HttpResponse::MakeForTesting(200, std::string(kTaskResponse)))); + EXPECT_CALL(*mock_client_, Delete(_, _, _, _, _)) + .WillOnce(Return(HttpResponse::MakeForTesting(200, "{}"))); + + ICEBERG_UNWRAP_OR_FAIL(auto scan, MakeScan(MakeContext())); + { + ICEBERG_UNWRAP_OR_FAIL(auto stream, scan->PlanFilesStream()); + // Consume the first task from tok-p1; tok-p2 has never been fetched. + ICEBERG_UNWRAP_OR_FAIL(auto task, stream->Next()); + ASSERT_TRUE(task.has_value()); + // Destroy stream here — tok-p2 is still pending, so destructor calls DELETE. + } } // -------------------------------------------------------------------------- -// PlanFilesStream: stream destroyed before full consumption → DELETE /plan. +// PlanFilesStream: stream destroyed with no Next() calls at all → +// DELETE /plan called by the destructor. // -------------------------------------------------------------------------- TEST_F(RestTableScanTest, PlanFilesStreamCancelOnPartialConsumption) { constexpr std::string_view kPlanResponse = - R"({"status":"completed","plan-id":"plan-partial","plan-tasks":["tok-p1"]})"; + R"({"status":"completed","plan-id":"plan-never-consumed","plan-tasks":["tok-p1"]})"; EXPECT_CALL(*mock_client_, Post(_, _, _, _, _)) .WillOnce(Return(HttpResponse::MakeForTesting(200, std::string(kPlanResponse)))); @@ -565,7 +694,7 @@ TEST_F(RestTableScanTest, PlanFilesStreamCancelOnPartialConsumption) { ICEBERG_UNWRAP_OR_FAIL(auto scan, MakeScan(MakeContext())); { ICEBERG_UNWRAP_OR_FAIL(auto stream, scan->PlanFilesStream()); - // Destroy without consuming — destructor must call DELETE /plan. + // Destroy without any Next() call — destructor must call DELETE /plan. } } From 31b2182028202cdddb4e9938e990934995b52241 Mon Sep 17 00:00:00 2001 From: Sandeep Gottimukkala Date: Mon, 28 Sep 2026 23:19:58 -0700 Subject: [PATCH 15/16] test(rest): add RestCatalog::LoadTable scan-planning-mode selection tests - Add MakeForTesting() factory to RestCatalog that bypasses FetchServerConfig, enabling unit tests to inject pre-built client/paths/endpoints. - Add rest_catalog_unit_test.cc with four tests covering: table config server scan -> RestTable, client config server scan -> RestTable, server scan with missing PlanTableScan endpoint -> NotSupported, and default (no config) -> plain Table. - Fix RestIncrementalAppendScan::PlanFiles() to check kInvalidSnapshotId before calling Snapshot(), so a table with no current snapshot returns empty results rather than a NotFound error. - Fix make_unique deduction failure by replacing brace-init {} with std::vector{} for plan_task_tokens arg. - Relax PlanTableScanResponse::Validate() to allow plan-id in failed responses, enabling the cancel-on-fail code path to be reached. - Update rest_json_serde_test.cc: remove FailedWithPlanId invalid case, add FailedWithPlanId roundtrip test. - Add iceberg/manifest/manifest_entry.h and iceberg/constants.h includes to rest_table_scan_test.cc to fix DataFile and kInvalidSnapshotId access. --- src/iceberg/catalog/rest/rest_catalog.cc | 18 ++ src/iceberg/catalog/rest/rest_catalog.h | 7 + src/iceberg/catalog/rest/rest_table_scan.cc | 5 +- src/iceberg/catalog/rest/types.cc | 6 +- src/iceberg/test/CMakeLists.txt | 1 + src/iceberg/test/rest_catalog_unit_test.cc | 214 ++++++++++++++++++++ src/iceberg/test/rest_json_serde_test.cc | 17 +- src/iceberg/test/rest_table_scan_test.cc | 3 + 8 files changed, 260 insertions(+), 11 deletions(-) create mode 100644 src/iceberg/test/rest_catalog_unit_test.cc diff --git a/src/iceberg/catalog/rest/rest_catalog.cc b/src/iceberg/catalog/rest/rest_catalog.cc index 5a8bb2507..5e3339878 100644 --- a/src/iceberg/catalog/rest/rest_catalog.cc +++ b/src/iceberg/catalog/rest/rest_catalog.cc @@ -28,6 +28,7 @@ #include +#include "iceberg/catalog/rest/auth/auth_manager_internal.h" #include "iceberg/catalog/rest/auth/auth_managers.h" #include "iceberg/catalog/rest/auth/auth_session.h" #include "iceberg/catalog/rest/catalog_properties.h" @@ -455,6 +456,23 @@ Result> RestCatalog::Make( snapshot_mode, std::move(default_context), std::move(reporter), metrics_executor)); } +Result> RestCatalog::MakeForTesting( + RestCatalogProperties config, std::shared_ptr file_io, + std::shared_ptr client, std::shared_ptr paths, + std::unordered_set endpoints) { + std::string catalog_name = config.Get(RestCatalogProperties::kName); + ICEBERG_ASSIGN_OR_RAISE( + auto auth_manager, + auth::MakeNoopAuthManager(catalog_name, config.configs())); + auto session = auth::AuthSession::MakeDefault({}); + ICEBERG_ASSIGN_OR_RAISE(auto snapshot_mode, config.SnapshotLoadingMode()); + return std::shared_ptr( + new RestCatalog(std::move(config), std::move(file_io), std::move(client), + std::move(paths), std::move(endpoints), std::move(auth_manager), + std::move(session), snapshot_mode, SessionContext::Empty(), + /*reporter=*/nullptr, /*metrics_executor=*/nullptr)); +} + RestCatalog::RestCatalog(RestCatalogProperties config, std::shared_ptr file_io, std::shared_ptr client, std::shared_ptr paths, diff --git a/src/iceberg/catalog/rest/rest_catalog.h b/src/iceberg/catalog/rest/rest_catalog.h index 8b194b668..52a033178 100644 --- a/src/iceberg/catalog/rest/rest_catalog.h +++ b/src/iceberg/catalog/rest/rest_catalog.h @@ -58,6 +58,13 @@ class ICEBERG_REST_EXPORT RestCatalog final static Result> Make(const RestCatalogProperties& config, Executor* metrics_executor = nullptr); + /// \brief Test-only factory that constructs a RestCatalog with pre-built dependencies, + /// bypassing the FetchServerConfig HTTP exchange performed by Make(). + static Result> MakeForTesting( + RestCatalogProperties config, std::shared_ptr file_io, + std::shared_ptr client, std::shared_ptr paths, + std::unordered_set endpoints); + std::string_view name() const override; Result> AsCatalog() override; diff --git a/src/iceberg/catalog/rest/rest_table_scan.cc b/src/iceberg/catalog/rest/rest_table_scan.cc index dbb805655..84b6059cd 100644 --- a/src/iceberg/catalog/rest/rest_table_scan.cc +++ b/src/iceberg/catalog/rest/rest_table_scan.cc @@ -31,6 +31,7 @@ #include "iceberg/catalog/rest/resource_paths.h" #include "iceberg/catalog/rest/rest_file_io.h" #include "iceberg/catalog/rest/types.h" +#include "iceberg/constants.h" #include "iceberg/json_serde_internal.h" #include "iceberg/partition_spec.h" #include "iceberg/result.h" @@ -297,7 +298,8 @@ Result ExecuteScanPlanStream(const RestScanContext& ctx, ICEBERG_ASSIGN_OR_RAISE( auto tasks, FetchPlanningResult(ctx, schema, mutable_plan_id, specs, *scan_io_slot)); return std::make_unique(ctx, schema_ptr, mutable_plan_id, - std::move(tasks), {}, specs, scan_io_slot); + std::move(tasks), std::vector{}, + specs, scan_io_slot); } if (result.plan_status == PlanStatus::kFailed) { @@ -496,6 +498,7 @@ Result>> RestIncrementalAppendScan::Pl if (context_.to_snapshot_id.has_value()) { request.end_snapshot_id = context_.to_snapshot_id; } else { + if (metadata_->current_snapshot_id == kInvalidSnapshotId) return {}; ICEBERG_ASSIGN_OR_RAISE(auto snapshot, metadata_->Snapshot()); if (!snapshot) return {}; request.end_snapshot_id = snapshot->snapshot_id; diff --git a/src/iceberg/catalog/rest/types.cc b/src/iceberg/catalog/rest/types.cc index ef20f44da..7384e2e39 100644 --- a/src/iceberg/catalog/rest/types.cc +++ b/src/iceberg/catalog/rest/types.cc @@ -298,10 +298,10 @@ Status PlanTableScanResponse::Validate() const { "Invalid response: tasks can only be defined when status is 'completed'"); } if (!plan_id.empty() && plan_status != PlanStatus::kSubmitted && - plan_status != PlanStatus::kCompleted) { + plan_status != PlanStatus::kCompleted && plan_status != PlanStatus::kFailed) { return ValidationFailed( - "Invalid response: plan id can only be defined when status is 'submitted' or " - "'completed'"); + "Invalid response: plan id can only be defined when status is 'submitted', " + "'completed', or 'failed'"); } if (!HasNonEmptyFileScanTasks(*this) && !delete_files.empty()) { return ValidationFailed( diff --git a/src/iceberg/test/CMakeLists.txt b/src/iceberg/test/CMakeLists.txt index 0a2166ed6..70694187a 100644 --- a/src/iceberg/test/CMakeLists.txt +++ b/src/iceberg/test/CMakeLists.txt @@ -321,6 +321,7 @@ if(ICEBERG_BUILD_REST) catalog_properties_test.cc error_handlers_test.cc endpoint_test.cc + rest_catalog_unit_test.cc rest_file_io_test.cc rest_json_serde_test.cc rest_metrics_reporter_test.cc diff --git a/src/iceberg/test/rest_catalog_unit_test.cc b/src/iceberg/test/rest_catalog_unit_test.cc new file mode 100644 index 000000000..4a822a64a --- /dev/null +++ b/src/iceberg/test/rest_catalog_unit_test.cc @@ -0,0 +1,214 @@ +/* + * 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/catalog/rest/rest_catalog.h" + +#include +#include +#include +#include + +#include +#include + +#include "iceberg/catalog/rest/catalog_properties.h" +#include "iceberg/catalog/rest/endpoint.h" +#include "iceberg/catalog/rest/error_handlers.h" +#include "iceberg/catalog/rest/http_client.h" +#include "iceberg/catalog/rest/resource_paths.h" +#include "iceberg/catalog/rest/rest_table.h" +#include "iceberg/file_io.h" +#include "iceberg/table_identifier.h" +#include "iceberg/test/matchers.h" + +namespace iceberg::rest { + +using ::testing::_; +using ::testing::Return; + +// -------------------------------------------------------------------------- +// Mock HTTP client (same pattern as rest_table_scan_test.cc) +// -------------------------------------------------------------------------- +class MockHttpClient : public HttpClient { + public: + MockHttpClient() : HttpClient({}) {} + + MOCK_METHOD(Result, Get, + (const std::string& path, + (const std::unordered_map&)params, + (const std::unordered_map&)headers, + const ErrorHandler& error_handler, auth::AuthSession& session), + (override)); + + MOCK_METHOD(Result, Post, + (const std::string& path, const std::string& body, + (const std::unordered_map&)headers, + const ErrorHandler& error_handler, auth::AuthSession& session), + (override)); + + MOCK_METHOD(Result, Delete, + (const std::string& path, + (const std::unordered_map&)params, + (const std::unordered_map&)headers, + const ErrorHandler& error_handler, auth::AuthSession& session), + (override)); +}; + +// -------------------------------------------------------------------------- +// Minimal FileIO stub +// -------------------------------------------------------------------------- +class NoOpFileIO : public FileIO { + public: + Result ReadFile(const std::string&, std::optional) override { + return IOError("NoOpFileIO"); + } + Status WriteFile(const std::string&, std::string_view) override { return {}; } + Status DeleteFile(const std::string&) override { return {}; } +}; + +// -------------------------------------------------------------------------- +// Helper JSON bodies for LoadTable GET responses +// -------------------------------------------------------------------------- + +// Minimal table metadata blob (no snapshots, no partitions). +constexpr std::string_view kBaseMetadataJson = + R"("metadata-location":"s3://bucket/metadata/v1.json","metadata":{"format-version":2,"table-uuid":"test-uuid","location":"s3://bucket/test","last-sequence-number":0,"last-updated-ms":0,"last-column-id":1,"schemas":[{"type":"struct","schema-id":1,"fields":[{"id":1,"name":"id","type":"int","required":true}]}],"current-schema-id":1,"partition-specs":[{"spec-id":0,"fields":[]}],"default-spec-id":0,"last-partition-id":0,"sort-orders":[{"order-id":0,"fields":[]}],"default-sort-order-id":0,"properties":{}})"; + +// LoadTable response where server config requests server-side scan planning. +const std::string kLoadTableServerScanResponse = + std::string("{\"config\":{\"scan-planning-mode\":\"server\"},") + + std::string(kBaseMetadataJson) + "}"; + +// LoadTable response with no scan-planning-mode set (defaults to client). +const std::string kLoadTableDefaultResponse = + std::string("{") + std::string(kBaseMetadataJson) + "}"; + +// -------------------------------------------------------------------------- +// Test fixture: builds a RestCatalog via MakeForTesting so no HTTP calls +// are made during catalog construction. +// -------------------------------------------------------------------------- +class RestCatalogLoadTableTest : public ::testing::Test { + protected: + void SetUp() override { + mock_client_ = std::make_shared(); + file_io_ = std::make_shared(); + + ICEBERG_UNWRAP_OR_FAIL(paths_, + ResourcePaths::Make("http://test-server", /*prefix=*/"", + /*namespace_separator=*/"%1F")); + + identifier_ = TableIdentifier{.ns = Namespace{{"default"}}, .name = "my_table"}; + + all_plan_endpoints_ = {Endpoint::LoadTable(), Endpoint::PlanTableScan(), + Endpoint::FetchPlanningResult(), Endpoint::CancelPlanning(), + Endpoint::FetchScanTasks()}; + + no_plan_endpoint_set_ = {Endpoint::LoadTable()}; + } + + // Builds a RestCatalog with the given supported endpoints and an optional + // client-side scan-planning-mode setting. + Result> MakeCatalog( + const std::unordered_set& endpoints, + const std::string& client_scan_mode = "") { + std::unordered_map props{ + {"uri", "http://test-server"}, + }; + if (!client_scan_mode.empty()) { + props["scan-planning-mode"] = client_scan_mode; + } + auto config = RestCatalogProperties::FromMap(props); + return RestCatalog::MakeForTesting(std::move(config), file_io_, mock_client_, paths_, + endpoints); + } + + std::shared_ptr mock_client_; + std::shared_ptr file_io_; + std::shared_ptr paths_; + TableIdentifier identifier_; + std::unordered_set all_plan_endpoints_; + std::unordered_set no_plan_endpoint_set_; +}; + +// -------------------------------------------------------------------------- +// Table config "scan-planning-mode":"server" → LoadTable returns a RestTable. +// -------------------------------------------------------------------------- +TEST_F(RestCatalogLoadTableTest, TableConfigServerScanReturnsRestTable) { + EXPECT_CALL(*mock_client_, Get(_, _, _, _, _)) + .WillOnce( + Return(HttpResponse::MakeForTesting(200, kLoadTableServerScanResponse))); + + ICEBERG_UNWRAP_OR_FAIL(auto catalog, MakeCatalog(all_plan_endpoints_)); + ICEBERG_UNWRAP_OR_FAIL(auto as_catalog, catalog->AsCatalog()); + ICEBERG_UNWRAP_OR_FAIL(auto table, as_catalog->LoadTable(identifier_)); + + EXPECT_NE(dynamic_cast(table.get()), nullptr) + << "Expected RestTable when server config requests server-side scan planning"; +} + +// -------------------------------------------------------------------------- +// Client config "scan-planning-mode":"server", table config absent → +// LoadTable returns a RestTable. +// -------------------------------------------------------------------------- +TEST_F(RestCatalogLoadTableTest, ClientConfigServerScanReturnsRestTable) { + EXPECT_CALL(*mock_client_, Get(_, _, _, _, _)) + .WillOnce(Return(HttpResponse::MakeForTesting(200, kLoadTableDefaultResponse))); + + ICEBERG_UNWRAP_OR_FAIL(auto catalog, + MakeCatalog(all_plan_endpoints_, /*client_scan_mode=*/"server")); + ICEBERG_UNWRAP_OR_FAIL(auto as_catalog, catalog->AsCatalog()); + ICEBERG_UNWRAP_OR_FAIL(auto table, as_catalog->LoadTable(identifier_)); + + EXPECT_NE(dynamic_cast(table.get()), nullptr) + << "Expected RestTable when client config requests server-side scan planning"; +} + +// -------------------------------------------------------------------------- +// "scan-planning-mode":"server" but PlanTableScan endpoint missing → +// LoadTable returns NotSupported. +// -------------------------------------------------------------------------- +TEST_F(RestCatalogLoadTableTest, ServerScanWithMissingEndpointReturnsNotSupported) { + EXPECT_CALL(*mock_client_, Get(_, _, _, _, _)) + .WillOnce( + Return(HttpResponse::MakeForTesting(200, kLoadTableServerScanResponse))); + + ICEBERG_UNWRAP_OR_FAIL(auto catalog, MakeCatalog(no_plan_endpoint_set_)); + ICEBERG_UNWRAP_OR_FAIL(auto as_catalog, catalog->AsCatalog()); + auto result = as_catalog->LoadTable(identifier_); + + EXPECT_THAT(result, IsError(ErrorKind::kNotSupported)); +} + +// -------------------------------------------------------------------------- +// No scan-planning-mode set anywhere → LoadTable returns a plain Table +// (not RestTable). +// -------------------------------------------------------------------------- +TEST_F(RestCatalogLoadTableTest, DefaultScanModeReturnsPlainTable) { + EXPECT_CALL(*mock_client_, Get(_, _, _, _, _)) + .WillOnce(Return(HttpResponse::MakeForTesting(200, kLoadTableDefaultResponse))); + + ICEBERG_UNWRAP_OR_FAIL(auto catalog, MakeCatalog(all_plan_endpoints_)); + ICEBERG_UNWRAP_OR_FAIL(auto as_catalog, catalog->AsCatalog()); + ICEBERG_UNWRAP_OR_FAIL(auto table, as_catalog->LoadTable(identifier_)); + + EXPECT_EQ(dynamic_cast(table.get()), nullptr) + << "Expected plain Table (not RestTable) when no scan-planning-mode is configured"; +} + +} // namespace iceberg::rest diff --git a/src/iceberg/test/rest_json_serde_test.cc b/src/iceberg/test/rest_json_serde_test.cc index ec41e4a66..64e992d18 100644 --- a/src/iceberg/test/rest_json_serde_test.cc +++ b/src/iceberg/test/rest_json_serde_test.cc @@ -1569,13 +1569,6 @@ INSTANTIATE_TEST_SUITE_P( R"({"status":"submitted","plan-id":"somePlanId","plan-tasks":[]})", .expected_error_kind = ErrorKind::kValidationFailed, .expected_error_msg = "tasks can only be defined when status is 'completed'"}, - PlanTableScanResponseInvalidParam{ - .test_name = "FailedWithPlanId", - .json_str = - R"({"status":"failed","plan-id":"somePlanId","error":{"message":"x","type":"y","code":500}})", - .expected_error_kind = ErrorKind::kValidationFailed, - .expected_error_msg = - "plan id can only be defined when status is 'submitted' or 'completed'"}, PlanTableScanResponseInvalidParam{ .test_name = "FailedWithoutError", .json_str = R"({"status":"failed"})", @@ -2604,6 +2597,16 @@ TEST(PlanTableScanResponseRoundtripTest, FailedWithError) { EXPECT_EQ(*result, *result2); } +TEST(PlanTableScanResponseRoundtripTest, FailedWithPlanId) { + // A server may include a plan-id in a failed response so the client can cancel. + auto json = nlohmann::json::parse( + R"({"status":"failed","plan-id":"plan-123","error":{"message":"Planning failed","type":"PlanningException","code":500}})"); + auto result = PlanTableScanResponseFromJson(json, EmptySpecs(), EmptySchema()); + ASSERT_THAT(result, IsOk()); + EXPECT_EQ(result->plan_id, "plan-123"); + EXPECT_EQ(result->plan_status, PlanStatus::kFailed); +} + TEST(FetchPlanningResultResponseRoundtripTest, CompletedWithPlanTasks) { auto json = nlohmann::json::parse( R"({"status": "completed", "plan-tasks": ["task-1", "task-2"]})"); diff --git a/src/iceberg/test/rest_table_scan_test.cc b/src/iceberg/test/rest_table_scan_test.cc index 6da511733..b3492004f 100644 --- a/src/iceberg/test/rest_table_scan_test.cc +++ b/src/iceberg/test/rest_table_scan_test.cc @@ -35,7 +35,9 @@ #include "iceberg/catalog/rest/http_client.h" #include "iceberg/catalog/rest/resource_paths.h" #include "iceberg/catalog/rest/rest_table.h" +#include "iceberg/constants.h" #include "iceberg/file_io.h" +#include "iceberg/manifest/manifest_entry.h" #include "iceberg/partition_spec.h" #include "iceberg/schema.h" #include "iceberg/snapshot.h" @@ -914,6 +916,7 @@ TEST_F(RestIncrementalAppendScanTest, PlanFilesEmptyWhenNoCurrentSnapshot) { .partition_specs = {spec}, .default_spec_id = spec->spec_id(), .last_partition_id = 999, + .current_snapshot_id = kInvalidSnapshotId, }); EXPECT_CALL(*mock_client_, Post(_, _, _, _, _)).Times(0); From 23d149a483d5ea114e3f77286d610ae861abf906 Mon Sep 17 00:00:00 2001 From: Sandeep Gottimukkala Date: Mon, 28 Sep 2026 23:21:22 -0700 Subject: [PATCH 16/16] style: apply clang-format to rest catalog and scan files --- src/iceberg/catalog/rest/rest_catalog.cc | 5 +- src/iceberg/catalog/rest/rest_table_scan.cc | 84 +++++++++++---------- src/iceberg/test/rest_catalog_unit_test.cc | 11 +-- src/iceberg/test/rest_table_scan_test.cc | 50 ++++++------ 4 files changed, 76 insertions(+), 74 deletions(-) diff --git a/src/iceberg/catalog/rest/rest_catalog.cc b/src/iceberg/catalog/rest/rest_catalog.cc index 5e3339878..e583fbc1c 100644 --- a/src/iceberg/catalog/rest/rest_catalog.cc +++ b/src/iceberg/catalog/rest/rest_catalog.cc @@ -461,9 +461,8 @@ Result> RestCatalog::MakeForTesting( std::shared_ptr client, std::shared_ptr paths, std::unordered_set endpoints) { std::string catalog_name = config.Get(RestCatalogProperties::kName); - ICEBERG_ASSIGN_OR_RAISE( - auto auth_manager, - auth::MakeNoopAuthManager(catalog_name, config.configs())); + ICEBERG_ASSIGN_OR_RAISE(auto auth_manager, + auth::MakeNoopAuthManager(catalog_name, config.configs())); auto session = auth::AuthSession::MakeDefault({}); ICEBERG_ASSIGN_OR_RAISE(auto snapshot_mode, config.SnapshotLoadingMode()); return std::shared_ptr( diff --git a/src/iceberg/catalog/rest/rest_table_scan.cc b/src/iceberg/catalog/rest/rest_table_scan.cc index 84b6059cd..b321f4251 100644 --- a/src/iceberg/catalog/rest/rest_table_scan.cc +++ b/src/iceberg/catalog/rest/rest_table_scan.cc @@ -104,7 +104,8 @@ Result>> ResolveScanTasks( if (plan_tasks.has_value()) { for (const auto& token : *plan_tasks) { - ICEBERG_ASSIGN_OR_RAISE(auto tasks, FetchScanTasks(ctx, schema, token, specs, scan_io)); + ICEBERG_ASSIGN_OR_RAISE(auto tasks, + FetchScanTasks(ctx, schema, token, specs, scan_io)); result.insert(result.end(), tasks.begin(), tasks.end()); } } @@ -125,9 +126,11 @@ Result>> FetchScanTasks( ctx.client->Post(path, json_request, /*headers=*/{}, *PlanTaskErrorHandler::Instance(), *ctx.session)); ICEBERG_ASSIGN_OR_RAISE(auto json, FromJsonString(response.body())); - ICEBERG_ASSIGN_OR_RAISE(auto result, FetchScanTasksResponseFromJson(json, specs, schema)); + ICEBERG_ASSIGN_OR_RAISE(auto result, + FetchScanTasksResponseFromJson(json, specs, schema)); ICEBERG_RETURN_UNEXPECTED(result.Validate()); - ICEBERG_RETURN_UNEXPECTED(ApplyStorageCredentials(ctx, result.storage_credentials, scan_io)); + ICEBERG_RETURN_UNEXPECTED( + ApplyStorageCredentials(ctx, result.storage_credentials, scan_io)); return ResolveScanTasks(ctx, schema, result.plan_tasks, result.file_scan_tasks, specs, scan_io); @@ -172,13 +175,13 @@ Result>> FetchPlanningResult( switch (result.plan_status) { case PlanStatus::kCompleted: { - if (auto s = ApplyStorageCredentials(ctx, result.storage_credentials, scan_io); !s) { + if (auto s = ApplyStorageCredentials(ctx, result.storage_credentials, scan_io); + !s) { CancelPlanning(ctx, plan_id); return std::unexpected(s.error()); } - auto tasks = - ResolveScanTasks(ctx, schema, result.plan_tasks, result.file_scan_tasks, specs, - scan_io); + auto tasks = ResolveScanTasks(ctx, schema, result.plan_tasks, + result.file_scan_tasks, specs, scan_io); if (!tasks) CancelPlanning(ctx, plan_id); return tasks; } @@ -271,23 +274,20 @@ class RestFileScanTaskStream final : public FileScanTaskStream { /// /// scan_io_slot is shared with the caller (RestTableScan) so that credentials /// vended by FetchScanTasks responses update RestTableScan::io() in place. -Result ExecuteScanPlanStream(const RestScanContext& ctx, - const Schema& schema, - std::shared_ptr schema_ptr, - PlanTableScanRequest request, - const SpecsById& specs, - ScanIoSlot scan_io_slot) { +Result ExecuteScanPlanStream( + const RestScanContext& ctx, const Schema& schema, std::shared_ptr schema_ptr, + PlanTableScanRequest request, const SpecsById& specs, ScanIoSlot scan_io_slot) { ICEBERG_ENDPOINT_CHECK(ctx.supported_endpoints, Endpoint::PlanTableScan()); ICEBERG_ASSIGN_OR_RAISE(auto path, ctx.paths->Plan(ctx.identifier)); ICEBERG_ASSIGN_OR_RAISE(auto request_json, ToJson(request)); ICEBERG_ASSIGN_OR_RAISE(auto json_request, ToJsonString(request_json)); - ICEBERG_ASSIGN_OR_RAISE( - const auto response, - ctx.client->Post(path, json_request, /*headers=*/{}, *PlanErrorHandler::Instance(), - *ctx.session)); + ICEBERG_ASSIGN_OR_RAISE(const auto response, + ctx.client->Post(path, json_request, /*headers=*/{}, + *PlanErrorHandler::Instance(), *ctx.session)); ICEBERG_ASSIGN_OR_RAISE(auto json, FromJsonString(response.body())); - ICEBERG_ASSIGN_OR_RAISE(auto result, PlanTableScanResponseFromJson(json, specs, schema)); + ICEBERG_ASSIGN_OR_RAISE(auto result, + PlanTableScanResponseFromJson(json, specs, schema)); ICEBERG_RETURN_UNEXPECTED(result.Validate()); const std::string plan_id = result.plan_id; @@ -295,11 +295,11 @@ Result ExecuteScanPlanStream(const RestScanContext& ctx, if (result.plan_status == PlanStatus::kSubmitted) { // Poll until COMPLETED, eagerly collecting all tasks into the buffer. std::string mutable_plan_id = plan_id; - ICEBERG_ASSIGN_OR_RAISE( - auto tasks, FetchPlanningResult(ctx, schema, mutable_plan_id, specs, *scan_io_slot)); - return std::make_unique(ctx, schema_ptr, mutable_plan_id, - std::move(tasks), std::vector{}, - specs, scan_io_slot); + ICEBERG_ASSIGN_OR_RAISE(auto tasks, FetchPlanningResult(ctx, schema, mutable_plan_id, + specs, *scan_io_slot)); + return std::make_unique( + ctx, schema_ptr, mutable_plan_id, std::move(tasks), std::vector{}, + specs, scan_io_slot); } if (result.plan_status == PlanStatus::kFailed) { @@ -312,7 +312,8 @@ Result ExecuteScanPlanStream(const RestScanContext& ctx, } // kCompleted: apply credentials from the initial response, then build the lazy stream. - if (auto s = ApplyStorageCredentials(ctx, result.storage_credentials, *scan_io_slot); !s) { + if (auto s = ApplyStorageCredentials(ctx, result.storage_credentials, *scan_io_slot); + !s) { CancelPlanning(ctx, plan_id); return std::unexpected(s.error()); } @@ -340,25 +341,25 @@ Result>> ExecuteScanPlan( ICEBERG_ASSIGN_OR_RAISE(auto path, ctx.paths->Plan(ctx.identifier)); ICEBERG_ASSIGN_OR_RAISE(auto request_json, ToJson(request)); ICEBERG_ASSIGN_OR_RAISE(auto json_request, ToJsonString(request_json)); - ICEBERG_ASSIGN_OR_RAISE( - const auto response, - ctx.client->Post(path, json_request, /*headers=*/{}, *PlanErrorHandler::Instance(), - *ctx.session)); + ICEBERG_ASSIGN_OR_RAISE(const auto response, + ctx.client->Post(path, json_request, /*headers=*/{}, + *PlanErrorHandler::Instance(), *ctx.session)); ICEBERG_ASSIGN_OR_RAISE(auto json, FromJsonString(response.body())); - ICEBERG_ASSIGN_OR_RAISE(auto result, PlanTableScanResponseFromJson(json, specs, schema)); + ICEBERG_ASSIGN_OR_RAISE(auto result, + PlanTableScanResponseFromJson(json, specs, schema)); ICEBERG_RETURN_UNEXPECTED(result.Validate()); const std::string plan_id = result.plan_id; switch (result.plan_status) { case PlanStatus::kCompleted: { - if (auto s = ApplyStorageCredentials(ctx, result.storage_credentials, scan_io); !s) { + if (auto s = ApplyStorageCredentials(ctx, result.storage_credentials, scan_io); + !s) { CancelPlanning(ctx, plan_id); return std::unexpected(s.error()); } - auto tasks = - ResolveScanTasks(ctx, schema, result.plan_tasks, result.file_scan_tasks, specs, - scan_io); + auto tasks = ResolveScanTasks(ctx, schema, result.plan_tasks, + result.file_scan_tasks, specs, scan_io); if (!tasks) CancelPlanning(ctx, plan_id); return tasks; } @@ -402,7 +403,8 @@ Result> RestTableScan::Make( } Result RestTableScan::PlanFilesStream() const { - *scan_io_slot_ = nullptr; // reset so stale credentials from a prior plan are not reused + *scan_io_slot_ = + nullptr; // reset so stale credentials from a prior plan are not reused TableMetadataCache metadata_cache(metadata_.get()); ICEBERG_ASSIGN_OR_RAISE(auto specs, metadata_cache.GetPartitionSpecsById()); @@ -430,8 +432,8 @@ Result RestTableScan::PlanFilesStream() const { } } - return ExecuteScanPlanStream(rest_context_, *schema_, schema_, std::move(request), specs, - scan_io_slot_); + return ExecuteScanPlanStream(rest_context_, *schema_, schema_, std::move(request), + specs, scan_io_slot_); } const std::shared_ptr& RestTableScan::io() const { @@ -477,9 +479,9 @@ Result> RestIncrementalAppendScan::Make( ICEBERG_PRECHECK(metadata != nullptr, "Table metadata cannot be null"); ICEBERG_PRECHECK(schema != nullptr, "Schema cannot be null"); ICEBERG_PRECHECK(io != nullptr, "FileIO cannot be null"); - return std::unique_ptr(new RestIncrementalAppendScan( - std::move(metadata), std::move(schema), std::move(io), std::move(context), - std::move(rest_context))); + return std::unique_ptr( + new RestIncrementalAppendScan(std::move(metadata), std::move(schema), std::move(io), + std::move(context), std::move(rest_context))); } Result>> RestIncrementalAppendScan::PlanFiles() @@ -543,8 +545,8 @@ Result> RestIncrementalAppendScanBuilder: ICEBERG_RETURN_UNEXPECTED(CheckErrors()); ICEBERG_RETURN_UNEXPECTED(context_.Validate()); ICEBERG_ASSIGN_OR_RAISE(auto schema, ResolveSnapshotSchema()); - return RestIncrementalAppendScan::Make(metadata_, schema.get(), io_, std::move(context_), - rest_context_); + return RestIncrementalAppendScan::Make(metadata_, schema.get(), io_, + std::move(context_), rest_context_); } } // namespace iceberg::rest diff --git a/src/iceberg/test/rest_catalog_unit_test.cc b/src/iceberg/test/rest_catalog_unit_test.cc index 4a822a64a..26a5e7b18 100644 --- a/src/iceberg/test/rest_catalog_unit_test.cc +++ b/src/iceberg/test/rest_catalog_unit_test.cc @@ -17,8 +17,6 @@ * under the License. */ -#include "iceberg/catalog/rest/rest_catalog.h" - #include #include #include @@ -32,6 +30,7 @@ #include "iceberg/catalog/rest/error_handlers.h" #include "iceberg/catalog/rest/http_client.h" #include "iceberg/catalog/rest/resource_paths.h" +#include "iceberg/catalog/rest/rest_catalog.h" #include "iceberg/catalog/rest/rest_table.h" #include "iceberg/file_io.h" #include "iceberg/table_identifier.h" @@ -115,7 +114,7 @@ class RestCatalogLoadTableTest : public ::testing::Test { identifier_ = TableIdentifier{.ns = Namespace{{"default"}}, .name = "my_table"}; - all_plan_endpoints_ = {Endpoint::LoadTable(), Endpoint::PlanTableScan(), + all_plan_endpoints_ = {Endpoint::LoadTable(), Endpoint::PlanTableScan(), Endpoint::FetchPlanningResult(), Endpoint::CancelPlanning(), Endpoint::FetchScanTasks()}; @@ -151,8 +150,7 @@ class RestCatalogLoadTableTest : public ::testing::Test { // -------------------------------------------------------------------------- TEST_F(RestCatalogLoadTableTest, TableConfigServerScanReturnsRestTable) { EXPECT_CALL(*mock_client_, Get(_, _, _, _, _)) - .WillOnce( - Return(HttpResponse::MakeForTesting(200, kLoadTableServerScanResponse))); + .WillOnce(Return(HttpResponse::MakeForTesting(200, kLoadTableServerScanResponse))); ICEBERG_UNWRAP_OR_FAIL(auto catalog, MakeCatalog(all_plan_endpoints_)); ICEBERG_UNWRAP_OR_FAIL(auto as_catalog, catalog->AsCatalog()); @@ -185,8 +183,7 @@ TEST_F(RestCatalogLoadTableTest, ClientConfigServerScanReturnsRestTable) { // -------------------------------------------------------------------------- TEST_F(RestCatalogLoadTableTest, ServerScanWithMissingEndpointReturnsNotSupported) { EXPECT_CALL(*mock_client_, Get(_, _, _, _, _)) - .WillOnce( - Return(HttpResponse::MakeForTesting(200, kLoadTableServerScanResponse))); + .WillOnce(Return(HttpResponse::MakeForTesting(200, kLoadTableServerScanResponse))); ICEBERG_UNWRAP_OR_FAIL(auto catalog, MakeCatalog(no_plan_endpoint_set_)); ICEBERG_UNWRAP_OR_FAIL(auto as_catalog, catalog->AsCatalog()); diff --git a/src/iceberg/test/rest_table_scan_test.cc b/src/iceberg/test/rest_table_scan_test.cc index b3492004f..e3c61e21c 100644 --- a/src/iceberg/test/rest_table_scan_test.cc +++ b/src/iceberg/test/rest_table_scan_test.cc @@ -62,8 +62,8 @@ MATCHER_P2(JsonBodyHas, key, expected_value, "") { } nlohmann::json expected = expected_value; if (json.at(key) != expected) { - *result_listener << "JSON[\"" << key << "\"] = " << json.at(key) - << ", expected " << expected; + *result_listener << "JSON[\"" << key << "\"] = " << json.at(key) << ", expected " + << expected; return false; } return true; @@ -407,8 +407,9 @@ TEST_F(RestTableScanTest, UseSnapshotPropagatesUseSnapshotSchemaInContext) { constexpr std::string_view kResponseBody = R"({"status":"completed"})"; EXPECT_CALL(*mock_client_, - Post(_, testing::AllOf(JsonBodyHas("snapshot-id", kSnapshotId), - JsonBodyHas("use-snapshot-schema", true)), + Post(_, + testing::AllOf(JsonBodyHas("snapshot-id", kSnapshotId), + JsonBodyHas("use-snapshot-schema", true)), _, _, _)) .WillOnce(Return(HttpResponse::MakeForTesting(200, std::string(kResponseBody)))); @@ -428,8 +429,9 @@ TEST_F(RestTableScanTest, DefaultScanDoesNotSetUseSnapshotSchema) { constexpr std::string_view kResponseBody = R"({"status":"completed"})"; EXPECT_CALL(*mock_client_, - Post(_, testing::AllOf(JsonBodyHas("use-snapshot-schema", false), - JsonBodyLacks("snapshot-id")), + Post(_, + testing::AllOf(JsonBodyHas("use-snapshot-schema", false), + JsonBodyLacks("snapshot-id")), _, _, _)) .WillOnce(Return(HttpResponse::MakeForTesting(200, std::string(kResponseBody)))); @@ -812,8 +814,7 @@ class RestIncrementalAppendScanTest : public RestTableScanTest { // snapshot range. Result> MakeIncrementalScan( RestScanContext ctx, std::optional from_snapshot_id = std::nullopt, - bool from_inclusive = false, - std::optional to_snapshot_id = std::nullopt) { + bool from_inclusive = false, std::optional to_snapshot_id = std::nullopt) { RestIncrementalAppendScanBuilder builder(metadata_, file_io_, "test.my_table", nullptr, std::move(ctx)); if (from_snapshot_id.has_value()) { @@ -892,8 +893,8 @@ TEST_F(RestIncrementalAppendScanTest, PlanFilesFailed) { // PlanFiles: PlanTableScan endpoint missing → NotSupported. // -------------------------------------------------------------------------- TEST_F(RestIncrementalAppendScanTest, PlanFilesEndpointNotSupported) { - ICEBERG_UNWRAP_OR_FAIL(auto scan, - MakeIncrementalScan(MakeContext(std::unordered_set{}))); + ICEBERG_UNWRAP_OR_FAIL( + auto scan, MakeIncrementalScan(MakeContext(std::unordered_set{}))); auto result = scan->PlanFiles(); EXPECT_THAT(result, IsError(ErrorKind::kNotSupported)); } @@ -922,10 +923,9 @@ TEST_F(RestIncrementalAppendScanTest, PlanFilesEmptyWhenNoCurrentSnapshot) { EXPECT_CALL(*mock_client_, Post(_, _, _, _, _)).Times(0); // Use Make() directly to bypass builder validation that requires a snapshot. - ICEBERG_UNWRAP_OR_FAIL( - auto scan, RestIncrementalAppendScan::Make(empty_metadata, schema_, file_io_, - internal::TableScanContext{}, - MakeContext())); + ICEBERG_UNWRAP_OR_FAIL(auto scan, RestIncrementalAppendScan::Make( + empty_metadata, schema_, file_io_, + internal::TableScanContext{}, MakeContext())); ICEBERG_UNWRAP_OR_FAIL(auto tasks, scan->PlanFiles()); EXPECT_TRUE(tasks.empty()); } @@ -937,7 +937,8 @@ TEST_F(RestIncrementalAppendScanTest, PlanFilesEmptyWhenNoCurrentSnapshot) { TEST_F(RestIncrementalAppendScanTest, PlanFilesWithExplicitToSnapshotId) { constexpr int64_t kToSnapshotId = 1000L; constexpr std::string_view kResponseBody = R"({"status":"completed"})"; - EXPECT_CALL(*mock_client_, Post(_, JsonBodyHas("end-snapshot-id", kToSnapshotId), _, _, _)) + EXPECT_CALL(*mock_client_, + Post(_, JsonBodyHas("end-snapshot-id", kToSnapshotId), _, _, _)) .WillOnce(Return(HttpResponse::MakeForTesting(200, std::string(kResponseBody)))); ICEBERG_UNWRAP_OR_FAIL( @@ -955,13 +956,14 @@ TEST_F(RestIncrementalAppendScanTest, PlanFilesWithFromSnapshotIdExclusive) { constexpr int64_t kCurrentSnapshotId = 1000L; constexpr std::string_view kResponseBody = R"({"status":"completed"})"; EXPECT_CALL(*mock_client_, - Post(_, testing::AllOf(JsonBodyHas("start-snapshot-id", kFromSnapshotId), - JsonBodyHas("end-snapshot-id", kCurrentSnapshotId)), + Post(_, + testing::AllOf(JsonBodyHas("start-snapshot-id", kFromSnapshotId), + JsonBodyHas("end-snapshot-id", kCurrentSnapshotId)), _, _, _)) .WillOnce(Return(HttpResponse::MakeForTesting(200, std::string(kResponseBody)))); - ICEBERG_UNWRAP_OR_FAIL( - auto scan, MakeIncrementalScan(MakeContext(), kFromSnapshotId, /*inclusive=*/false)); + ICEBERG_UNWRAP_OR_FAIL(auto scan, MakeIncrementalScan(MakeContext(), kFromSnapshotId, + /*inclusive=*/false)); ICEBERG_UNWRAP_OR_FAIL(auto tasks, scan->PlanFiles()); EXPECT_TRUE(tasks.empty()); } @@ -975,8 +977,9 @@ TEST_F(RestIncrementalAppendScanTest, PlanFilesWithFromSnapshotIdInclusiveNoPare constexpr int64_t kFromSnapshotId = 1000L; constexpr std::string_view kResponseBody = R"({"status":"completed"})"; EXPECT_CALL(*mock_client_, - Post(_, testing::AllOf(JsonBodyLacks("start-snapshot-id"), - JsonBodyHas("end-snapshot-id", kFromSnapshotId)), + Post(_, + testing::AllOf(JsonBodyLacks("start-snapshot-id"), + JsonBodyHas("end-snapshot-id", kFromSnapshotId)), _, _, _)) .WillOnce(Return(HttpResponse::MakeForTesting(200, std::string(kResponseBody)))); @@ -1054,8 +1057,9 @@ TEST_F(RestIncrementalAppendScanTest, PlanFilesWithFromSnapshotIdInclusiveWithPa constexpr std::string_view kResponseBody = R"({"status":"completed"})"; EXPECT_CALL(*mock_client_, - Post(_, testing::AllOf(JsonBodyHas("start-snapshot-id", kParentSnapshotId), - JsonBodyHas("end-snapshot-id", kCurrentSnapshotId)), + Post(_, + testing::AllOf(JsonBodyHas("start-snapshot-id", kParentSnapshotId), + JsonBodyHas("end-snapshot-id", kCurrentSnapshotId)), _, _, _)) .WillOnce(Return(HttpResponse::MakeForTesting(200, std::string(kResponseBody))));