Skip to content

Commit e576a53

Browse files
zhjwpkuzhjwpku
committed
fix: review comments
1 parent 5cf237c commit e576a53

File tree

3 files changed

+75
-66
lines changed

3 files changed

+75
-66
lines changed

‎src/iceberg/json_internal.h‎

Lines changed: 0 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -75,12 +75,6 @@ ICEBERG_EXPORT nlohmann::json ToJson(const SortOrder& sort_order);
7575
ICEBERG_EXPORT Result<std::unique_ptr<SortOrder>> SortOrderFromJson(
7676
const nlohmann::json& json, const std::shared_ptr<Schema>& current_schema);
7777

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.
81-
ICEBERG_EXPORT Result<std::unique_ptr<SortOrder>> SortOrderFromJson(
82-
const nlohmann::json& json);
83-
8478
/// \brief Convert an Iceberg Schema to JSON.
8579
///
8680
/// \param[in] schema The Iceberg schema to convert.

‎src/iceberg/test/json_internal_test.cc‎

Lines changed: 10 additions & 6 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,6 +102,10 @@ 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);
@@ -116,7 +115,12 @@ TEST(JsonInternalTest, SortOrder) {
116115
{"transform":"identity","source-id":5,"direction":"asc","null-order":"nulls-first"},
117116
{"transform":"identity","source-id":7,"direction":"desc","null-order":"nulls-last"}]})"_json;
118117

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

