Skip to content

Commit 92cbbda

Browse files
zhjwpkuzhjwpku
authored
refactor: make SortOrder constructor private (#305)
1 parent f8323d2 commit 92cbbda

File tree

7 files changed

+162
-115
lines changed

7 files changed

+162
-115
lines changed

‎src/iceberg/json_internal.cc‎

Lines changed: 21 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -206,6 +206,19 @@ Result<std::unique_ptr<SortField>> SortFieldFromJson(const nlohmann::json& json)
206206
null_order);
207207
}
208208

209+
Result<std::unique_ptr<SortOrder>> SortOrderFromJson(
210+
const nlohmann::json& json, const std::shared_ptr<Schema>& current_schema) {
211+
ICEBERG_ASSIGN_OR_RAISE(auto order_id, GetJsonValue<int32_t>(json, kOrderId));
212+
ICEBERG_ASSIGN_OR_RAISE(auto fields, GetJsonValue<nlohmann::json>(json, kFields));
213+
214+
std::vector<SortField> sort_fields;
215+
for (const auto& field_json : fields) {
216+
ICEBERG_ASSIGN_OR_RAISE(auto sort_field, SortFieldFromJson(field_json));
217+
sort_fields.push_back(std::move(*sort_field));
218+
}
219+
return SortOrder::Make(*current_schema, order_id, std::move(sort_fields));
220+
}
221+
209222
Result<std::unique_ptr<SortOrder>> SortOrderFromJson(const nlohmann::json& json) {
210223
ICEBERG_ASSIGN_OR_RAISE(auto order_id, GetJsonValue<int32_t>(json, kOrderId));
211224
ICEBERG_ASSIGN_OR_RAISE(auto fields, GetJsonValue<nlohmann::json>(json, kFields));
@@ -215,7 +228,7 @@ Result<std::unique_ptr<SortOrder>> SortOrderFromJson(const nlohmann::json& json)
215228
ICEBERG_ASSIGN_OR_RAISE(auto sort_field, SortFieldFromJson(field_json));
216229
sort_fields.push_back(std::move(*sort_field));
217230
}
218-
return std::make_unique<SortOrder>(order_id, std::move(sort_fields));
231+
return SortOrder::Make(order_id, std::move(sort_fields));
219232
}
220233

