Skip to content

Commit 66b1c88

Browse files
Sandeep GottimukkalaSandeep Gottimukkala
authored andcommitted
Fixes and cf
1 parent 1f2bf61 commit 66b1c88

File tree

7 files changed

+22
-68
lines changed

7 files changed

+22
-68
lines changed

src/iceberg/catalog/rest/json_serde.cc

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -611,8 +611,8 @@ Result<FetchPlanningResultResponse> FetchPlanningResultResponseFromJson(
611611
partition_specs_by_id,
612612
const Schema& schema) {
613613
FetchPlanningResultResponse response;
614-
ICEBERG_ASSIGN_OR_RAISE(auto status_str, GetJsonValue<std::string>(json, kPlanStatus));
615-
response.plan_status = PlanStatus(PlanStatus::FromString(status_str));
614+
ICEBERG_ASSIGN_OR_RAISE(response.plan_status,
615+
GetJsonValue<std::string>(json, kPlanStatus));
616616
ICEBERG_RETURN_UNEXPECTED(
617617
BaseScanTaskResponseFromJson(json, &response, partition_specs_by_id, schema));
618618
ICEBERG_RETURN_UNEXPECTED(response.Validate());

src/iceberg/catalog/rest/types.cc

Lines changed: 3 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -154,8 +154,7 @@ bool PlanTableScanResponse::operator==(const PlanTableScanResponse& other) const
154154

155155
bool FetchPlanningResultResponse::operator==(
156156
const FetchPlanningResultResponse& other) const {
157-
return BaseScanTaskResponse::operator==(other) &&
158-
plan_status.ToString() == other.plan_status.ToString();
157+
return BaseScanTaskResponse::operator==(other) && plan_status == other.plan_status;
159158
}
160159

161160
bool FetchScanTasksRequest::operator==(const FetchScanTasksRequest& other) const {
@@ -233,11 +232,10 @@ Status PlanTableScanResponse::Validate() const {
233232
}
234233

235234
Status FetchPlanningResultResponse::Validate() const {
236-
if (plan_status.ToString() == "unknown") {
235+
if (plan_status.empty()) {
237236
return ValidationFailed("Invalid status: null");
238237
}
239-
if (plan_status.ToString() != "completed" &&
240-
(!plan_tasks.empty() || !file_scan_tasks.empty())) {
238+
if (plan_status != "completed" && (!plan_tasks.empty() || !file_scan_tasks.empty())) {
241239
return ValidationFailed(
242240
"Invalid response: tasks can only be returned in a 'completed' status");
243241
}

src/iceberg/catalog/rest/types.h

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -343,7 +343,7 @@ struct ICEBERG_REST_EXPORT PlanTableScanResponse : BaseScanTaskResponse {
343343
/// \brief Response from polling an asynchronous scan plan, including current status and
344344
/// available scan tasks.
345345
struct ICEBERG_REST_EXPORT FetchPlanningResultResponse : BaseScanTaskResponse {
346-
PlanStatus plan_status;
346+
std::string plan_status;
347347
// TODO: Add credentials.
348348

349349
Status Validate() const;

src/iceberg/json_serde.cc

Lines changed: 4 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -1761,12 +1761,11 @@ Result<DataFile> DataFileFromJson(
17611761
DataFile df;
17621762

17631763
ICEBERG_ASSIGN_OR_RAISE(auto content_str, GetJsonValue<std::string>(json, kContent));
1764-
const auto upper_content = StringUtils::ToUpper(content_str);
1765-
if (upper_content == "DATA") {
1764+
if (content_str == ToString(DataFile::Content::kData)) {
17661765
df.content = DataFile::Content::kData;
1767-
} else if (upper_content == "POSITION_DELETES") {
1766+
} else if (content_str == ToString(DataFile::Content::kPositionDeletes)) {
17681767
df.content = DataFile::Content::kPositionDeletes;
1769-
} else if (upper_content == "EQUALITY_DELETES") {
1768+
} else if (content_str == ToString(DataFile::Content::kEqualityDeletes)) {
17701769
df.content = DataFile::Content::kEqualityDeletes;
17711770
} else {
17721771
return JsonParseError("Unknown data file content: {}", content_str);
@@ -1835,7 +1834,7 @@ Result<DataFile> DataFileFromJson(
18351834
ICEBERG_RETURN_UNEXPECTED(parse_int_map(kNullValueCounts, df.null_value_counts));
18361835
ICEBERG_RETURN_UNEXPECTED(parse_int_map(kNanValueCounts, df.nan_value_counts));
18371836

1838-
// Parse BinaryMap: {"keys": [int, ...], "values": [base64 binary, ...]}
1837+
// Parse BinaryMap: {"keys": [int, ...], "values": [...]}
18391838
auto parse_binary_map = [&](std::string_view key,
18401839
std::map<int32_t, std::vector<uint8_t>>& target) -> Status {
18411840
if (!json.contains(key) || json.at(key).is_null()) return {};

src/iceberg/table_scan.h

Lines changed: 0 additions & 41 deletions
Original file line numberDiff line numberDiff line change
@@ -34,47 +34,6 @@
3434

3535
namespace iceberg {
3636

37-
class ICEBERG_EXPORT PlanStatus {
38-
public:
39-
enum class Status {
40-
kCompleted,
41-
kSubmitted,
42-
kCancelled,
43-
kFailed,
44-
kUnknown
45-
};
46-
47-
PlanStatus() : status_(Status::kUnknown) {}
48-
explicit PlanStatus(Status status) : status_(status) {}
49-
50-
static Status FromString(std::string_view status_str) {
51-
if (status_str == "completed") {
52-
return Status::kCompleted;
53-
} else if (status_str == "submitted") {
54-
return Status::kSubmitted;
55-
} else if (status_str == "cancelled") {
56-
return Status::kCancelled;
57-
} else if (status_str == "failed") {
58-
return Status::kFailed;
59-
}
60-
return Status::kUnknown;
61-
}
62-
63-
const std::string ToString() const {
64-
switch (status_) {
65-
case Status::kCompleted: return "completed";
66-
case Status::kSubmitted: return "submitted";
67-
case Status::kCancelled: return "cancelled";
68-
case Status::kFailed: return "failed";
69-
default: return "unknown";
70-
}
71-
}
72-
73-
private:
74-
Status status_;
75-
76-
};
77-
7837
/// \brief An abstract scan task.
7938
class ICEBERG_EXPORT ScanTask {
8039
public:

src/iceberg/test/json_serde_test.cc

Lines changed: 10 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -779,7 +779,7 @@ TEST(TableRequirementJsonTest, TableRequirementUnknownType) {
779779

780780
TEST(DataFileFromJsonTest, RequiredFieldsOnly) {
781781
auto json = R"({
782-
"content": "DATA",
782+
"content": "data",
783783
"file-path": "s3://bucket/data/file.parquet",
784784
"file-format": "PARQUET",
785785
"file-size-in-bytes": 12345,
@@ -799,8 +799,7 @@ TEST(DataFileFromJsonTest, RequiredFieldsOnly) {
799799
EXPECT_FALSE(df.partition_spec_id.has_value());
800800
}
801801

802-
TEST(DataFileFromJsonTest, LowercaseContentAndFormat) {
803-
// The REST API sends uppercase, but we should handle lowercase too.
802+
TEST(DataFileFromJsonTest, LowercaseFormat) {
804803
auto json = R"({
805804
"content": "data",
806805
"file-path": "s3://bucket/data/file.avro",
@@ -817,7 +816,7 @@ TEST(DataFileFromJsonTest, LowercaseContentAndFormat) {
817816

818817
TEST(DataFileFromJsonTest, WithOptionalFields) {
819818
auto json = R"({
820-
"content": "DATA",
819+
"content": "data",
821820
"file-path": "s3://bucket/data/file.parquet",
822821
"file-format": "PARQUET",
823822
"spec-id": 1,
@@ -852,7 +851,7 @@ TEST(DataFileFromJsonTest, WithOptionalFields) {
852851

853852
TEST(DataFileFromJsonTest, EqualityDeleteFile) {
854853
auto json = R"({
855-
"content": "EQUALITY_DELETES",
854+
"content": "equality_deletes",
856855
"file-path": "s3://bucket/deletes/eq_delete.parquet",
857856
"file-format": "PARQUET",
858857
"file-size-in-bytes": 5000,
@@ -871,7 +870,7 @@ TEST(DataFileFromJsonTest, EqualityDeleteFile) {
871870

872871
TEST(DataFileFromJsonTest, PositionDeleteFileWithReferencedDataFile) {
873872
auto json = R"({
874-
"content": "POSITION_DELETES",
873+
"content": "position_deletes",
875874
"file-path": "s3://bucket/deletes/pos_delete.parquet",
876875
"file-format": "PARQUET",
877876
"file-size-in-bytes": 3000,
@@ -904,7 +903,7 @@ TEST(DataFileFromJsonTest, InvalidContentType) {
904903
TEST(DataFileFromJsonTest, MissingRequiredField) {
905904
// Missing "file-path"
906905
auto json = R"({
907-
"content": "DATA",
906+
"content": "data",
908907
"file-format": "PARQUET",
909908
"file-size-in-bytes": 100,
910909
"record-count": 10
@@ -931,7 +930,7 @@ TEST(FileScanTasksFromJsonTest, EmptyArray) {
931930
TEST(FileScanTasksFromJsonTest, SingleTaskNoDeleteFiles) {
932931
auto json = R"([{
933932
"data-file": {
934-
"content": "DATA",
933+
"content": "data",
935934
"file-path": "s3://bucket/data/file.parquet",
936935
"file-format": "PARQUET",
937936
"file-size-in-bytes": 12345,
@@ -959,7 +958,7 @@ TEST(FileScanTasksFromJsonTest, TaskWithDeleteFileReferences) {
959958

960959
auto json = R"([{
961960
"data-file": {
962-
"content": "DATA",
961+
"content": "data",
963962
"file-path": "s3://bucket/data/file.parquet",
964963
"file-format": "PARQUET",
965964
"file-size-in-bytes": 12345,
@@ -973,14 +972,13 @@ TEST(FileScanTasksFromJsonTest, TaskWithDeleteFileReferences) {
973972
ASSERT_EQ(result.value().size(), 1U);
974973
const auto& task = result.value()[0];
975974
ASSERT_EQ(task.delete_files().size(), 1U);
976-
EXPECT_EQ(task.delete_files()[0]->file_path,
977-
"s3://bucket/deletes/pos_delete.parquet");
975+
EXPECT_EQ(task.delete_files()[0]->file_path, "s3://bucket/deletes/pos_delete.parquet");
978976
}
979977

980978
TEST(FileScanTasksFromJsonTest, DeleteFileReferenceOutOfRange) {
981979
auto json = R"([{
982980
"data-file": {
983-
"content": "DATA",
981+
"content": "data",
984982
"file-path": "s3://bucket/data/file.parquet",
985983
"file-format": "PARQUET",
986984
"file-size-in-bytes": 100,

src/iceberg/test/rest_json_serde_test.cc

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1467,7 +1467,7 @@ TEST(FetchScanTasksResponseFromJsonTest, WithFileScanTasks) {
14671467
"plan-tasks": [],
14681468
"delete-files": [
14691469
{
1470-
"content": "POSITION_DELETES",
1470+
"content": "position_deletes",
14711471
"file-path": "s3://bucket/deletes/delete.parquet",
14721472
"file-format": "PARQUET",
14731473
"file-size-in-bytes": 512,
@@ -1477,7 +1477,7 @@ TEST(FetchScanTasksResponseFromJsonTest, WithFileScanTasks) {
14771477
"file-scan-tasks": [
14781478
{
14791479
"data-file": {
1480-
"content": "DATA",
1480+
"content": "data",
14811481
"file-path": "s3://bucket/data/file.parquet",
14821482
"file-format": "PARQUET",
14831483
"file-size-in-bytes": 12345,

0 commit comments

Comments
 (0)