Skip to content

Commit 5cf237c

Browse files
zhjwpkuzhjwpku
committed
refactor: make SortOrder constructor private
1 parent 3b59ef2 commit 5cf237c

File tree

7 files changed

+103
-66
lines changed

7 files changed

+103
-66
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: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -69,8 +69,15 @@ 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.
75+
ICEBERG_EXPORT Result<std::unique_ptr<SortOrder>> SortOrderFromJson(
76+
const nlohmann::json& json, const std::shared_ptr<Schema>& current_schema);
77+
78+
/// \brief Deserializes a JSON object into a `SortOrder` object.
79+
///
80+
/// Same as above but the returned SortOrder will not be validated against any schema.
7481
ICEBERG_EXPORT Result<std::unique_ptr<SortOrder>> SortOrderFromJson(
7582
const nlohmann::json& json);
7683

‎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: 2 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -110,14 +110,13 @@ TEST(JsonInternalTest, SortOrder) {
110110
auto identity_transform = Transform::Identity();
111111
SortField st_ts(5, identity_transform, SortDirection::kAscending, NullOrder::kFirst);
112112
SortField st_bar(7, identity_transform, SortDirection::kDescending, NullOrder::kLast);
113-
SortOrder sort_order(100, {st_ts, st_bar});
114-
113+
ICEBERG_UNWRAP_OR_FAIL(auto sort_order, SortOrder::Make(100, {st_ts, st_bar}));
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+
TestJsonConversion(*sort_order, expected_sort_order);
121120
}
122121

123122
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,

‎src/iceberg/test/sort_order_test.cc‎

Lines changed: 43 additions & 43 deletions
Original file line numberDiff line numberDiff line change
@@ -70,9 +70,9 @@ TEST(SortOrderTest, Basics) {
7070
NullOrder::kFirst);
7171
SortField st_field2(7, identity_transform, SortDirection::kDescending,
7272
NullOrder::kFirst);
73-
SortOrder sort_order(100, {st_field1, st_field2});
74-
ASSERT_EQ(sort_order, sort_order);
75-
std::span<const SortField> fields = sort_order.fields();
73+
ICEBERG_UNWRAP_OR_FAIL(auto sort_order, SortOrder::Make(100, {st_field1, st_field2}));
74+
ASSERT_EQ(*sort_order, *sort_order);
75+
std::span<const SortField> fields = sort_order->fields();
7676
ASSERT_EQ(2, fields.size());
7777
ASSERT_EQ(st_field1, fields[0]);
7878
ASSERT_EQ(st_field2, fields[1]);
@@ -81,8 +81,8 @@ TEST(SortOrderTest, Basics) {
8181
" identity(5) asc nulls-first\n"
8282
" identity(7) desc nulls-first\n"
8383
"]";
84-
EXPECT_EQ(sort_order.ToString(), sort_order_str);
85-
EXPECT_EQ(std::format("{}", sort_order), sort_order_str);
84+
EXPECT_EQ(sort_order->ToString(), sort_order_str);
85+
EXPECT_EQ(std::format("{}", *sort_order), sort_order_str);
8686
}
8787
}
8888

@@ -96,21 +96,21 @@ TEST(SortOrderTest, Equality) {
9696
SortField st_field2(7, identity_transform, SortDirection::kDescending,
9797
NullOrder::kFirst);
9898
SortField st_field3(7, bucket_transform, SortDirection::kAscending, NullOrder::kFirst);
99-
SortOrder sort_order1(100, {st_field1, st_field2});
100-
SortOrder sort_order2(100, {st_field2, st_field3});
101-
SortOrder sort_order3(100, {st_field1, st_field3});
102-
SortOrder sort_order4(101, {st_field1, st_field2});
103-
SortOrder sort_order5(100, {st_field2, st_field1});
104-
105-
ASSERT_EQ(sort_order1, sort_order1);
106-
ASSERT_NE(sort_order1, sort_order2);
107-
ASSERT_NE(sort_order2, sort_order1);
108-
ASSERT_NE(sort_order1, sort_order3);
109-
ASSERT_NE(sort_order3, sort_order1);
110-
ASSERT_NE(sort_order1, sort_order4);
111-
ASSERT_NE(sort_order4, sort_order1);
112-
ASSERT_NE(sort_order1, sort_order5);
113-
ASSERT_NE(sort_order5, sort_order1);
99+
ICEBERG_UNWRAP_OR_FAIL(auto sort_order1, SortOrder::Make(100, {st_field1, st_field2}));
100+
ICEBERG_UNWRAP_OR_FAIL(auto sort_order2, SortOrder::Make(100, {st_field2, st_field3}));
101+
ICEBERG_UNWRAP_OR_FAIL(auto sort_order3, SortOrder::Make(100, {st_field1, st_field3}));
102+
ICEBERG_UNWRAP_OR_FAIL(auto sort_order4, SortOrder::Make(101, {st_field1, st_field2}));
103+
ICEBERG_UNWRAP_OR_FAIL(auto sort_order5, SortOrder::Make(100, {st_field2, st_field1}));
104+
105+
ASSERT_EQ(*sort_order1, *sort_order1);
106+
ASSERT_NE(*sort_order1, *sort_order2);
107+
ASSERT_NE(*sort_order2, *sort_order1);
108+
ASSERT_NE(*sort_order1, *sort_order3);
109+
ASSERT_NE(*sort_order3, *sort_order1);
110+
ASSERT_NE(*sort_order1, *sort_order4);
111+
ASSERT_NE(*sort_order4, *sort_order1);
112+
ASSERT_NE(*sort_order1, *sort_order5);
113+
ASSERT_NE(*sort_order5, *sort_order1);
114114
}
115115