221234
nlohmann::json ToJson(const SchemaField& field) {
@@ -919,9 +932,11 @@ Status ParsePartitionSpecs(const nlohmann::json& json, int8_t format_version,
919932
///
920933
/// \param[in] json The JSON object to parse.
921934
/// \param[in] format_version The format version of the table.
935+
/// \param[in] current_schema The current schema.
922936
/// \param[out] default_sort_order_id The default sort order ID.
923937
/// \param[out] sort_orders The list of sort orders.
924938
Status ParseSortOrders(const nlohmann::json& json, int8_t format_version,
939+
const std::shared_ptr<Schema>& current_schema,
925940
int32_t& default_sort_order_id,
926941
std::vector<std::shared_ptr<SortOrder>>& sort_orders) {
927942
if (json.contains(kSortOrders)) {
@@ -930,7 +945,8 @@ Status ParseSortOrders(const nlohmann::json& json, int8_t format_version,
930945
ICEBERG_ASSIGN_OR_RAISE(auto sort_order_array,
931946
GetJsonValue<nlohmann::json>(json, kSortOrders));
932947
for (const auto& sort_order_json : sort_order_array) {
933-
ICEBERG_ASSIGN_OR_RAISE(auto sort_order, SortOrderFromJson(sort_order_json));
948+
ICEBERG_ASSIGN_OR_RAISE(auto sort_order,
949+
SortOrderFromJson(sort_order_json, current_schema));
934950
sort_orders.push_back(std::move(sort_order));
935951
}
936952
} else {
@@ -1005,9 +1021,9 @@ Result<std::unique_ptr<TableMetadata>> TableMetadataFromJson(const nlohmann::jso
10051021
}
10061022
}
10071023

1008-
ICEBERG_RETURN_UNEXPECTED(ParseSortOrders(json, table_metadata->format_version,
1009-
table_metadata->default_sort_order_id,
1010-
table_metadata->sort_orders));
1024+
ICEBERG_RETURN_UNEXPECTED(ParseSortOrders(
1025+
json, table_metadata->format_version, current_schema,
1026+
table_metadata->default_sort_order_id, table_metadata->sort_orders));
10111027

10121028
if (json.contains(kProperties)) {
10131029
ICEBERG_ASSIGN_OR_RAISE(table_metadata->properties, FromJsonMap(json, kProperties));

‎src/iceberg/json_internal.h‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -69,10 +69,11 @@ ICEBERG_EXPORT nlohmann::json ToJson(const SortOrder& sort_order);
6969
/// Each `SortField` will be parsed using the `SortFieldFromJson` function.
7070
///
7171
/// \param json The JSON object representing a `SortOrder`.
72+
/// \param current_schema The current schema associated with the sort order.
7273
/// \return An `expected` value containing either a `SortOrder` object or an error. If the
7374
/// JSON is malformed or missing expected fields, an error will be returned.
7475
ICEBERG_EXPORT Result<std::unique_ptr<SortOrder>> SortOrderFromJson(
75-
const nlohmann::json& json);
76+
const nlohmann::json& json, const std::shared_ptr<Schema>& current_schema);
7677

7778
/// \brief Convert an Iceberg Schema to JSON.
7879
///

‎src/iceberg/sort_order.cc‎

Lines changed: 5 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -20,6 +20,7 @@
2020
#include "iceberg/sort_order.h"
2121

2222
#include <format>
23+
#include <memory>
2324
#include <optional>
2425
#include <ranges>
2526

@@ -36,8 +37,8 @@ SortOrder::SortOrder(int32_t order_id, std::vector<SortField> fields)
3637
: order_id_(order_id), fields_(std::move(fields)) {}
3738

3839
const std::shared_ptr<SortOrder>& SortOrder::Unsorted() {
39-
static const std::shared_ptr<SortOrder> unsorted =
40-
std::make_shared<SortOrder>(kUnsortedOrderId, std::vector<SortField>{});
40+
static const std::shared_ptr<SortOrder> unsorted = std::shared_ptr<SortOrder>(
41+
new SortOrder(kUnsortedOrderId, std::vector<SortField>{}));
4142
return unsorted;
4243
}
4344

@@ -113,7 +114,7 @@ Result<std::unique_ptr<SortOrder>> SortOrder::Make(const Schema& schema, int32_t
113114
return InvalidArgument("Sort order must have at least one sort field");
114115
}
115116

116-
auto sort_order = std::make_unique<SortOrder>(sort_id, std::move(fields));
117+
auto sort_order = std::unique_ptr<SortOrder>(new SortOrder(sort_id, std::move(fields)));
117118
ICEBERG_RETURN_UNEXPECTED(sort_order->Validate(schema));
118119
return sort_order;
119120
}
@@ -128,7 +129,7 @@ Result<std::unique_ptr<SortOrder>> SortOrder::Make(int32_t sort_id,
128129
return InvalidArgument("Sort order must have at least one sort field");
129130
}
130131

131-
return std::make_unique<SortOrder>(sort_id, std::move(fields));
132+
return std::unique_ptr<SortOrder>(new SortOrder(sort_id, std::move(fields)));
132133
}
133134

134135
} // namespace iceberg

‎src/iceberg/sort_order.h‎

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -41,8 +41,6 @@ class ICEBERG_EXPORT SortOrder : public util::Formattable {
4141
static constexpr int32_t kUnsortedOrderId = 0;
4242
static constexpr int32_t kInitialSortOrderId = 1;
4343

44-
SortOrder(int32_t order_id, std::vector<SortField> fields);
45-
4644
/// \brief Get an unsorted sort order singleton.
4745
static const std::shared_ptr<SortOrder>& Unsorted();
4846

@@ -95,6 +93,12 @@ class ICEBERG_EXPORT SortOrder : public util::Formattable {
9593
std::vector<SortField> fields);
9694

9795
private:
96+
/// \brief Constructs a SortOrder instance.
97+
/// \param order_id The sort order id.
98+
/// \param fields The sort fields.
99+
/// \note Use the static Make methods to create SortOrder instances.
100+
SortOrder(int32_t order_id, std::vector<SortField> fields);
101+
98102
/// \brief Compare two sort orders for equality.
99103
bool Equals(const SortOrder& other) const;
100104

‎src/iceberg/test/json_internal_test.cc‎

Lines changed: 12 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -49,11 +49,6 @@ Result<std::unique_ptr<SortField>> FromJsonHelper(const nlohmann::json& json) {
4949
return SortFieldFromJson(json);
5050
}
5151

52-
template <>
53-
Result<std::unique_ptr<SortOrder>> FromJsonHelper(const nlohmann::json& json) {
54-
return SortOrderFromJson(json);
55-
}
56-
5752
template <>
5853
Result<std::unique_ptr<PartitionField>> FromJsonHelper(const nlohmann::json& json) {
5954
return PartitionFieldFromJson(json);
@@ -107,17 +102,26 @@ TEST(JsonInternalTest, SortField) {
107102
}
108103

109104
TEST(JsonInternalTest, SortOrder) {
105+
auto schema = std::make_shared<Schema>(
106+
std::vector<SchemaField>{SchemaField(5, "region", iceberg::string(), false),
107+
SchemaField(7, "ts", iceberg::int64(), false)},
108+
/*schema_id=*/100);
110109
auto identity_transform = Transform::Identity();
111110
SortField st_ts(5, identity_transform, SortDirection::kAscending, NullOrder::kFirst);
112111
SortField st_bar(7, identity_transform, SortDirection::kDescending, NullOrder::kLast);
113-
SortOrder sort_order(100, {st_ts, st_bar});
114-
112+
ICEBERG_UNWRAP_OR_FAIL(auto sort_order, SortOrder::Make(100, {st_ts, st_bar}));
113+
EXPECT_TRUE(sort_order->Validate(*schema));
115114
nlohmann::json expected_sort_order =
116115
R"({"order-id":100,"fields":[
117116
{"transform":"identity","source-id":5,"direction":"asc","null-order":"nulls-first"},
118117
{"transform":"identity","source-id":7,"direction":"desc","null-order":"nulls-last"}]})"_json;
119118

120-
TestJsonConversion(sort_order, expected_sort_order);
119+
auto json = ToJson(*sort_order);
120+
EXPECT_EQ(expected_sort_order, json) << "JSON conversion mismatch.";
121+
122+
// Specialize FromJson based on type (T)
123+
ICEBERG_UNWRAP_OR_FAIL(auto obj_ex, SortOrderFromJson(expected_sort_order, schema));
124+
EXPECT_EQ(*sort_order, *obj_ex) << "Deserialized object mismatch.";
121125
}
122126

