Skip to content

Commit 31b2182

Browse files
Sandeep GottimukkalaSandeep Gottimukkala
authored andcommitted
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<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.
1 parent c0d99f9 commit 31b2182

8 files changed

Lines changed: 260 additions & 11 deletions

File tree

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

Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -28,6 +28,7 @@
2828

2929
#include <nlohmann/json.hpp>
3030

31+
#include "iceberg/catalog/rest/auth/auth_manager_internal.h"
3132
#include "iceberg/catalog/rest/auth/auth_managers.h"
3233
#include "iceberg/catalog/rest/auth/auth_session.h"
3334
#include "iceberg/catalog/rest/catalog_properties.h"
@@ -455,6 +456,23 @@ Result<std::shared_ptr<RestCatalog>> RestCatalog::Make(
455456
snapshot_mode, std::move(default_context), std::move(reporter), metrics_executor));
456457
}
457458

459+
Result<std::shared_ptr<RestCatalog>> RestCatalog::MakeForTesting(
460+
RestCatalogProperties config, std::shared_ptr<FileIO> file_io,
461+
std::shared_ptr<HttpClient> client, std::shared_ptr<ResourcePaths> paths,
462+
std::unordered_set<Endpoint> endpoints) {
463+
std::string catalog_name = config.Get(RestCatalogProperties::kName);
464+
ICEBERG_ASSIGN_OR_RAISE(
465+
auto auth_manager,
466+
auth::MakeNoopAuthManager(catalog_name, config.configs()));
467+
auto session = auth::AuthSession::MakeDefault({});
468+
ICEBERG_ASSIGN_OR_RAISE(auto snapshot_mode, config.SnapshotLoadingMode());
469+
return std::shared_ptr<RestCatalog>(
470+
new RestCatalog(std::move(config), std::move(file_io), std::move(client),
471+
std::move(paths), std::move(endpoints), std::move(auth_manager),
472+
std::move(session), snapshot_mode, SessionContext::Empty(),
473+
/*reporter=*/nullptr, /*metrics_executor=*/nullptr));
474+
}
475+
458476
RestCatalog::RestCatalog(RestCatalogProperties config, std::shared_ptr<FileIO> file_io,
459477
std::shared_ptr<HttpClient> client,
460478
std::shared_ptr<ResourcePaths> paths,

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

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -58,6 +58,13 @@ class ICEBERG_REST_EXPORT RestCatalog final
5858
static Result<std::shared_ptr<RestCatalog>> Make(const RestCatalogProperties& config,
5959
Executor* metrics_executor = nullptr);
6060

61+
/// \brief Test-only factory that constructs a RestCatalog with pre-built dependencies,
62+
/// bypassing the FetchServerConfig HTTP exchange performed by Make().
63+
static Result<std::shared_ptr<RestCatalog>> MakeForTesting(
64+
RestCatalogProperties config, std::shared_ptr<FileIO> file_io,
65+
std::shared_ptr<HttpClient> client, std::shared_ptr<ResourcePaths> paths,
66+
std::unordered_set<Endpoint> endpoints);
67+
6168
std::string_view name() const override;
6269

6370
Result<std::shared_ptr<Catalog>> AsCatalog() override;

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

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -31,6 +31,7 @@
3131
#include "iceberg/catalog/rest/resource_paths.h"
3232
#include "iceberg/catalog/rest/rest_file_io.h"
3333
#include "iceberg/catalog/rest/types.h"
34+
#include "iceberg/constants.h"
3435
#include "iceberg/json_serde_internal.h"
3536
#include "iceberg/partition_spec.h"
3637
#include "iceberg/result.h"
@@ -297,7 +298,8 @@ Result<FileScanTaskStreamPtr> ExecuteScanPlanStream(const RestScanContext& ctx,
297298
ICEBERG_ASSIGN_OR_RAISE(
298299
auto tasks, FetchPlanningResult(ctx, schema, mutable_plan_id, specs, *scan_io_slot));
299300
return std::make_unique<RestFileScanTaskStream>(ctx, schema_ptr, mutable_plan_id,
300-
std::move(tasks), {}, specs, scan_io_slot);
301+
std::move(tasks), std::vector<std::string>{},
302+
specs, scan_io_slot);
301303
}
302304

