Skip to content

Commit d388f81

Browse files
Sandeep GottimukkalaSandeep Gottimukkala
Sandeep Gottimukkala
authored and
Sandeep Gottimukkala
committed
Add tests and address all comments
1 parent 24ec964 commit d388f81

2 files changed

Lines changed: 209 additions & 29 deletions

File tree

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

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -117,7 +117,8 @@ Result<std::string> ResourcePaths::CommitTransaction() const {
117117

118118
Result<std::string> ResourcePaths::Plan(const TableIdentifier& ident,
119119
std::optional<std::string> plan_id) const {
120-
ICEBERG_ASSIGN_OR_RAISE(std::string encoded_namespace, EncodeNamespace(ident.ns));
120+
ICEBERG_ASSIGN_OR_RAISE(std::string encoded_namespace,
121+
EncodeNamespace(ident.ns, namespace_separator_));
121122
ICEBERG_ASSIGN_OR_RAISE(std::string encoded_table_name, EncodeString(ident.name));
122123
if (plan_id.has_value()) {
123124
ICEBERG_ASSIGN_OR_RAISE(std::string encoded_plan_id, EncodeString(plan_id.value()));
@@ -129,7 +130,8 @@ Result<std::string> ResourcePaths::Plan(const TableIdentifier& ident,
129130
}
130131

131132
Result<std::string> ResourcePaths::FetchScanTasks(const TableIdentifier& ident) const {
132-
ICEBERG_ASSIGN_OR_RAISE(std::string encoded_namespace, EncodeNamespace(ident.ns));
133+
ICEBERG_ASSIGN_OR_RAISE(std::string encoded_namespace,
134+
EncodeNamespace(ident.ns, namespace_separator_));
133135
ICEBERG_ASSIGN_OR_RAISE(std::string encoded_table_name, EncodeString(ident.name));
134136
return std::format("{}/v1/{}namespaces/{}/tables/{}/tasks", base_uri_, prefix_,
135137
encoded_namespace, encoded_table_name);

‎src/iceberg/test/rest_json_serde_test.cc‎

Lines changed: 205 additions & 27 deletions
Original file line numberDiff line numberDiff line change
@@ -1394,7 +1394,6 @@ static std::unordered_map<int32_t, std::shared_ptr<PartitionSpec>> EmptySpecs()
13941394
// --- PlanTableScanResponse ---
13951395

13961396
TEST(PlanTableScanResponseFromJsonTest, SubmittedStatusMissingOptionalFields) {
1397-
// "submitted" response: only status and plan-id, no tasks
13981397
auto json = nlohmann::json::parse(R"({"status":"submitted","plan-id":"abc-123"})");
13991398
auto result = PlanTableScanResponseFromJson(json, EmptySpecs(), EmptySchema());
14001399
ASSERT_THAT(result, IsOk());
@@ -1406,7 +1405,6 @@ TEST(PlanTableScanResponseFromJsonTest, SubmittedStatusMissingOptionalFields) {
14061405
}
14071406

14081407
TEST(PlanTableScanResponseFromJsonTest, CompletedStatusWithPlanTasks) {
1409-
// "completed" response with plan-tasks but no file-scan-tasks
14101408
auto json = nlohmann::json::parse(
14111409
R"({"status":"completed","plan-id":"abc-123","plan-tasks":["task-1","task-2"],"delete-files":[],"file-scan-tasks":[]})");
14121410
auto result = PlanTableScanResponseFromJson(json, EmptySpecs(), EmptySchema());
@@ -1418,21 +1416,79 @@ TEST(PlanTableScanResponseFromJsonTest, CompletedStatusWithPlanTasks) {
14181416
EXPECT_EQ(result->plan_tasks[1], "task-2");
14191417
}
14201418

1421-
TEST(PlanTableScanResponseFromJsonTest, MissingRequiredStatus) {
1422-
auto json = nlohmann::json::parse(R"({"plan-id":"abc-123"})");
1423-
auto result = PlanTableScanResponseFromJson(json, EmptySpecs(), EmptySchema());
1424-
ASSERT_THAT(result, IsError(ErrorKind::kJsonParseError));
1425-
EXPECT_THAT(result, HasErrorMessage("Missing 'status'"));
1426-
}
1427-
14281419
TEST(PlanTableScanResponseFromJsonTest, MissingPlanIdDefaultsToEmptyForFailedStatus) {
1429-
// plan-id is optional for non-submitted/completed statuses
14301420
auto json = nlohmann::json::parse(R"({"status":"failed"})");
14311421
auto result = PlanTableScanResponseFromJson(json, EmptySpecs(), EmptySchema());
14321422
ASSERT_THAT(result, IsOk());
14331423
EXPECT_TRUE(result->plan_id.empty());
14341424
}
14351425

1426+
struct PlanTableScanResponseInvalidParam {
1427+
std::string test_name;
1428+
std::string json_str;
1429+
ErrorKind expected_error_kind;
1430+
std::string expected_error_msg;
1431+
};
1432+
1433+
class PlanTableScanResponseInvalidTest
1434+
: public ::testing::TestWithParam<PlanTableScanResponseInvalidParam> {};
1435+
1436+
TEST_P(PlanTableScanResponseInvalidTest, InvalidInput) {
1437+
const auto& param = GetParam();
1438+
auto result = PlanTableScanResponseFromJson(nlohmann::json::parse(param.json_str),
1439+
EmptySpecs(), EmptySchema());
1440+
ASSERT_THAT(result, IsError(param.expected_error_kind));
1441+
if (!param.expected_error_msg.empty()) {
1442+
EXPECT_THAT(result, HasErrorMessage(param.expected_error_msg));
1443+
}
1444+
}
1445+
1446+
INSTANTIATE_TEST_SUITE_P(
1447+
PlanTableScanResponseInvalidCases, PlanTableScanResponseInvalidTest,
1448+
::testing::Values(
1449+
PlanTableScanResponseInvalidParam{
1450+
.test_name = "EmptyJson",
1451+
.json_str = R"({})",
1452+
.expected_error_kind = ErrorKind::kJsonParseError,
1453+
.expected_error_msg = "Missing 'status'"},
1454+
PlanTableScanResponseInvalidParam{
1455+
.test_name = "UnknownStatus",
1456+
.json_str = R"({"status":"someStatus"})",
1457+
.expected_error_kind = ErrorKind::kJsonParseError,
1458+
.expected_error_msg = "Unknown plan status"},
1459+
PlanTableScanResponseInvalidParam{
1460+
.test_name = "SubmittedWithoutPlanId",
1461+
.json_str = R"({"status":"submitted"})",
1462+
.expected_error_kind = ErrorKind::kValidationFailed,
1463+
.expected_error_msg = "plan id should be defined when status is 'submitted'"},
1464+
PlanTableScanResponseInvalidParam{
1465+
.test_name = "CancelledStatus",
1466+
.json_str = R"({"status":"cancelled"})",
1467+
.expected_error_kind = ErrorKind::kValidationFailed,
1468+
.expected_error_msg = "'cancelled' is not a valid status for planTableScan"},
1469+
PlanTableScanResponseInvalidParam{
1470+
.test_name = "SubmittedWithTasks",
1471+
.json_str =
1472+
R"({"status":"submitted","plan-id":"somePlanId","plan-tasks":["task1","task2"]})",
1473+
.expected_error_kind = ErrorKind::kValidationFailed,
1474+
.expected_error_msg = "tasks can only be defined when status is 'completed'"},
1475+
PlanTableScanResponseInvalidParam{
1476+
.test_name = "FailedWithPlanId",
1477+
.json_str = R"({"status":"failed","plan-id":"somePlanId"})",
1478+
.expected_error_kind = ErrorKind::kValidationFailed,
1479+
.expected_error_msg =
1480+
"plan id can only be defined when status is 'submitted' or 'completed'"},
1481+
PlanTableScanResponseInvalidParam{
1482+
.test_name = "DeleteFilesWithoutFileScanTasks",
1483+
.json_str =
1484+
R"({"status":"completed","delete-files":[{"content":"position_deletes","file-path":"s3://bucket/d.parquet","file-format":"PARQUET","file-size-in-bytes":512,"record-count":5}],"file-scan-tasks":[]})",
1485+
.expected_error_kind = ErrorKind::kValidationFailed,
1486+
.expected_error_msg =
1487+
"deleteFiles should only be returned with fileScanTasks"}),
1488+
[](const ::testing::TestParamInfo<PlanTableScanResponseInvalidParam>& info) {
1489+
return info.param.test_name;
1490+
});
1491+
14361492
// --- FetchPlanningResultResponse ---
14371493

14381494
TEST(FetchPlanningResultResponseFromJsonTest, SubmittedStatusNoTasks) {
@@ -1455,17 +1511,58 @@ TEST(FetchPlanningResultResponseFromJsonTest, CompletedStatusWithPlanTasks) {
14551511
EXPECT_EQ(result->plan_tasks[0], "task-1");
14561512
}
14571513

1458-
TEST(FetchPlanningResultResponseFromJsonTest, MissingRequiredStatus) {
1459-
auto json = nlohmann::json::parse(R"({})");
1460-
auto result = FetchPlanningResultResponseFromJson(json, EmptySpecs(), EmptySchema());
1461-
ASSERT_THAT(result, IsError(ErrorKind::kJsonParseError));
1462-
EXPECT_THAT(result, HasErrorMessage("Missing 'status'"));
1514+
struct FetchPlanningResultResponseInvalidParam {
1515+
std::string test_name;
1516+
std::string json_str;
1517+
ErrorKind expected_error_kind;
1518+
std::string expected_error_msg;
1519+
};
1520+
1521+
class FetchPlanningResultResponseInvalidTest
1522+
: public ::testing::TestWithParam<FetchPlanningResultResponseInvalidParam> {};
1523+
1524+
TEST_P(FetchPlanningResultResponseInvalidTest, InvalidInput) {
1525+
const auto& param = GetParam();
1526+
auto result = FetchPlanningResultResponseFromJson(nlohmann::json::parse(param.json_str),
1527+
EmptySpecs(), EmptySchema());
1528+
ASSERT_THAT(result, IsError(param.expected_error_kind));
1529+
if (!param.expected_error_msg.empty()) {
1530+
EXPECT_THAT(result, HasErrorMessage(param.expected_error_msg));
1531+
}
14631532
}
14641533

1534+
INSTANTIATE_TEST_SUITE_P(
1535+
FetchPlanningResultResponseInvalidCases, FetchPlanningResultResponseInvalidTest,
1536+
::testing::Values(
1537+
FetchPlanningResultResponseInvalidParam{
1538+
.test_name = "EmptyJson",
1539+
.json_str = R"({})",
1540+
.expected_error_kind = ErrorKind::kJsonParseError,
1541+
.expected_error_msg = "Missing 'status'"},
1542+
FetchPlanningResultResponseInvalidParam{
1543+
.test_name = "UnknownStatus",
1544+
.json_str = R"({"status":"someStatus"})",
1545+
.expected_error_kind = ErrorKind::kJsonParseError,
1546+
.expected_error_msg = "Unknown plan status"},
1547+
FetchPlanningResultResponseInvalidParam{
1548+
.test_name = "SubmittedWithTasks",
1549+
.json_str = R"({"status":"submitted","plan-tasks":["task1","task2"]})",
1550+
.expected_error_kind = ErrorKind::kValidationFailed,
1551+
.expected_error_msg = "tasks can only be returned in a 'completed' status"},
1552+
FetchPlanningResultResponseInvalidParam{
1553+
.test_name = "DeleteFilesWithoutFileScanTasks",
1554+
.json_str =
1555+
R"({"status":"submitted","delete-files":[{"content":"position_deletes","file-path":"s3://bucket/d.parquet","file-format":"PARQUET","file-size-in-bytes":512,"record-count":5}]})",
1556+
.expected_error_kind = ErrorKind::kValidationFailed,
1557+
.expected_error_msg =
1558+
"deleteFiles should only be returned with fileScanTasks"}),
1559+
[](const ::testing::TestParamInfo<FetchPlanningResultResponseInvalidParam>& info) {
1560+
return info.param.test_name;
1561+
});
1562+
14651563
// --- FetchScanTasksResponse ---
14661564

14671565
TEST(FetchScanTasksResponseFromJsonTest, WithFileScanTasks) {
1468-
// One file scan task with a data file and one delete file referenced by index.
14691566
auto json = nlohmann::json::parse(R"({
14701567
"plan-tasks": [],
14711568
"delete-files": [
@@ -1512,13 +1609,92 @@ TEST(FetchScanTasksResponseFromJsonTest, WithPlanTasksOnly) {
15121609
EXPECT_TRUE(result->file_scan_tasks.empty());
15131610
}
15141611

1515-
TEST(FetchScanTasksResponseFromJsonTest, AllFieldsMissing) {
1516-
// Both plan-tasks and file-scan-tasks absent → Validate() should fail
1517-
auto json = nlohmann::json::parse(R"({})");
1518-
auto result = FetchScanTasksResponseFromJson(json, EmptySpecs(), EmptySchema());
1519-
ASSERT_THAT(result, IsError(ErrorKind::kValidationFailed));
1612+
struct FetchScanTasksResponseInvalidParam {
1613+
std::string test_name;
1614+
std::string json_str;
1615+
ErrorKind expected_error_kind;
1616+
std::string expected_error_msg;
1617+
};
1618+
1619+
class FetchScanTasksResponseInvalidTest
1620+
: public ::testing::TestWithParam<FetchScanTasksResponseInvalidParam> {};
1621+
1622+
TEST_P(FetchScanTasksResponseInvalidTest, InvalidInput) {
1623+
const auto& param = GetParam();
1624+
auto result = FetchScanTasksResponseFromJson(nlohmann::json::parse(param.json_str),
1625+
EmptySpecs(), EmptySchema());
1626+
ASSERT_THAT(result, IsError(param.expected_error_kind));
1627+
if (!param.expected_error_msg.empty()) {
1628+
EXPECT_THAT(result, HasErrorMessage(param.expected_error_msg));
1629+
}
15201630
}
15211631

1632+
INSTANTIATE_TEST_SUITE_P(
1633+
FetchScanTasksResponseInvalidCases, FetchScanTasksResponseInvalidTest,
1634+
::testing::Values(
1635+
FetchScanTasksResponseInvalidParam{
1636+
.test_name = "EmptyJson",
1637+
.json_str = R"({})",
1638+
.expected_error_kind = ErrorKind::kValidationFailed,
1639+
.expected_error_msg = "planTasks and fileScanTask cannot both be null"},
1640+
FetchScanTasksResponseInvalidParam{
1641+
.test_name = "DeleteFilesWithoutFileScanTasks",
1642+
.json_str =
1643+
R"({"plan-tasks":["task1","task2"],"delete-files":[{"content":"position_deletes","file-path":"s3://bucket/d.parquet","file-format":"PARQUET","file-size-in-bytes":512,"record-count":5}],"file-scan-tasks":[]})",
1644+
.expected_error_kind = ErrorKind::kValidationFailed,
1645+
.expected_error_msg =
1646+
"deleteFiles should only be returned with fileScanTasks"}),
1647+
[](const ::testing::TestParamInfo<FetchScanTasksResponseInvalidParam>& info) {
1648+
return info.param.test_name;
1649+
});
1650+
1651+
// --- PlanTableScanRequest validation ---
1652+
1653+
struct PlanTableScanRequestValidationParam {
1654+
std::string test_name;
1655+
PlanTableScanRequest request;
1656+
std::string expected_error_msg;
1657+
};
1658+
1659+
class PlanTableScanRequestValidationTest
1660+
: public ::testing::TestWithParam<PlanTableScanRequestValidationParam> {};
1661+
1662+
TEST_P(PlanTableScanRequestValidationTest, ValidationFailed) {
1663+
const auto& param = GetParam();
1664+
auto status = param.request.Validate();
1665+
ASSERT_THAT(status, IsError(ErrorKind::kValidationFailed));
1666+
EXPECT_THAT(status, HasErrorMessage(param.expected_error_msg));
1667+
}
1668+
1669+
INSTANTIATE_TEST_SUITE_P(
1670+
PlanTableScanRequestValidationCases, PlanTableScanRequestValidationTest,
1671+
::testing::Values(
1672+
PlanTableScanRequestValidationParam{
1673+
.test_name = "SnapshotIdWithStartSnapshotId",
1674+
.request = {.snapshot_id = 1, .start_snapshot_id = 2, .end_snapshot_id = 5},
1675+
.expected_error_msg =
1676+
"cannot provide both snapshotId and startSnapshotId/endSnapshotId"},
1677+
PlanTableScanRequestValidationParam{
1678+
.test_name = "SnapshotIdWithEndSnapshotId",
1679+
.request = {.snapshot_id = 1, .end_snapshot_id = 5},
1680+
.expected_error_msg =
1681+
"cannot provide both snapshotId and startSnapshotId/endSnapshotId"},
1682+
PlanTableScanRequestValidationParam{
1683+
.test_name = "StartSnapshotIdWithoutEnd",
1684+
.request = {.start_snapshot_id = 1},
1685+
.expected_error_msg = "startSnapshotId and endSnapshotId is required"},
1686+
PlanTableScanRequestValidationParam{
1687+
.test_name = "EndSnapshotIdWithoutStart",
1688+
.request = {.end_snapshot_id = 5},
1689+
.expected_error_msg = "startSnapshotId and endSnapshotId is required"},
1690+
PlanTableScanRequestValidationParam{
1691+
.test_name = "NegativeMinRowsRequested",
1692+
.request = {.min_rows_requested = -1},
1693+
.expected_error_msg = "minRowsRequested is negative"}),
1694+
[](const ::testing::TestParamInfo<PlanTableScanRequestValidationParam>& info) {
1695+
return info.param.test_name;
1696+
});
1697+
15221698
// --- DataFileFromJson ---
15231699

15241700
TEST(DataFileFromJsonTest, RequiredFieldsOnly) {
@@ -1710,8 +1886,8 @@ TEST(FileScanTasksFromJsonTest, TaskWithDeleteFileReferences) {
17101886
"delete-file-references": [0]
17111887
}])"_json;
17121888

1713-
auto result =
1714-
FileScanTasksFromJson(json, {std::make_shared<DataFile>(delete_file)}, {}, Schema({}, 0));
1889+
auto result = FileScanTasksFromJson(json, {std::make_shared<DataFile>(delete_file)}, {},
1890+
Schema({}, 0));
17151891
ASSERT_THAT(result, IsOk());
17161892
ASSERT_EQ(result.value().size(), 1U);
17171893
const auto& task = result.value()[0];
@@ -1810,7 +1986,8 @@ TEST(FetchScanTasksResponseRoundtripTest, WithFileScanTasksAndDeleteFiles) {
18101986
ASSERT_THAT(result, IsOk());
18111987

18121988
auto roundtrip_json = ToJson(*result);
1813-
auto result2 = FetchScanTasksResponseFromJson(roundtrip_json, EmptySpecs(), EmptySchema());
1989+
auto result2 =
1990+
FetchScanTasksResponseFromJson(roundtrip_json, EmptySpecs(), EmptySchema());
18141991
ASSERT_THAT(result2, IsOk());
18151992
EXPECT_EQ(*result, *result2);
18161993
}
@@ -1821,14 +1998,15 @@ TEST(PlanTableScanResponseRoundtripTest, SubmittedStatus) {
18211998
ASSERT_THAT(result, IsOk());
18221999

18232000
auto roundtrip_json = ToJson(*result);
1824-
auto result2 = PlanTableScanResponseFromJson(roundtrip_json, EmptySpecs(), EmptySchema());
2001+
auto result2 =
2002+
PlanTableScanResponseFromJson(roundtrip_json, EmptySpecs(), EmptySchema());
18252003
ASSERT_THAT(result2, IsOk());
18262004
EXPECT_EQ(*result, *result2);
18272005
}
18282006

18292007
TEST(FetchPlanningResultResponseRoundtripTest, CompletedWithPlanTasks) {
1830-
auto json =
1831-
nlohmann::json::parse(R"({"status": "completed", "plan-tasks": ["task-1", "task-2"]})");
2008+
auto json = nlohmann::json::parse(
2009+
R"({"status": "completed", "plan-tasks": ["task-1", "task-2"]})");
18322010
auto result = FetchPlanningResultResponseFromJson(json, EmptySpecs(), EmptySchema());
18332011
ASSERT_THAT(result, IsOk());
18342012

0 commit comments

Comments
 (0)