123127
TEST(JsonInternalTest, PartitionField) {

‎src/iceberg/test/metadata_serde_test.cc‎

Lines changed: 19 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -148,12 +148,17 @@ TEST(MetadataSerdeTest, DeserializeV2Valid) {
148148
std::vector<PartitionField>{PartitionField(/*source_id=*/1, /*field_id=*/1000, "x",
149149
Transform::Identity())});
150150

151-
auto expected_sort_order = std::make_shared<SortOrder>(
152-
/*order_id=*/3,
153-
std::vector<SortField>{SortField(/*source_id=*/2, Transform::Identity(),
154-
SortDirection::kAscending, NullOrder::kFirst),
155-
SortField(/*source_id=*/3, Transform::Bucket(4),
156-
SortDirection::kDescending, NullOrder::kLast)});
151+
ICEBERG_UNWRAP_OR_FAIL(
152+
auto sort_order,
153+
SortOrder::Make(*expected_schema_2,
154+
/*order_id=*/3,
155+
std::vector<SortField>{
156+
SortField(/*source_id=*/2, Transform::Identity(),
157+
SortDirection::kAscending, NullOrder::kFirst),
158+
SortField(/*source_id=*/3, Transform::Bucket(4),
159+
SortDirection::kDescending, NullOrder::kLast)}));
160+
161+
auto expected_sort_order = std::shared_ptr<SortOrder>(std::move(sort_order));
157162

158163
auto expected_snapshot_1 = std::make_shared<Snapshot>(Snapshot{
159164
.snapshot_id = 3051729675574597004,
@@ -228,13 +233,18 @@ TEST(MetadataSerdeTest, DeserializeV2ValidMinimal) {
228233
std::vector<PartitionField>{PartitionField(/*source_id=*/1, /*field_id=*/1000, "x",
229234
Transform::Identity())});
230235

231-
auto expected_sort_order = std::make_shared<SortOrder>(
232-
/*order_id=*/3, std::vector<SortField>{
236+
ICEBERG_UNWRAP_OR_FAIL(
237+
auto sort_order,
238+
SortOrder::Make(*expected_schema,
239+
/*order_id=*/3,
240+
std::vector<SortField>{
233241
SortField(/*source_id=*/2, Transform::Identity(),
234242
SortDirection::kAscending, NullOrder::kFirst),
235243
SortField(/*source_id=*/3, Transform::Bucket(4),
236244
SortDirection::kDescending, NullOrder::kLast),
237-
});
245+
}));
246+
247+
auto expected_sort_order = std::shared_ptr<SortOrder>(std::move(sort_order));
238248

239249
TableMetadata expected{
240250
.format_version = 2,

0 commit comments

Comments
 (0)