122126
TEST(JsonInternalTest, PartitionField) {

‎src/iceberg/test/sort_order_test.cc‎

Lines changed: 65 additions & 54 deletions
Original file line numberDiff line numberDiff line change
@@ -32,75 +32,76 @@
3232

3333
namespace iceberg {
3434

35-
class SortOrderMakeTest : public ::testing::Test {
35+
class SortOrderTest : public ::testing::Test {
3636
protected:
3737
void SetUp() override {
3838
field1_ = std::make_unique<SchemaField>(1, "x", int32(), true);
3939
field2_ = std::make_unique<SchemaField>(2, "y", string(), true);
40-
field3_ = std::make_unique<SchemaField>(3, "time", timestamp(), true);
40+
field3_ = std::make_unique<SchemaField>(5, "ts", iceberg::timestamp(), true);
41+
field4_ = std::make_unique<SchemaField>(7, "bar", iceberg::string(), true);
4142

4243
schema_ = std::make_unique<Schema>(
43-
std::vector<SchemaField>{*field1_, *field2_, *field3_}, 1);
44+
std::vector<SchemaField>{*field1_, *field2_, *field3_, *field4_}, 1);
4445

4546
sort_field1_ = std::make_unique<SortField>(
4647
1, Transform::Identity(), SortDirection::kAscending, NullOrder::kFirst);
4748
sort_field2_ = std::make_unique<SortField>(
4849
2, Transform::Bucket(10), SortDirection::kDescending, NullOrder::kLast);
4950
sort_field3_ = std::make_unique<SortField>(
50-
3, Transform::Day(), SortDirection::kAscending, NullOrder::kFirst);
51+
5, Transform::Day(), SortDirection::kAscending, NullOrder::kFirst);
5152
}
5253

5354
std::unique_ptr<Schema> schema_;
5455
std::unique_ptr<SchemaField> field1_;
5556
std::unique_ptr<SchemaField> field2_;
5657
std::unique_ptr<SchemaField> field3_;
58+
std::unique_ptr<SchemaField> field4_;
5759

5860
std::unique_ptr<SortField> sort_field1_;
5961
std::unique_ptr<SortField> sort_field2_;
6062
std::unique_ptr<SortField> sort_field3_;
6163
};
6264

63-
TEST(SortOrderTest, Basics) {
64-
{
65-
SchemaField field1(5, "ts", iceberg::timestamp(), true);
66-
SchemaField field2(7, "bar", iceberg::string(), true);
67-
68-
auto identity_transform = Transform::Identity();
69-
SortField st_field1(5, identity_transform, SortDirection::kAscending,
70-
NullOrder::kFirst);
71-
SortField st_field2(7, identity_transform, SortDirection::kDescending,
72-
NullOrder::kFirst);
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();
76-
ASSERT_EQ(2, fields.size());
77-
ASSERT_EQ(st_field1, fields[0]);
78-
ASSERT_EQ(st_field2, fields[1]);
79-
auto sort_order_str =
80-
"[\n"
81-
" identity(5) asc nulls-first\n"
82-
" identity(7) desc nulls-first\n"
83-
"]";
84-
EXPECT_EQ(sort_order->ToString(), sort_order_str);
85-
EXPECT_EQ(std::format("{}", *sort_order), sort_order_str);
86-
}
65+
TEST_F(SortOrderTest, Basics) {
66+
auto identity_transform = Transform::Identity();
67+
SortField st_field1(5, identity_transform, SortDirection::kAscending,
68+
NullOrder::kFirst);
69+
SortField st_field2(7, identity_transform, SortDirection::kDescending,
70+
NullOrder::kFirst);
71+
ICEBERG_UNWRAP_OR_FAIL(auto sort_order,
72+
SortOrder::Make(*schema_, 100, {st_field1, st_field2}));
73+
ASSERT_EQ(*sort_order, *sort_order);
74+
std::span<const SortField> fields = sort_order->fields();
75+
ASSERT_EQ(2, fields.size());
76+
ASSERT_EQ(st_field1, fields[0]);
77+
ASSERT_EQ(st_field2, fields[1]);
78+
auto sort_order_str =
79+
"[\n"
80+
" identity(5) asc nulls-first\n"
81+
" identity(7) desc nulls-first\n"
82+
"]";
83+
EXPECT_EQ(sort_order->ToString(), sort_order_str);
84+
EXPECT_EQ(std::format("{}", *sort_order), sort_order_str);
8785
}
8886

89-
TEST(SortOrderTest, Equality) {
90-
SchemaField field1(5, "ts", iceberg::timestamp(), true);
91-
SchemaField field2(7, "bar", iceberg::string(), true);
87+
TEST_F(SortOrderTest, Equality) {
9288
auto bucket_transform = Transform::Bucket(8);
9389
auto identity_transform = Transform::Identity();
9490
SortField st_field1(5, identity_transform, SortDirection::kAscending,
9591
NullOrder::kFirst);
9692
SortField st_field2(7, identity_transform, SortDirection::kDescending,
9793
NullOrder::kFirst);
9894
SortField st_field3(7, bucket_transform, SortDirection::kAscending, NullOrder::kFirst);
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}));
95+
ICEBERG_UNWRAP_OR_FAIL(auto sort_order1,
96+
SortOrder::Make(*schema_, 100, {st_field1, st_field2}));
97+
ICEBERG_UNWRAP_OR_FAIL(auto sort_order2,
98+
SortOrder::Make(*schema_, 100, {st_field2, st_field3}));
99+
ICEBERG_UNWRAP_OR_FAIL(auto sort_order3,
100+
SortOrder::Make(*schema_, 100, {st_field1, st_field3}));
101+
ICEBERG_UNWRAP_OR_FAIL(auto sort_order4,
102+
SortOrder::Make(*schema_, 101, {st_field1, st_field2}));
103+
ICEBERG_UNWRAP_OR_FAIL(auto sort_order5,
104+
SortOrder::Make(*schema_, 100, {st_field2, st_field1}));
104105

105106
ASSERT_EQ(*sort_order1, *sort_order1);
106107
ASSERT_NE(*sort_order1, *sort_order2);
@@ -113,26 +114,23 @@ TEST(SortOrderTest, Equality) {
113114
ASSERT_NE(*sort_order5, *sort_order1);
114115
}
115116

116-
TEST(SortOrderTest, IsUnsorted) {
117+
TEST_F(SortOrderTest, IsUnsorted) {
117118
auto unsorted = SortOrder::Unsorted();
118119
EXPECT_TRUE(unsorted->is_unsorted());
119120
EXPECT_FALSE(unsorted->is_sorted());
120121
}
121122

122-
TEST(SortOrderTest, IsSorted) {
123-
SchemaField field1(5, "ts", iceberg::timestamp(), true);
123+
TEST_F(SortOrderTest, IsSorted) {
124124
auto identity_transform = Transform::Identity();
125125
SortField st_field1(5, identity_transform, SortDirection::kAscending,
126126
NullOrder::kFirst);
127-
ICEBERG_UNWRAP_OR_FAIL(auto sorted_order, SortOrder::Make(100, {st_field1}));
127+
ICEBERG_UNWRAP_OR_FAIL(auto sorted_order, SortOrder::Make(*schema_, 100, {st_field1}));
128128

129129
EXPECT_TRUE(sorted_order->is_sorted());
130130
EXPECT_FALSE(sorted_order->is_unsorted());
131131
}
132132

133-
TEST(SortOrderTest, Satisfies) {
134-
SchemaField field1(5, "ts", iceberg::timestamp(), true);
135-
SchemaField field2(7, "bar", iceberg::string(), true);
133+
TEST_F(SortOrderTest, Satisfies) {
136134
auto identity_transform = Transform::Identity();
137135
auto bucket_transform = Transform::Bucket(8);
138136

@@ -142,10 +140,12 @@ TEST(SortOrderTest, Satisfies) {
142140
NullOrder::kFirst);
143141
SortField st_field3(7, bucket_transform, SortDirection::kAscending, NullOrder::kFirst);
144142

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}));
143+
ICEBERG_UNWRAP_OR_FAIL(auto sort_order1,
144+
SortOrder::Make(*schema_, 100, {st_field1, st_field2}));
145+
ICEBERG_UNWRAP_OR_FAIL(auto sort_order2, SortOrder::Make(*schema_, 101, {st_field1}));
146+
ICEBERG_UNWRAP_OR_FAIL(auto sort_order3,
147+
SortOrder::Make(*schema_, 102, {st_field1, st_field3}));
148+
ICEBERG_UNWRAP_OR_FAIL(auto sort_order4, SortOrder::Make(*schema_, 104, {st_field2}));
149149
auto unsorted = SortOrder::Unsorted();
150150

151151
// Any order satisfies an unsorted order, including unsorted itself
@@ -177,7 +177,7 @@ TEST(SortOrderTest, Satisfies) {
177177
EXPECT_FALSE(sort_order2->Satisfies(*sort_order4));
178178
}
179179

180-
TEST_F(SortOrderMakeTest, MakeValidSortOrder) {
180+
TEST_F(SortOrderTest, MakeValidSortOrder) {
181181
ICEBERG_UNWRAP_OR_FAIL(
182182
auto sort_order,
183183
SortOrder::Make(*schema_, 1, std::vector<SortField>{*sort_field1_, *sort_field2_}));
@@ -189,14 +189,14 @@ TEST_F(SortOrderMakeTest, MakeValidSortOrder) {
189189
EXPECT_EQ(sort_order->fields()[1], *sort_field2_);
190190
}
191191

192-
TEST_F(SortOrderMakeTest, MakeInvalidSortOrderEmptyFields) {
192+
TEST_F(SortOrderTest, MakeInvalidSortOrderEmptyFields) {
193193
auto sort_order = SortOrder::Make(*schema_, 1, std::vector<SortField>{});
194194
EXPECT_THAT(sort_order, IsError(ErrorKind::kInvalidArgument));
195195
EXPECT_THAT(sort_order,
196196
HasErrorMessage("Sort order must have at least one sort field"));
197197
}
198198

199-
TEST_F(SortOrderMakeTest, MakeInvalidSortOrderUnsortedId) {
199+
TEST_F(SortOrderTest, MakeInvalidSortOrderUnsortedId) {
200200
auto sort_order = SortOrder::Make(*schema_, SortOrder::kUnsortedOrderId,
201201
std::vector<SortField>{*sort_field1_});
202202
EXPECT_THAT(sort_order, IsError(ErrorKind::kInvalidArgument));
@@ -205,7 +205,7 @@ TEST_F(SortOrderMakeTest, MakeInvalidSortOrderUnsortedId) {
205205
SortOrder::kUnsortedOrderId)));
206206
}
207207

208-
TEST_F(SortOrderMakeTest, MakeValidUnsortedSortOrder) {
208+
TEST_F(SortOrderTest, MakeValidUnsortedSortOrder) {
209209
ICEBERG_UNWRAP_OR_FAIL(auto sort_order, SortOrder::Make(SortOrder::kUnsortedOrderId,
210210
std::vector<SortField>{}));
211211
ASSERT_NE(sort_order, nullptr);
@@ -214,7 +214,18 @@ TEST_F(SortOrderMakeTest, MakeValidUnsortedSortOrder) {
214214
EXPECT_EQ(sort_order->fields().size(), 0);
215215
}
216216

217-
TEST_F(SortOrderMakeTest, MakeInvalidSortOrderNonPrimitiveField) {
217+
TEST_F(SortOrderTest, MakeInvalidSortOrderTransformCannotApply) {
218+
// Day transform cannot be applied to string type
219+
SortField sort_field_invalid(2, Transform::Day(), SortDirection::kAscending,
220+
NullOrder::kFirst);
221+
222+
auto sort_order = SortOrder::Make(
223+
*schema_, 1, std::vector<SortField>{*sort_field1_, sort_field_invalid});
224+
EXPECT_THAT(sort_order, IsError(ErrorKind::kInvalidArgument));
225+
EXPECT_THAT(sort_order, HasErrorMessage("Invalid source type"));
226+
}
227+
228+
TEST_F(SortOrderTest, MakeInvalidSortOrderNonPrimitiveField) {
218229
auto struct_field = std::make_unique<SchemaField>(
219230
4, "struct_field",
220231
std::make_shared<StructType>(std::vector<SchemaField>{
@@ -234,7 +245,7 @@ TEST_F(SortOrderMakeTest, MakeInvalidSortOrderNonPrimitiveField) {
234245
EXPECT_THAT(sort_order, HasErrorMessage("Invalid source type"));
235246
}
236247

237-
TEST_F(SortOrderMakeTest, MakeInvalidSortOrderFieldNotInSchema) {
248+
TEST_F(SortOrderTest, MakeInvalidSortOrderFieldNotInSchema) {
238249
SortField sort_field_invalid(999, Transform::Identity(), SortDirection::kAscending,
239250
NullOrder::kFirst);
240251

@@ -244,7 +255,7 @@ TEST_F(SortOrderMakeTest, MakeInvalidSortOrderFieldNotInSchema) {
244255
EXPECT_THAT(sort_order, HasErrorMessage("Cannot find source column for sort field"));
245256
}
246257

247-
TEST_F(SortOrderMakeTest, MakeUnboundSortOrder) {
258+
TEST_F(SortOrderTest, MakeUnboundSortOrder) {
248259
SortField sort_field_invalid(999, Transform::Identity(), SortDirection::kAscending,
249260
NullOrder::kFirst);
250261

0 commit comments

Comments
 (0)