feat(rest): add scan plan endpoint support to REST catalog client - #783
gsandeep1241 wants to merge 7 commits into
Conversation
|
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. |
|
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. |
|
Hey @gsandeep1241, do you want to revive this? |
|
@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! |
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.
fb8da85 to
d8a7d6c
Compare
| 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); |
There was a problem hiding this comment.
Do we want to introduce a new error type? Or use InvalidArgument instead?
06fb014 to
2bab861
Compare
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.
2bab861 to
bcc9da4
Compare
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.
Thanks @wgtmac for reviving this! This is now ready for review. Please take a look when you can :) |
| RestScanContext rest_context); | ||
|
|
||
| /// \brief Plans files via the REST scan planning endpoints. | ||
| Result<std::vector<std::shared_ptr<FileScanTask>>> PlanFiles() const override; |
There was a problem hiding this comment.
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.
| /// 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; |
There was a problem hiding this comment.
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.
| auto start = std::chrono::steady_clock::now(); | ||
|
|
||
| for (int retry = 0; retry <= kMaxRetries; ++retry) { | ||
| ICEBERG_ASSIGN_OR_RAISE( |
There was a problem hiding this comment.
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.
|
|
||
| Status RestTableScan::ApplyStorageCredentials( | ||
| const std::vector<StorageCredential>& credentials) const { | ||
| if (credentials.empty()) return {}; |
There was a problem hiding this comment.
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.
|
|
||
| /// \brief Returns a RestTableScanBuilder that will delegate PlanFiles() to the | ||
| /// REST catalog server. | ||
| Result<std::unique_ptr<DataTableScanBuilder>> NewScan() const override; |
There was a problem hiding this comment.
This only overrides NewScan(). NewIncrementalAppendScan() and NewIncrementalChangelogScan() still use the base table path and plan manifests locally, even when scan-planning-mode=server.
|
|
||
| EXPECT_CALL(*mock_client_, Post(_, _, _, _, _)) | ||
| .WillOnce(Return(HttpResponse::MakeForTesting(200, std::string(kSubmittedBody)))); | ||
| EXPECT_CALL(*mock_client_, Get(_, _, _, _, _)) |
There was a problem hiding this comment.
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.
| MakeContext(std::nullopt)); | ||
| builder.UseSnapshot(kSnapshotId); | ||
| ICEBERG_UNWRAP_OR_FAIL(auto scan, builder.Build()); | ||
| EXPECT_TRUE(scan->context().use_snapshot_schema); |
There was a problem hiding this comment.
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.
| ICEBERG_UNWRAP_OR_FAIL(auto tasks, scan->PlanFiles()); | ||
| EXPECT_TRUE(tasks.empty()); | ||
|
|
||
| auto* rest_scan = dynamic_cast<RestTableScan*>(scan.get()); |
There was a problem hiding this comment.
This downcast hides the public API problem. Real callers hold a DataTableScan and use io(), which still returns the original table IO.
| // -------------------------------------------------------------------------- | ||
| // No storage credentials: effective_io() falls back to the table's FileIO. | ||
| // -------------------------------------------------------------------------- | ||
| TEST_F(RestTableScanTest, NoStorageCredentialsEffectiveIoFallsBackToTableIO) { |
There was a problem hiding this comment.
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.
| // -------------------------------------------------------------------------- | ||
| // RestTable::NewScan returns a RestTableScanBuilder (not a plain builder). | ||
| // -------------------------------------------------------------------------- | ||
| TEST_F(RestTableScanTest, RestTableNewScanReturnsRestTableScanBuilder) { |
There was a problem hiding this comment.
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.
2 participants
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.