303305
if (result.plan_status == PlanStatus::kFailed) {
@@ -496,6 +498,7 @@ Result<std::vector<std::shared_ptr<FileScanTask>>> RestIncrementalAppendScan::Pl
496498
if (context_.to_snapshot_id.has_value()) {
497499
request.end_snapshot_id = context_.to_snapshot_id;
498500
} else {
501+
if (metadata_->current_snapshot_id == kInvalidSnapshotId) return {};
499502
ICEBERG_ASSIGN_OR_RAISE(auto snapshot, metadata_->Snapshot());
500503
if (!snapshot) return {};
501504
request.end_snapshot_id = snapshot->snapshot_id;

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

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -298,10 +298,10 @@ Status PlanTableScanResponse::Validate() const {
298298
"Invalid response: tasks can only be defined when status is 'completed'");
299299
}
300300
if (!plan_id.empty() && plan_status != PlanStatus::kSubmitted &&
301-
plan_status != PlanStatus::kCompleted) {
301+
plan_status != PlanStatus::kCompleted && plan_status != PlanStatus::kFailed) {
302302
return ValidationFailed(
303-
"Invalid response: plan id can only be defined when status is 'submitted' or "
304-
"'completed'");
303+
"Invalid response: plan id can only be defined when status is 'submitted', "
304+
"'completed', or 'failed'");
305305
}
306306
if (!HasNonEmptyFileScanTasks(*this) && !delete_files.empty()) {
307307
return ValidationFailed(

‎src/iceberg/test/CMakeLists.txt‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -321,6 +321,7 @@ if(ICEBERG_BUILD_REST)
321321
catalog_properties_test.cc
322322
error_handlers_test.cc
323323
endpoint_test.cc
324+
rest_catalog_unit_test.cc
324325
rest_file_io_test.cc
325326
rest_json_serde_test.cc
326327
rest_metrics_reporter_test.cc
Lines changed: 214 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,214 @@
1+
/*
2+
* Licensed to the Apache Software Foundation (ASF) under one
3+
* or more contributor license agreements. See the NOTICE file
4+
* distributed with this work for additional information
5+
* regarding copyright ownership. The ASF licenses this file
6+
* to you under the Apache License, Version 2.0 (the
7+
* "License"); you may not use this file except in compliance
8+
* with the License. You may obtain a copy of the License at
9+
*
10+
* http://www.apache.org/licenses/LICENSE-2.0
11+
*
12+
* Unless required by applicable law or agreed to in writing,
13+
* software distributed under the License is distributed on an
14+
* "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
15+
* KIND, either express or implied. See the License for the
16+
* specific language governing permissions and limitations
17+
* under the License.
18+
*/
19+
20+
#include "iceberg/catalog/rest/rest_catalog.h"
21+
22+
#include <memory>
23+
#include <string>
24+
#include <unordered_map>
25+
#include <unordered_set>
26+
27+
#include <gmock/gmock.h>
28+
#include <gtest/gtest.h>
29+
30+
#include "iceberg/catalog/rest/catalog_properties.h"
31+
#include "iceberg/catalog/rest/endpoint.h"
32+
#include "iceberg/catalog/rest/error_handlers.h"
33+
#include "iceberg/catalog/rest/http_client.h"
34+
#include "iceberg/catalog/rest/resource_paths.h"
35+
#include "iceberg/catalog/rest/rest_table.h"
36+
#include "iceberg/file_io.h"
37+
#include "iceberg/table_identifier.h"
38+
#include "iceberg/test/matchers.h"
39+
40+
namespace iceberg::rest {
41+
42+
using ::testing::_;
43+
using ::testing::Return;
44+
45+
// --------------------------------------------------------------------------
46+
// Mock HTTP client (same pattern as rest_table_scan_test.cc)
47+
// --------------------------------------------------------------------------
48+
class MockHttpClient : public HttpClient {
49+
public:
50+
MockHttpClient() : HttpClient({}) {}
51+
52+
MOCK_METHOD(Result<HttpResponse>, Get,
53+
(const std::string& path,
54+
(const std::unordered_map<std::string, std::string>&)params,
55+
(const std::unordered_map<std::string, std::string>&)headers,
56+
const ErrorHandler& error_handler, auth::AuthSession& session),
57+
(override));
58+
59+
MOCK_METHOD(Result<HttpResponse>, Post,
60+
(const std::string& path, const std::string& body,
61+
(const std::unordered_map<std::string, std::string>&)headers,
62+
const ErrorHandler& error_handler, auth::AuthSession& session),
63+
(override));
64+
65+
MOCK_METHOD(Result<HttpResponse>, Delete,
66+
(const std::string& path,
67+
(const std::unordered_map<std::string, std::string>&)params,
68+
(const std::unordered_map<std::string, std::string>&)headers,
69+
const ErrorHandler& error_handler, auth::AuthSession& session),
70+
(override));
71+
};
72+
73+
// --------------------------------------------------------------------------
74+
// Minimal FileIO stub
75+
// --------------------------------------------------------------------------
76+
class NoOpFileIO : public FileIO {
77+
public:
78+
Result<std::string> ReadFile(const std::string&, std::optional<size_t>) override {
79+
return IOError("NoOpFileIO");
80+
}
81+
Status WriteFile(const std::string&, std::string_view) override { return {}; }
82+
Status DeleteFile(const std::string&) override { return {}; }
83+
};
84+
85+
// --------------------------------------------------------------------------
86+
// Helper JSON bodies for LoadTable GET responses
87+
// --------------------------------------------------------------------------
88+
89+
// Minimal table metadata blob (no snapshots, no partitions).
90+
constexpr std::string_view kBaseMetadataJson =
91+
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":{}})";
92+
93+
// LoadTable response where server config requests server-side scan planning.
94+
const std::string kLoadTableServerScanResponse =
95+
std::string("{\"config\":{\"scan-planning-mode\":\"server\"},") +
96+
std::string(kBaseMetadataJson) + "}";
97+
98+
// LoadTable response with no scan-planning-mode set (defaults to client).
99+
const std::string kLoadTableDefaultResponse =
100+
std::string("{") + std::string(kBaseMetadataJson) + "}";
101+
102+
// --------------------------------------------------------------------------
103+
// Test fixture: builds a RestCatalog via MakeForTesting so no HTTP calls
104+
// are made during catalog construction.
105+
// --------------------------------------------------------------------------
106+
class RestCatalogLoadTableTest : public ::testing::Test {
107+
protected:
108+
void SetUp() override {
109+
mock_client_ = std::make_shared<MockHttpClient>();
110+
file_io_ = std::make_shared<NoOpFileIO>();
111+
112+
ICEBERG_UNWRAP_OR_FAIL(paths_,
113+
ResourcePaths::Make("http://test-server", /*prefix=*/"",
114+
/*namespace_separator=*/"%1F"));
115+
116+
identifier_ = TableIdentifier{.ns = Namespace{{"default"}}, .name = "my_table"};
117+
118+
all_plan_endpoints_ = {Endpoint::LoadTable(), Endpoint::PlanTableScan(),
119+
Endpoint::FetchPlanningResult(), Endpoint::CancelPlanning(),
120+
Endpoint::FetchScanTasks()};
121+
122+
no_plan_endpoint_set_ = {Endpoint::LoadTable()};
123+
}
124+
125+
// Builds a RestCatalog with the given supported endpoints and an optional
126+
// client-side scan-planning-mode setting.
127+
Result<std::shared_ptr<RestCatalog>> MakeCatalog(
128+
const std::unordered_set<Endpoint>& endpoints,
129+
const std::string& client_scan_mode = "") {
130+
std::unordered_map<std::string, std::string> props{
131+
{"uri", "http://test-server"},
132+
};
133+
if (!client_scan_mode.empty()) {
134+
props["scan-planning-mode"] = client_scan_mode;
135+
}
136+
auto config = RestCatalogProperties::FromMap(props);
137+
return RestCatalog::MakeForTesting(std::move(config), file_io_, mock_client_, paths_,
138+
endpoints);
139+
}
140+
141+
std::shared_ptr<MockHttpClient> mock_client_;
142+
std::shared_ptr<FileIO> file_io_;
143+
std::shared_ptr<ResourcePaths> paths_;
144+
TableIdentifier identifier_;
145+
std::unordered_set<Endpoint> all_plan_endpoints_;
146+
std::unordered_set<Endpoint> no_plan_endpoint_set_;
147+
};
148+
149+
// --------------------------------------------------------------------------
150+
// Table config "scan-planning-mode":"server" → LoadTable returns a RestTable.
151+
// --------------------------------------------------------------------------
152+
TEST_F(RestCatalogLoadTableTest, TableConfigServerScanReturnsRestTable) {
153+
EXPECT_CALL(*mock_client_, Get(_, _, _, _, _))
154+
.WillOnce(
155+
Return(HttpResponse::MakeForTesting(200, kLoadTableServerScanResponse)));
156+
157+
ICEBERG_UNWRAP_OR_FAIL(auto catalog, MakeCatalog(all_plan_endpoints_));
158+
ICEBERG_UNWRAP_OR_FAIL(auto as_catalog, catalog->AsCatalog());
159+
ICEBERG_UNWRAP_OR_FAIL(auto table, as_catalog->LoadTable(identifier_));
160+
161+
EXPECT_NE(dynamic_cast<RestTable*>(table.get()), nullptr)
162+
<< "Expected RestTable when server config requests server-side scan planning";
163+
}
164+
165+
// --------------------------------------------------------------------------
166+
// Client config "scan-planning-mode":"server", table config absent →
167+
// LoadTable returns a RestTable.
168+
// --------------------------------------------------------------------------
169+
TEST_F(RestCatalogLoadTableTest, ClientConfigServerScanReturnsRestTable) {
170+
EXPECT_CALL(*mock_client_, Get(_, _, _, _, _))
171+
.WillOnce(Return(HttpResponse::MakeForTesting(200, kLoadTableDefaultResponse)));
172+
173+
ICEBERG_UNWRAP_OR_FAIL(auto catalog,
174+
MakeCatalog(all_plan_endpoints_, /*client_scan_mode=*/"server"));
175+
ICEBERG_UNWRAP_OR_FAIL(auto as_catalog, catalog->AsCatalog());
176+
ICEBERG_UNWRAP_OR_FAIL(auto table, as_catalog->LoadTable(identifier_));
177+
178+
EXPECT_NE(dynamic_cast<RestTable*>(table.get()), nullptr)
179+
<< "Expected RestTable when client config requests server-side scan planning";
180+
}
181+
182+
// --------------------------------------------------------------------------
183+
// "scan-planning-mode":"server" but PlanTableScan endpoint missing →
184+
// LoadTable returns NotSupported.
185+
// --------------------------------------------------------------------------
186+
TEST_F(RestCatalogLoadTableTest, ServerScanWithMissingEndpointReturnsNotSupported) {
187+
EXPECT_CALL(*mock_client_, Get(_, _, _, _, _))
188+
.WillOnce(
189+
Return(HttpResponse::MakeForTesting(200, kLoadTableServerScanResponse)));
190+
191+
ICEBERG_UNWRAP_OR_FAIL(auto catalog, MakeCatalog(no_plan_endpoint_set_));
192+
ICEBERG_UNWRAP_OR_FAIL(auto as_catalog, catalog->AsCatalog());
193+
auto result = as_catalog->LoadTable(identifier_);
194+
195+
EXPECT_THAT(result, IsError(ErrorKind::kNotSupported));
196+
}
197+
198+
// --------------------------------------------------------------------------
199+
// No scan-planning-mode set anywhere → LoadTable returns a plain Table
200+
// (not RestTable).
201+
// --------------------------------------------------------------------------
202+
TEST_F(RestCatalogLoadTableTest, DefaultScanModeReturnsPlainTable) {
203+
EXPECT_CALL(*mock_client_, Get(_, _, _, _, _))
204+
.WillOnce(Return(HttpResponse::MakeForTesting(200, kLoadTableDefaultResponse)));
205+
206+
ICEBERG_UNWRAP_OR_FAIL(auto catalog, MakeCatalog(all_plan_endpoints_));
207+
ICEBERG_UNWRAP_OR_FAIL(auto as_catalog, catalog->AsCatalog());
208+
ICEBERG_UNWRAP_OR_FAIL(auto table, as_catalog->LoadTable(identifier_));
209+
210+
EXPECT_EQ(dynamic_cast<RestTable*>(table.get()), nullptr)
211+
<< "Expected plain Table (not RestTable) when no scan-planning-mode is configured";
212+
}
213+
214+
} // namespace iceberg::rest

‎src/iceberg/test/rest_json_serde_test.cc‎

Lines changed: 10 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -1569,13 +1569,6 @@ INSTANTIATE_TEST_SUITE_P(
15691569
R"({"status":"submitted","plan-id":"somePlanId","plan-tasks":[]})",
15701570
.expected_error_kind = ErrorKind::kValidationFailed,
15711571
.expected_error_msg = "tasks can only be defined when status is 'completed'"},
1572-
PlanTableScanResponseInvalidParam{
1573-
.test_name = "FailedWithPlanId",
1574-
.json_str =
1575-
R"({"status":"failed","plan-id":"somePlanId","error":{"message":"x","type":"y","code":500}})",
1576-
.expected_error_kind = ErrorKind::kValidationFailed,
1577-
.expected_error_msg =
1578-
"plan id can only be defined when status is 'submitted' or 'completed'"},
15791572
PlanTableScanResponseInvalidParam{
15801573
.test_name = "FailedWithoutError",
15811574
.json_str = R"({"status":"failed"})",
@@ -2604,6 +2597,16 @@ TEST(PlanTableScanResponseRoundtripTest, FailedWithError) {
26042597
EXPECT_EQ(*result, *result2);
26052598
}
26062599

2600+
TEST(PlanTableScanResponseRoundtripTest, FailedWithPlanId) {
2601+
// A server may include a plan-id in a failed response so the client can cancel.
2602+
auto json = nlohmann::json::parse(
2603+
R"({"status":"failed","plan-id":"plan-123","error":{"message":"Planning failed","type":"PlanningException","code":500}})");
2604+
auto result = PlanTableScanResponseFromJson(json, EmptySpecs(), EmptySchema());
2605+
ASSERT_THAT(result, IsOk());
2606+
EXPECT_EQ(result->plan_id, "plan-123");
2607+
EXPECT_EQ(result->plan_status, PlanStatus::kFailed);
2608+
}
2609+
26072610
TEST(FetchPlanningResultResponseRoundtripTest, CompletedWithPlanTasks) {
26082611
auto json = nlohmann::json::parse(
26092612
R"({"status": "completed", "plan-tasks": ["task-1", "task-2"]})");

‎src/iceberg/test/rest_table_scan_test.cc‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -35,7 +35,9 @@
3535
#include "iceberg/catalog/rest/http_client.h"
3636
#include "iceberg/catalog/rest/resource_paths.h"
3737
#include "iceberg/catalog/rest/rest_table.h"
38+
#include "iceberg/constants.h"
3839
#include "iceberg/file_io.h"
40+
#include "iceberg/manifest/manifest_entry.h"
3941
#include "iceberg/partition_spec.h"
4042
#include "iceberg/schema.h"
4143
#include "iceberg/snapshot.h"
@@ -914,6 +916,7 @@ TEST_F(RestIncrementalAppendScanTest, PlanFilesEmptyWhenNoCurrentSnapshot) {
914916
.partition_specs = {spec},
915917
.default_spec_id = spec->spec_id(),
916918
.last_partition_id = 999,
919+
.current_snapshot_id = kInvalidSnapshotId,
917920
});
918921

919922
EXPECT_CALL(*mock_client_, Post(_, _, _, _, _)).Times(0);

0 commit comments

Comments
 (0)