Skip to content

Commit 67a5602

Browse files
Sandeep GottimukkalaSandeep Gottimukkala
Sandeep Gottimukkala
authored and
Sandeep Gottimukkala
committed
refactor(rest): clean up scan plan API naming and structure
- Rename ScanPlanErrorHandler to PlanErrorHandler; add PlanTaskErrorHandler for FetchScanTasks with correct 404 handling per spec - Merge ScanPlan overloads into Plan(ident, optional<string> plan_id) - Rename ResourcePaths::ScanTask to FetchScanTasks - Rename statsFields -> stats_fields, min_rows_required -> min_rows_requested - Encode plan_id in URL path - Move BaseScanTaskResponseFromJson into anonymous namespace - Fix kMinRowsRequired wire value to "min-rows-requested"
1 parent 86714b9 commit 67a5602

8 files changed

Lines changed: 42 additions & 47 deletions

File tree

‎src/iceberg/catalog/rest/error_handlers.cc‎

Lines changed: 7 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -186,12 +186,12 @@ Status ViewCommitErrorHandler::Accept(const ErrorResponse& error) const {
186186
return DefaultErrorHandler::Accept(error);
187187
}
188188

189-
const std::shared_ptr<ScanPlanErrorHandler>& ScanPlanErrorHandler::Instance() {
190-
static const std::shared_ptr<ScanPlanErrorHandler> instance{new ScanPlanErrorHandler()};
189+
const std::shared_ptr<PlanErrorHandler>& PlanErrorHandler::Instance() {
190+
static const std::shared_ptr<PlanErrorHandler> instance{new PlanErrorHandler()};
191191
return instance;
192192
}
193193

194-
Status ScanPlanErrorHandler::Accept(const ErrorResponse& error) const {
194+
Status PlanErrorHandler::Accept(const ErrorResponse& error) const {
195195
switch (error.code) {
196196
case 404:
197197
if (error.type == kNoSuchNamespaceException) {
@@ -203,9 +203,6 @@ Status ScanPlanErrorHandler::Accept(const ErrorResponse& error) const {
203203
if (error.type == kNoSuchPlanIdException) {
204204
return NoSuchPlanId(error.message);
205205
}
206-
if (error.type == kNoSuchPlanTaskException) {
207-
return NoSuchPlanTask(error.message);
208-
}
209206
return NotFound(error.message);
210207
case 406:
211208
return NotSupported(error.message);
@@ -228,7 +225,10 @@ Status PlanTaskErrorHandler::Accept(const ErrorResponse& error) const {
228225
if (error.type == kNoSuchTableException) {
229226
return NoSuchTable(error.message);
230227
}
231-
return NoSuchPlanTask(error.message);
228+
if (error.type == kNoSuchPlanTaskException) {
229+
return NoSuchPlanTask(error.message);
230+
}
231+
return NotFound(error.message);
232232
}
233233

234234
return DefaultErrorHandler::Accept(error);

‎src/iceberg/catalog/rest/error_handlers.h‎

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -127,15 +127,15 @@ class ICEBERG_REST_EXPORT ViewCommitErrorHandler final : public DefaultErrorHand
127127
constexpr ViewCommitErrorHandler() = default;
128128
};
129129

130-
/// \brief Scan plan operation error handler.
131-
class ICEBERG_REST_EXPORT ScanPlanErrorHandler final : public DefaultErrorHandler {
130+
/// \brief Plan operation error handler.
131+
class ICEBERG_REST_EXPORT PlanErrorHandler final : public DefaultErrorHandler {
132132
public:
133-
static const std::shared_ptr<ScanPlanErrorHandler>& Instance();
133+
static const std::shared_ptr<PlanErrorHandler>& Instance();
134134

135135
Status Accept(const ErrorResponse& error) const override;
136136

137137
private:
138-
constexpr ScanPlanErrorHandler() = default;
138+
constexpr PlanErrorHandler() = default;
139139
};
140140

141141
/// \brief Fetch scan tasks operation error handler.

‎src/iceberg/catalog/rest/json_serde.cc‎

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -574,11 +574,11 @@ Result<nlohmann::json> ToJson(const PlanTableScanRequest& request) {
574574
if (request.end_snapshot_id.has_value()) {
575575
json[kEndSnapshotId] = request.end_snapshot_id.value();
576576
}
577-
if (!request.statsFields.empty()) {
578-
json[kStatsFields] = request.statsFields;
577+
if (!request.stats_fields.empty()) {
578+
json[kStatsFields] = request.stats_fields;
579579
}
580-
if (request.min_rows_required.has_value()) {
581-
json[kMinRowsRequested] = request.min_rows_required.value();
580+
if (request.min_rows_requested.has_value()) {
581+
json[kMinRowsRequested] = request.min_rows_requested.value();
582582
}
583583
return json;
584584
}

‎src/iceberg/catalog/rest/resource_paths.cc‎

Lines changed: 9 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -20,6 +20,7 @@
2020
#include "iceberg/catalog/rest/resource_paths.h"
2121

2222
#include <format>
23+
#include <optional>
2324

2425
#include "iceberg/catalog/rest/rest_util.h"
2526
#include "iceberg/table_identifier.h"
@@ -102,23 +103,20 @@ Result<std::string> ResourcePaths::CommitTransaction() const {
102103
return std::format("{}/v1/{}transactions/commit", base_uri_, prefix_);
103104
}
104105

105-
Result<std::string> ResourcePaths::ScanPlan(const TableIdentifier& ident) const {
106+
Result<std::string> ResourcePaths::Plan(const TableIdentifier& ident,
107+
std::optional<std::string> plan_id) const {
106108
ICEBERG_ASSIGN_OR_RAISE(std::string encoded_namespace, EncodeNamespace(ident.ns));
107109
ICEBERG_ASSIGN_OR_RAISE(std::string encoded_table_name, EncodeString(ident.name));
110+
if (plan_id.has_value()) {
111+
ICEBERG_ASSIGN_OR_RAISE(std::string encoded_plan_id, EncodeString(plan_id.value()));
112+
return std::format("{}/v1/{}namespaces/{}/tables/{}/plan/{}", base_uri_, prefix_,
113+
encoded_namespace, encoded_table_name, encoded_plan_id);
114+
}
108115
return std::format("{}/v1/{}namespaces/{}/tables/{}/plan", base_uri_, prefix_,
109116
encoded_namespace, encoded_table_name);
110117
}
111118

112-
Result<std::string> ResourcePaths::ScanPlan(const TableIdentifier& ident,
113-
const std::string& plan_id) const {
114-
ICEBERG_ASSIGN_OR_RAISE(std::string encoded_namespace, EncodeNamespace(ident.ns));
115-
ICEBERG_ASSIGN_OR_RAISE(std::string encoded_table_name, EncodeString(ident.name));
116-
ICEBERG_ASSIGN_OR_RAISE(std::string encoded_plan_id, EncodeString(plan_id));
117-
return std::format("{}/v1/{}namespaces/{}/tables/{}/plan/{}", base_uri_, prefix_,
118-
encoded_namespace, encoded_table_name, encoded_plan_id);
119-
}
120-
121-
Result<std::string> ResourcePaths::ScanTask(const TableIdentifier& ident) const {
119+
Result<std::string> ResourcePaths::FetchScanTasks(const TableIdentifier& ident) const {
122120
ICEBERG_ASSIGN_OR_RAISE(std::string encoded_namespace, EncodeNamespace(ident.ns));
123121
ICEBERG_ASSIGN_OR_RAISE(std::string encoded_table_name, EncodeString(ident.name));
124122
return std::format("{}/v1/{}namespaces/{}/tables/{}/tasks", base_uri_, prefix_,

‎src/iceberg/catalog/rest/resource_paths.h‎

Lines changed: 5 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -20,6 +20,7 @@
2020
#pragma once
2121

2222
#include <memory>
23+
#include <optional>
2324
#include <string>
2425

2526
#include "iceberg/catalog/rest/iceberg_rest_export.h"
@@ -82,17 +83,13 @@ class ICEBERG_REST_EXPORT ResourcePaths {
8283
Result<std::string> CommitTransaction() const;
8384

8485
/// \brief Get the /v1/{prefix}/namespaces/{namespace}/tables/{table}/plan endpoint
85-
/// path.
86-
Result<std::string> ScanPlan(const TableIdentifier& ident) const;
87-
88-
/// \brief Get the /v1/{prefix}/namespaces/{namespace}/tables/{table}/plan/{plan_id}
89-
/// endpoint path.
90-
Result<std::string> ScanPlan(const TableIdentifier& ident,
91-
const std::string& plan_id) const;
86+
/// path, or /plan/{plan_id} if plan_id is provided.
87+
Result<std::string> Plan(const TableIdentifier& ident,
88+
std::optional<std::string> plan_id = std::nullopt) const;
9289

9390
/// \brief Get the /v1/{prefix}/namespaces/{namespace}/tables/{table}/tasks endpoint
9491
/// path.
95-
Result<std::string> ScanTask(const TableIdentifier& ident) const;
92+
Result<std::string> FetchScanTasks(const TableIdentifier& ident) const;
9693

9794
private:
9895
ResourcePaths(std::string base_uri, const std::string& prefix);

‎src/iceberg/catalog/rest/rest_catalog.cc‎

Lines changed: 8 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -501,7 +501,7 @@ Result<std::shared_ptr<Table>> RestCatalog::RegisterTable(
501501
Result<PlanTableScanResponse> RestCatalog::PlanTableScan(
502502
const Table& table, const internal::TableScanContext& context) {
503503
ICEBERG_ENDPOINT_CHECK(supported_endpoints_, Endpoint::PlanTableScan());
504-
ICEBERG_ASSIGN_OR_RAISE(auto path, paths_->ScanPlan(table.name()));
504+
ICEBERG_ASSIGN_OR_RAISE(auto path, paths_->Plan(table.name()));
505505
ICEBERG_ASSIGN_OR_RAISE(auto schema_ptr, table.schema());
506506
ICEBERG_ASSIGN_OR_RAISE(auto specs_ref, table.specs());
507507

@@ -519,15 +519,15 @@ Result<PlanTableScanResponse> RestCatalog::PlanTableScan(
519519
request.use_snapshot_schema = true;
520520
}
521521
if (context.min_rows_requested.has_value()) {
522-
request.min_rows_required = context.min_rows_requested.value();
522+
request.min_rows_requested = context.min_rows_requested.value();
523523
}
524524

525525
ICEBERG_RETURN_UNEXPECTED(request.Validate());
526526
ICEBERG_ASSIGN_OR_RAISE(auto request_json, ToJson(request));
527527
ICEBERG_ASSIGN_OR_RAISE(auto json_request, ToJsonString(request_json));
528528
ICEBERG_ASSIGN_OR_RAISE(
529529
const auto response,
530-
client_->Post(path, json_request, /*headers=*/{}, *ScanPlanErrorHandler::Instance(),
530+
client_->Post(path, json_request, /*headers=*/{}, *PlanErrorHandler::Instance(),
531531
*catalog_session_));
532532

533533
ICEBERG_ASSIGN_OR_RAISE(auto json, FromJsonString(response.body()));
@@ -537,33 +537,33 @@ Result<PlanTableScanResponse> RestCatalog::PlanTableScan(
537537
Result<FetchPlanningResultResponse> RestCatalog::FetchPlanningResult(
538538
const Table& table, const std::string& plan_id) {
539539
ICEBERG_ENDPOINT_CHECK(supported_endpoints_, Endpoint::FetchPlanningResult());
540-
ICEBERG_ASSIGN_OR_RAISE(auto path, paths_->ScanPlan(table.name(), plan_id));
540+
ICEBERG_ASSIGN_OR_RAISE(auto path, paths_->Plan(table.name(), plan_id));
541541
ICEBERG_ASSIGN_OR_RAISE(auto schema_ptr, table.schema());
542542
ICEBERG_ASSIGN_OR_RAISE(auto specs_ref, table.specs());
543543

544544
ICEBERG_ASSIGN_OR_RAISE(
545545
const auto response,
546-
client_->Get(path, /*params=*/{}, /*headers=*/{}, *ScanPlanErrorHandler::Instance(),
546+
client_->Get(path, /*params=*/{}, /*headers=*/{}, *PlanErrorHandler::Instance(),
547547
*catalog_session_));
548548
ICEBERG_ASSIGN_OR_RAISE(auto json, FromJsonString(response.body()));
549549
return FetchPlanningResultResponseFromJson(json, specs_ref.get(), *schema_ptr);
550550
}
551551

552552
Status RestCatalog::CancelPlanning(const Table& table, const std::string& plan_id) {
553553
ICEBERG_ENDPOINT_CHECK(supported_endpoints_, Endpoint::CancelPlanning());
554-
ICEBERG_ASSIGN_OR_RAISE(auto path, paths_->ScanPlan(table.name(), plan_id));
554+
ICEBERG_ASSIGN_OR_RAISE(auto path, paths_->Plan(table.name(), plan_id));
555555

556556
ICEBERG_ASSIGN_OR_RAISE(
557557
const auto response,
558558
client_->Delete(path, /*params=*/{}, /*headers=*/{},
559-
*ScanPlanErrorHandler::Instance(), *catalog_session_));
559+
*PlanErrorHandler::Instance(), *catalog_session_));
560560
return {};
561561
}
562562

563563
Result<FetchScanTasksResponse> RestCatalog::FetchScanTasks(const Table& table,
564564
const std::string& plan_task) {
565565
ICEBERG_ENDPOINT_CHECK(supported_endpoints_, Endpoint::FetchScanTasks());
566-
ICEBERG_ASSIGN_OR_RAISE(auto path, paths_->ScanTask(table.name()));
566+
ICEBERG_ASSIGN_OR_RAISE(auto path, paths_->FetchScanTasks(table.name()));
567567
ICEBERG_ASSIGN_OR_RAISE(auto schema_ptr, table.schema());
568568
ICEBERG_ASSIGN_OR_RAISE(auto specs_ref, table.specs());
569569

‎src/iceberg/catalog/rest/types.cc‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -123,8 +123,8 @@ bool PlanTableScanRequest::operator==(const PlanTableScanRequest& other) const {
123123
filter == other.filter && case_sensitive == other.case_sensitive &&
124124
use_snapshot_schema == other.use_snapshot_schema &&
125125
start_snapshot_id == other.start_snapshot_id &&
126-
end_snapshot_id == other.end_snapshot_id && statsFields == other.statsFields &&
127-
min_rows_required == other.min_rows_required;
126+
end_snapshot_id == other.end_snapshot_id && stats_fields == other.stats_fields &&
127+
min_rows_requested == other.min_rows_requested;
128128
}
129129

130130
bool BaseScanTaskResponse::operator==(const BaseScanTaskResponse& other) const {
@@ -213,7 +213,7 @@ Status PlanTableScanRequest::Validate() const {
213213
"Invalid incremental scan: startSnapshotId and endSnapshotId is required");
214214
}
215215
}
216-
if (min_rows_required.has_value() && min_rows_required.value() < 0) {
216+
if (min_rows_requested.has_value() && min_rows_requested.value() < 0) {
217217
return ValidationFailed("Invalid scan: minRowsRequested is negative");
218218
}
219219
return {};

‎src/iceberg/catalog/rest/types.h‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -307,8 +307,8 @@ struct ICEBERG_REST_EXPORT PlanTableScanRequest {
307307
bool use_snapshot_schema = false;
308308
std::optional<int64_t> start_snapshot_id;
309309
std::optional<int64_t> end_snapshot_id;
310-
std::vector<std::string> statsFields;
311-
std::optional<int64_t> min_rows_required;
310+
std::vector<std::string> stats_fields;
311+
std::optional<int64_t> min_rows_requested;
312312

313313
Status Validate() const;
314314

0 commit comments

Comments
 (0)