116116
TEST(SortOrderTest, IsUnsorted) {
@@ -124,10 +124,10 @@ TEST(SortOrderTest, IsSorted) {
124124
auto identity_transform = Transform::Identity();
125125
SortField st_field1(5, identity_transform, SortDirection::kAscending,
126126
NullOrder::kFirst);
127-
SortOrder sorted_order(100, {st_field1});
127+
ICEBERG_UNWRAP_OR_FAIL(auto sorted_order, SortOrder::Make(100, {st_field1}));
128128

129-
EXPECT_TRUE(sorted_order.is_sorted());
130-
EXPECT_FALSE(sorted_order.is_unsorted());
129+
EXPECT_TRUE(sorted_order->is_sorted());
130+
EXPECT_FALSE(sorted_order->is_unsorted());
131131
}
132132

133133
TEST(SortOrderTest, Satisfies) {
@@ -142,39 +142,39 @@ TEST(SortOrderTest, Satisfies) {
142142
NullOrder::kFirst);
143143
SortField st_field3(7, bucket_transform, SortDirection::kAscending, NullOrder::kFirst);
144144

145-
SortOrder sort_order1(100, {st_field1, st_field2});
146-
SortOrder sort_order2(101, {st_field1});
147-
SortOrder sort_order3(102, {st_field1, st_field3});
148-
SortOrder sort_order4(104, {st_field2});
145+
ICEBERG_UNWRAP_OR_FAIL(auto sort_order1, SortOrder::Make(100, {st_field1, st_field2}));
146+
ICEBERG_UNWRAP_OR_FAIL(auto sort_order2, SortOrder::Make(101, {st_field1}));
147+
ICEBERG_UNWRAP_OR_FAIL(auto sort_order3, SortOrder::Make(102, {st_field1, st_field3}));
148+
ICEBERG_UNWRAP_OR_FAIL(auto sort_order4, SortOrder::Make(104, {st_field2}));
149149
auto unsorted = SortOrder::Unsorted();
150150

151151
// Any order satisfies an unsorted order, including unsorted itself
152152
EXPECT_TRUE(unsorted->Satisfies(*unsorted));
153-
EXPECT_TRUE(sort_order1.Satisfies(*unsorted));
154-
EXPECT_TRUE(sort_order2.Satisfies(*unsorted));
155-
EXPECT_TRUE(sort_order3.Satisfies(*unsorted));
153+
EXPECT_TRUE(sort_order1->Satisfies(*unsorted));
154+
EXPECT_TRUE(sort_order2->Satisfies(*unsorted));
155+
EXPECT_TRUE(sort_order3->Satisfies(*unsorted));
156156

157157
// Unsorted does not satisfy any sorted order
158-
EXPECT_FALSE(unsorted->Satisfies(sort_order1));
159-
EXPECT_FALSE(unsorted->Satisfies(sort_order2));
160-
EXPECT_FALSE(unsorted->Satisfies(sort_order3));
158+
EXPECT_FALSE(unsorted->Satisfies(*sort_order1));
159+
EXPECT_FALSE(unsorted->Satisfies(*sort_order2));
160+
EXPECT_FALSE(unsorted->Satisfies(*sort_order3));
161161

162162
// A sort order satisfies itself
163-
EXPECT_TRUE(sort_order1.Satisfies(sort_order1));
164-
EXPECT_TRUE(sort_order2.Satisfies(sort_order2));
165-
EXPECT_TRUE(sort_order3.Satisfies(sort_order3));
163+
EXPECT_TRUE(sort_order1->Satisfies(*sort_order1));
164+
EXPECT_TRUE(sort_order2->Satisfies(*sort_order2));
165+
EXPECT_TRUE(sort_order3->Satisfies(*sort_order3));
166166

167167
// A sort order with more fields satisfy one with fewer fields
168-
EXPECT_TRUE(sort_order1.Satisfies(sort_order2));
169-
EXPECT_TRUE(sort_order3.Satisfies(sort_order2));
168+
EXPECT_TRUE(sort_order1->Satisfies(*sort_order2));
169+
EXPECT_TRUE(sort_order3->Satisfies(*sort_order2));
170170

171171
// A sort order does not satisfy one with more fields
172-
EXPECT_FALSE(sort_order2.Satisfies(sort_order1));
173-
EXPECT_FALSE(sort_order2.Satisfies(sort_order3));
172+
EXPECT_FALSE(sort_order2->Satisfies(*sort_order1));
173+
EXPECT_FALSE(sort_order2->Satisfies(*sort_order3));
174174

175-
// A sort order does not satify one with different fields
176-
EXPECT_FALSE(sort_order4.Satisfies(sort_order2));
177-
EXPECT_FALSE(sort_order2.Satisfies(sort_order4));
175+
// A sort order does not satisfy one with different fields
176+
EXPECT_FALSE(sort_order4->Satisfies(*sort_order2));
177+
EXPECT_FALSE(sort_order2->Satisfies(*sort_order4));
178178
}
179179

180180
TEST_F(SortOrderMakeTest, MakeValidSortOrder) {

0 commit comments

Comments
 (0)