feat(rest): add scan plan endpoint support to REST catalog client by gsandeep1241 · Pull Request #783 · apache/iceberg-cpp

gsandeep1241 · 2026-06-26T06:37:31Z

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

github-actions · 2026-08-29T00:41:56Z

This pull request has been marked as stale due to 30 days of inactivity. It will be closed in 1 week if no further activity occurs. If you think that’s incorrect or this pull request requires a review, please simply write any comment. If closed, you can revive the PR at any time and @mention a reviewer or discuss it on the dev@iceberg.apache.org list. Thank you for your contributions.

github-actions · 2026-09-05T00:45:35Z

This pull request has been closed due to lack of activity. This is not a judgement on the merit of the PR in any way. It is just a way of keeping the PR queue manageable. If you think that is incorrect, or the pull request requires review, you can revive the PR at any time.

wgtmac · 2026-09-07T09:45:25Z

Hey @gsandeep1241, do you want to revive this?

gsandeep1241 · 2026-09-08T04:58:28Z

@wgtmac Thanks for re-opening it. Apologies for the long delay, I'll get back on it this week - next set of changes should be out for review in the next couple of days!

gsandeep1241 · 2026-09-13T21:31:08Z

+      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);


Do we want to introduce a new error type? Or use InvalidArgument instead?

gsandeep1241 · 2026-09-13T23:25:11Z

Hey @gsandeep1241, do you want to revive this?

Thanks @wgtmac for reviving this! This is now ready for review. Please take a look when you can :)

wgtmac · 2026-09-17T07:25:39Z

+      RestScanContext rest_context);
+
+  /// \brief Plans files via the REST scan planning endpoints.
+  Result<std::vector<std::shared_ptr<FileScanTask>>> PlanFiles() const override;


After #873, callers can use PlanFilesStream(). This class only overrides PlanFiles(), so the stream path still reads manifests locally and never calls /plan. This bypasses server-side planning.

wgtmac · 2026-09-17T07:25:39Z

+  /// 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<FileIO>& effective_io() const;


The scan is returned as std::unique_ptr<DataTableScan>, but DataTableScan::io() is not virtual and still returns the table IO. A normal caller cannot reach effective_io() without a downcast, so the vended credentials are not used by the normal read path.

Done, thanks!

wgtmac · 2026-09-17T07:25:39Z

+  auto start = std::chrono::steady_clock::now();
+
+  for (int retry = 0; retry <= kMaxRetries; ++retry) {
+    ICEBERG_ASSIGN_OR_RAISE(


After a plan-id exists, any GET, JSON parse, or response validation error here returns without calling CancelPlanning(). The server may keep the plan resources.

wgtmac · 2026-09-17T07:25:39Z

+
+Status RestTableScan::ApplyStorageCredentials(
+    const std::vector<StorageCredential>& credentials) const {
+  if (credentials.empty()) return {};


An empty credential list leaves scan_io_ unchanged. If this scan is planned twice and only the first response has credentials, the second result still uses the old credentials.

wgtmac · 2026-09-17T07:25:40Z

+
+  /// \brief Returns a RestTableScanBuilder that will delegate PlanFiles() to the
+  /// REST catalog server.
+  Result<std::unique_ptr<DataTableScanBuilder>> NewScan() const override;


This only overrides NewScan(). NewIncrementalAppendScan() and NewIncrementalChangelogScan() still use the base table path and plan manifests locally, even when scan-planning-mode=server.

Override for NewIncrementalAppendScan added. However, ChangeLogScanTask is not defined in the spec. Can we have server side support for these? What am I missing?

wgtmac · 2026-09-17T07:25:40Z

+
+  EXPECT_CALL(*mock_client_, Post(_, _, _, _, _))
+      .WillOnce(Return(HttpResponse::MakeForTesting(200, std::string(kSubmittedBody))));
+  EXPECT_CALL(*mock_client_, Get(_, _, _, _, _))


There is no case where a plan-id is returned and the poll GET, JSON parsing, or response validation fails. Those errors currently skip cancellation.

Done. Added tests.

wgtmac · 2026-09-17T07:25:40Z

+                               MakeContext(std::nullopt));
+  builder.UseSnapshot(kSnapshotId);
+  ICEBERG_UNWRAP_OR_FAIL(auto scan, builder.Build());
+  EXPECT_TRUE(scan->context().use_snapshot_schema);


This checks the builder context, not the JSON sent to the server. None of the POST expectations inspect the body, so request fields can be missing and these tests still pass.

wgtmac · 2026-09-17T07:25:40Z

+  ICEBERG_UNWRAP_OR_FAIL(auto tasks, scan->PlanFiles());
+  EXPECT_TRUE(tasks.empty());
+
+  auto* rest_scan = dynamic_cast<RestTableScan*>(scan.get());


This downcast hides the public API problem. Real callers hold a DataTableScan and use io(), which still returns the original table IO.

wgtmac · 2026-09-17T07:25:40Z

+// --------------------------------------------------------------------------
+// No storage credentials: effective_io() falls back to the table's FileIO.
+// --------------------------------------------------------------------------
+TEST_F(RestTableScanTest, NoStorageCredentialsEffectiveIoFallsBackToTableIO) {


This uses a fresh scan. It does not catch stale credentials when the same scan is planned twice, first with credentials and then without them.

Done, added test.

wgtmac · 2026-09-17T07:25:40Z

+// --------------------------------------------------------------------------
+// RestTable::NewScan returns a RestTableScanBuilder (not a plain builder).
+// --------------------------------------------------------------------------
+TEST_F(RestTableScanTest, RestTableNewScanReturnsRestTableScanBuilder) {


This builds RestTable directly. It does not cover RestCatalog::LoadTable() choosing the scan type from client and table scan-planning-mode, or the missing-endpoint error.

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

- 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

Upstream changed Table and DataTableScanBuilder constructors to require full_name/table_name and MetricsReporter parameters. Updated RestTable, RestTableScanBuilder, and their callers accordingly.

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.

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.

…port 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.

…lesStream, 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<shared_ptr<FileIO>>) 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.

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.

… and virtual io() - 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

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.

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.

…ial 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

…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

…ests - 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<RestFileScanTaskStream> deduction failure by replacing brace-init {} with std::vector<std::string>{} 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.

wgtmac reviewed Jul 2, 2026

View reviewed changes

Comment thread src/iceberg/catalog/rest/rest_catalog.cc Outdated

wgtmac reviewed Jul 2, 2026

View reviewed changes

Comment thread src/iceberg/catalog/rest/rest_table_scan.cc Outdated

wgtmac reviewed Jul 2, 2026

View reviewed changes

Comment thread src/iceberg/catalog/rest/rest_table_scan.cc

wgtmac reviewed Jul 2, 2026

View reviewed changes

Comment thread src/iceberg/catalog/rest/rest_table_scan.cc Outdated

github-actions Bot added the stale label Aug 29, 2026

github-actions Bot closed this Sep 5, 2026

wgtmac reopened this Sep 7, 2026

github-actions Bot removed the stale label Sep 8, 2026

gsandeep1241 force-pushed the sandeepg-scan-plan-endpoint-for-rest-catalog-client-2-impl branch from fb8da85 to d8a7d6c Compare September 13, 2026 19:53

gsandeep1241 commented Sep 13, 2026

View reviewed changes

gsandeep1241 requested a review from wgtmac September 13, 2026 21:33

gsandeep1241 marked this pull request as ready for review September 13, 2026 21:38

gsandeep1241 force-pushed the sandeepg-scan-plan-endpoint-for-rest-catalog-client-2-impl branch 2 times, most recently from 2bab861 to bcc9da4 Compare September 13, 2026 22:48

wgtmac reviewed Sep 17, 2026

View reviewed changes

Sandeep Gottimukkala and others added 8 commits September 17, 2026 23:32

Fix API compatibility after rebase on main

5e867b2

Upstream changed Table and DataTableScanBuilder constructors to require full_name/table_name and MetricsReporter parameters. Updated RestTable, RestTableScanBuilder, and their callers accordingly.

style: apply clang-format to fix CI pre-commit failures

92ba0d7

gsandeep1241 force-pushed the sandeepg-scan-plan-endpoint-for-rest-catalog-client-2-impl branch from a9cbb7d to 5de0344 Compare September 29, 2026 03:22

Sandeep Gottimukkala added 7 commits September 28, 2026 20:55

style: apply clang-format to rest catalog and scan files

23d149a

gsandeep1241 requested a review from wgtmac September 29, 2026 06:33

style: use raw string literal in rest_catalog_unit_test

9dbd5c5

Conversation

gsandeep1241 commented Jun 26, 2026

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

github-actions Bot commented Aug 29, 2026

Uh oh!

github-actions Bot commented Sep 5, 2026

Uh oh!

wgtmac commented Sep 7, 2026

Uh oh!

gsandeep1241 commented Sep 8, 2026

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gsandeep1241 commented Sep 13, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gsandeep1241 Sep 29, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

This branch has not been deployed

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

gsandeep1241 Sep 29, 2026 •

edited

Loading