Skip to content

Commit 1dcd138

Browse files
author
nullccxsy
committed
fix comments
1 parent 3df2d42 commit 1dcd138

File tree

4 files changed

+90
-68
lines changed

4 files changed

+90
-68
lines changed

‎src/iceberg/type.cc‎

Lines changed: 29 additions & 32 deletions
Original file line numberDiff line numberDiff line change
@@ -30,9 +30,9 @@
3030

3131
namespace iceberg {
3232

33-
Result<std::optional<std::reference_wrapper<const SchemaField>>>
34-
NestedType::GetFieldByName(std::string_view name) const {
35-
return GetFieldByName(name, true);
33+
Result<std::optional<SchemaFieldConstRef>> NestedType::GetFieldByName(
34+
std::string_view name) const {
35+
return GetFieldByName(name, /*case_sensitive=*/true);
3636
}
3737

3838
StructType::StructType(std::vector<SchemaField> fields) : fields_(std::move(fields)) {}
@@ -48,22 +48,22 @@ std::string StructType::ToString() const {
4848
return repr;
4949
}
5050
std::span<const SchemaField> StructType::fields() const { return fields_; }
51-
Result<std::optional<std::reference_wrapper<const SchemaField>>> StructType::GetFieldById(
51+
Result<std::optional<SchemaFieldConstRef>> StructType::GetFieldById(
5252
int32_t field_id) const {
5353
ICEBERG_RETURN_UNEXPECTED(InitFieldById());
5454
auto it = field_by_id_.find(field_id);
5555
if (it == field_by_id_.end()) return std::nullopt;
5656
return it->second;
5757
}
58-
Result<std::optional<std::reference_wrapper<const SchemaField>>>
59-
StructType::GetFieldByIndex(int32_t index) const {
58+
Result<std::optional<SchemaFieldConstRef>> StructType::GetFieldByIndex(
59+
int32_t index) const {
6060
if (index < 0 || static_cast<size_t>(index) >= fields_.size()) {
61-
return std::nullopt;
61+
return InvalidArgument("index {} is out of range[0, {})", index, fields_.size());
6262
}
6363
return fields_[index];
6464
}
65-
Result<std::optional<std::reference_wrapper<const SchemaField>>>
66-
StructType::GetFieldByName(std::string_view name, bool case_sensitive) const {
65+
Result<std::optional<SchemaFieldConstRef>> StructType::GetFieldByName(
66+
std::string_view name, bool case_sensitive) const {
6767
if (case_sensitive) {
6868
ICEBERG_RETURN_UNEXPECTED(InitFieldByName());
6969
auto it = field_by_name_.find(name);
@@ -93,8 +93,8 @@ Status StructType::InitFieldById() const {
9393
for (const auto& field : fields_) {
9494
auto it = field_by_id_.try_emplace(field.field_id(), field);
9595
if (!it.second) {
96-
return NotAllowed("Duplicate field id found: {} (prev name: {}, curr name: {})",
97-
field.field_id(), it.first->second.get().name(), field.name());
96+
return InvalidSchema("Duplicate field id found: {} (prev name: {}, curr name: {})",
97+
field.field_id(), it.first->second.get().name(), field.name());
9898
}
9999
}
100100
return {};
@@ -106,9 +106,9 @@ Status StructType::InitFieldByName() const {
106106
for (const auto& field : fields_) {
107107
auto it = field_by_name_.try_emplace(field.name(), field);
108108
if (!it.second) {
109-
return NotAllowed("Duplicate field name found: {} (prev id: {}, curr id: {})",
110-
it.first->first, it.first->second.get().field_id(),
111-
field.field_id());
109+
return InvalidSchema("Duplicate field name found: {} (prev id: {}, curr id: {})",
110+
it.first->first, it.first->second.get().field_id(),
111+
field.field_id());
112112
}
113113
}
114114
return {};
@@ -121,9 +121,9 @@ Status StructType::InitFieldByLowerCaseName() const {
121121
auto it =
122122
field_by_lowercase_name_.try_emplace(StringUtils::ToLower(field.name()), field);
123123
if (!it.second) {
124-
return NotAllowed("Duplicate field name found: {} (prev id: {}, curr id: {})",
125-
it.first->first, it.first->second.get().field_id(),
126-
field.field_id());
124+
return InvalidSchema("Duplicate field name found: {} (prev id: {}, curr id: {})",
125+
it.first->first, it.first->second.get().field_id(),
126+
field.field_id());
127127
}
128128
}
129129
return {};
@@ -149,29 +149,28 @@ std::string ListType::ToString() const {
149149
return repr;
150150
}
151151
std::span<const SchemaField> ListType::fields() const { return {&element_, 1}; }
152-
Result<std::optional<std::reference_wrapper<const SchemaField>>> ListType::GetFieldById(
152+
Result<std::optional<SchemaFieldConstRef>> ListType::GetFieldById(
153153
int32_t field_id) const {
154154
if (field_id == element_.field_id()) {
155155
return std::cref(element_);
156156
}
157157
return std::nullopt;
158158
}
159-
Result<std::optional<std::reference_wrapper<const SchemaField>>>
160-
ListType::GetFieldByIndex(int index) const {
159+
Result<std::optional<SchemaFieldConstRef>> ListType::GetFieldByIndex(int index) const {
161160
if (index == 0) {
162161
return std::cref(element_);
163162
}
164-
return std::nullopt;
163+
return InvalidArgument("index {} is out of range[0, {})", index, 1);
165164
}
166-
Result<std::optional<std::reference_wrapper<const SchemaField>>> ListType::GetFieldByName(
165+
Result<std::optional<SchemaFieldConstRef>> ListType::GetFieldByName(
167166
std::string_view name, bool case_sensitive) const {
168167
if (case_sensitive) {
169-
if (name == element_.name()) {
168+
if (name == kElementName) {
170169
return std::cref(element_);
171170
}
172171
return std::nullopt;
173172
}
174-
if (StringUtils::ToLower(name) == StringUtils::ToLower(element_.name())) {
173+
if (StringUtils::ToLower(name) == kElementName) {
175174
return std::cref(element_);
176175
}
177176
return std::nullopt;
@@ -209,25 +208,23 @@ std::string MapType::ToString() const {
209208
return repr;
210209
}
211210
std::span<const SchemaField> MapType::fields() const { return fields_; }
212-
Result<std::optional<std::reference_wrapper<const SchemaField>>> MapType::GetFieldById(
213-
int32_t field_id) const {
211+
Result<std::optional<SchemaFieldConstRef>> MapType::GetFieldById(int32_t field_id) const {
214212
if (field_id == key().field_id()) {
215213
return key();
216214
} else if (field_id == value().field_id()) {
217215
return value();
218216
}
219217
return std::nullopt;
220218
}
221-
Result<std::optional<std::reference_wrapper<const SchemaField>>> MapType::GetFieldByIndex(
222-
int32_t index) const {
219+
Result<std::optional<SchemaFieldConstRef>> MapType::GetFieldByIndex(int32_t index) const {
223220
if (index == 0) {
224221
return key();
225222
} else if (index == 1) {
226223
return value();
227224
}
228-
return std::nullopt;
225+
return InvalidArgument("index {} is out of range[0, {})", index, 2);
229226
}
230-
Result<std::optional<std::reference_wrapper<const SchemaField>>> MapType::GetFieldByName(
227+
Result<std::optional<SchemaFieldConstRef>> MapType::GetFieldByName(
231228
std::string_view name, bool case_sensitive) const {
232229
if (case_sensitive) {
233230
if (name == kKeyName) {
@@ -237,9 +234,9 @@ Result<std::optional<std::reference_wrapper<const SchemaField>>> MapType::GetFie
237234
}
238235
return std::nullopt;
239236
}
240-
if (StringUtils::ToLower(name) == StringUtils::ToLower(kKeyName)) {
237+
if (StringUtils::ToLower(name) == kKeyName) {
241238
return key();
242-
} else if (StringUtils::ToLower(name) == StringUtils::ToLower(kValueName)) {
239+
} else if (StringUtils::ToLower(name) == kValueName) {
243240
return value();
244241
}
245242
return std::nullopt;

‎src/iceberg/type.h‎

Lines changed: 22 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -68,6 +68,7 @@ class ICEBERG_EXPORT PrimitiveType : public Type {
6868
bool is_nested() const override { return false; }
6969
};
7070

71+
using SchemaFieldConstRef = std::reference_wrapper<const SchemaField>;
7172
/// \brief A data type that has child fields.
7273
class ICEBERG_EXPORT NestedType : public Type {
7374
public:
@@ -79,23 +80,23 @@ class ICEBERG_EXPORT NestedType : public Type {
7980
/// \brief Get a field by field ID.
8081
///
8182
/// \note This is O(1) complexity.
82-
[[nodiscard]] virtual Result<std::optional<std::reference_wrapper<const SchemaField>>>
83-
GetFieldById(int32_t field_id) const = 0;
83+
[[nodiscard]] virtual Result<std::optional<SchemaFieldConstRef>> GetFieldById(
84+
int32_t field_id) const = 0;
8485
/// \brief Get a field by index.
8586
///
8687
/// \note This is O(1) complexity.
87-
[[nodiscard]] virtual Result<std::optional<std::reference_wrapper<const SchemaField>>>
88-
GetFieldByIndex(int32_t index) const = 0;
89-
/// \brief Get a field by name. Behavior is not allowed if
88+
[[nodiscard]] virtual Result<std::optional<SchemaFieldConstRef>> GetFieldByIndex(
89+
int32_t index) const = 0;
90+
/// \brief Get a field by name. Return an error Status if
9091
/// the field name is not unique; prefer GetFieldById or GetFieldByIndex
9192
/// when possible.
9293
///
9394
/// \note This is currently O(1) complexity.
94-
[[nodiscard]] virtual Result<std::optional<std::reference_wrapper<const SchemaField>>>
95-
GetFieldByName(std::string_view name, bool case_sensitive) const = 0;
95+
[[nodiscard]] virtual Result<std::optional<SchemaFieldConstRef>> GetFieldByName(
96+
std::string_view name, bool case_sensitive) const = 0;
9697
/// \brief Get a field by name(case-sensitive).
97-
[[nodiscard]] Result<std::optional<std::reference_wrapper<const SchemaField>>>
98-
GetFieldByName(std::string_view name) const;
98+
[[nodiscard]] Result<std::optional<SchemaFieldConstRef>> GetFieldByName(
99+
std::string_view name) const;
99100
};
100101

101102
/// \defgroup type-nested Nested Types
@@ -113,11 +114,11 @@ class ICEBERG_EXPORT StructType : public NestedType {
113114
std::string ToString() const override;
114115

115116
std::span<const SchemaField> fields() const override;
116-
Result<std::optional<std::reference_wrapper<const SchemaField>>> GetFieldById(
117+
Result<std::optional<SchemaFieldConstRef>> GetFieldById(
117118
int32_t field_id) const override;
118-
Result<std::optional<std::reference_wrapper<const SchemaField>>> GetFieldByIndex(
119+
Result<std::optional<SchemaFieldConstRef>> GetFieldByIndex(
119120
int32_t index) const override;
120-
Result<std::optional<std::reference_wrapper<const SchemaField>>> GetFieldByName(
121+
Result<std::optional<SchemaFieldConstRef>> GetFieldByName(
121122
std::string_view name, bool case_sensitive) const override;
122123

123124
using NestedType::GetFieldByName;
@@ -130,12 +131,9 @@ class ICEBERG_EXPORT StructType : public NestedType {
130131

131132
protected:
132133
std::vector<SchemaField> fields_;
133-
mutable std::unordered_map<int32_t, std::reference_wrapper<const SchemaField>>
134-
field_by_id_;
135-
mutable std::unordered_map<std::string_view, std::reference_wrapper<const SchemaField>>
136-
field_by_name_;
137-
mutable std::unordered_map<std::string, std::reference_wrapper<const SchemaField>>
138-
field_by_lowercase_name_;
134+
mutable std::unordered_map<int32_t, SchemaFieldConstRef> field_by_id_;
135+
mutable std::unordered_map<std::string_view, SchemaFieldConstRef> field_by_name_;
136+
mutable std::unordered_map<std::string, SchemaFieldConstRef> field_by_lowercase_name_;
139137
};
140138

141139
/// \brief A data type representing a list of values.
@@ -155,11 +153,11 @@ class ICEBERG_EXPORT ListType : public NestedType {
155153
std::string ToString() const override;
156154

157155
std::span<const SchemaField> fields() const override;
158-
Result<std::optional<std::reference_wrapper<const SchemaField>>> GetFieldById(
156+
Result<std::optional<SchemaFieldConstRef>> GetFieldById(
159157
int32_t field_id) const override;
160-
Result<std::optional<std::reference_wrapper<const SchemaField>>> GetFieldByIndex(
158+
Result<std::optional<SchemaFieldConstRef>> GetFieldByIndex(
161159
int32_t index) const override;
162-
Result<std::optional<std::reference_wrapper<const SchemaField>>> GetFieldByName(
160+
Result<std::optional<SchemaFieldConstRef>> GetFieldByName(
163161
std::string_view name, bool case_sensitive) const override;
164162

165163
using NestedType::GetFieldByName;
@@ -189,11 +187,11 @@ class ICEBERG_EXPORT MapType : public NestedType {
189187
std::string ToString() const override;
190188

191189
std::span<const SchemaField> fields() const override;
192-
Result<std::optional<std::reference_wrapper<const SchemaField>>> GetFieldById(
190+
Result<std::optional<SchemaFieldConstRef>> GetFieldById(
193191
int32_t field_id) const override;
194-
Result<std::optional<std::reference_wrapper<const SchemaField>>> GetFieldByIndex(
192+
Result<std::optional<SchemaFieldConstRef>> GetFieldByIndex(
195193
int32_t index) const override;
196-
Result<std::optional<std::reference_wrapper<const SchemaField>>> GetFieldByName(
194+
Result<std::optional<SchemaFieldConstRef>> GetFieldByName(
197195
std::string_view name, bool case_sensitive) const override;
198196

199197
using NestedType::GetFieldByName;

‎test/schema_test.cc‎

Lines changed: 8 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -47,8 +47,14 @@ TEST(SchemaTest, Basics) {
4747
ASSERT_THAT(schema.GetFieldByName("bar"), ::testing::Optional(field2));
4848

4949
ASSERT_EQ(std::nullopt, schema.GetFieldById(0));
50-
ASSERT_EQ(std::nullopt, schema.GetFieldByIndex(2));
51-
ASSERT_EQ(std::nullopt, schema.GetFieldByIndex(-1));
50+
auto result = schema.GetFieldByIndex(2);
51+
ASSERT_EQ(result.error().kind, iceberg::ErrorKind::kInvalidArgument);
52+
ASSERT_THAT(result.error().message,
53+
::testing::HasSubstr("index 2 is out of range[0, 2)"));
54+
result = schema.GetFieldByIndex(-1);
55+
ASSERT_EQ(result.error().kind, iceberg::ErrorKind::kInvalidArgument);
56+
ASSERT_THAT(result.error().message,
57+
::testing::HasSubstr("index -1 is out of range[0, 2)"));
5258
ASSERT_EQ(std::nullopt, schema.GetFieldByName("element"));
5359
}
5460
}

‎test/type_test.cc‎

Lines changed: 31 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -315,13 +315,22 @@ TEST(TypeTest, List) {
315315
std::span<const iceberg::SchemaField> fields = list.fields();
316316
ASSERT_EQ(1, fields.size());
317317
ASSERT_EQ(field, fields[0]);
318-
ASSERT_THAT(list.GetFieldById(5), ::testing::Optional(field));
318+
auto result = list.GetFieldByIndex(5);
319+
ASSERT_EQ(result.error().kind, iceberg::ErrorKind::kInvalidArgument);
320+
ASSERT_THAT(result.error().message,
321+
::testing::HasSubstr("index 5 is out of range[0, 1)"));
319322
ASSERT_THAT(list.GetFieldByIndex(0), ::testing::Optional(field));
320323
ASSERT_THAT(list.GetFieldByName("element"), ::testing::Optional(field));
321324

322325
ASSERT_EQ(std::nullopt, list.GetFieldById(0));
323-
ASSERT_EQ(std::nullopt, list.GetFieldByIndex(1));
324-
ASSERT_EQ(std::nullopt, list.GetFieldByIndex(-1));
326+
result = list.GetFieldByIndex(1);
327+
ASSERT_EQ(result.error().kind, iceberg::ErrorKind::kInvalidArgument);
328+
ASSERT_THAT(result.error().message,
329+
::testing::HasSubstr("index 1 is out of range[0, 1)"));
330+
result = list.GetFieldByIndex(-1);
331+
ASSERT_EQ(result.error().kind, iceberg::ErrorKind::kInvalidArgument);
332+
ASSERT_THAT(result.error().message,
333+
::testing::HasSubstr("index -1 is out of range[0, 1)"));
325334
ASSERT_EQ(std::nullopt, list.GetFieldByName("foo"));
326335
}
327336
ASSERT_THAT(
@@ -350,8 +359,14 @@ TEST(TypeTest, Map) {
350359
ASSERT_THAT(map.GetFieldByName("value"), ::testing::Optional(value));
351360

352361
ASSERT_EQ(std::nullopt, map.GetFieldById(0));
353-
ASSERT_EQ(std::nullopt, map.GetFieldByIndex(2));
354-
ASSERT_EQ(std::nullopt, map.GetFieldByIndex(-1));
362+
auto result = map.GetFieldByIndex(2);
363+
ASSERT_EQ(result.error().kind, iceberg::ErrorKind::kInvalidArgument);
364+
ASSERT_THAT(result.error().message,
365+
::testing::HasSubstr("index 2 is out of range[0, 2)"));
366+
result = map.GetFieldByIndex(-1);
367+
ASSERT_EQ(result.error().kind, iceberg::ErrorKind::kInvalidArgument);
368+
ASSERT_THAT(result.error().message,
369+
::testing::HasSubstr("index -1 is out of range[0, 2)"));
355370
ASSERT_EQ(std::nullopt, map.GetFieldByName("element"));
356371
}
357372
ASSERT_THAT(
@@ -389,8 +404,14 @@ TEST(TypeTest, Struct) {
389404
ASSERT_THAT(struct_.GetFieldByName("bar"), ::testing::Optional(field2));
390405

391406
ASSERT_EQ(std::nullopt, struct_.GetFieldById(0));
392-
ASSERT_EQ(std::nullopt, struct_.GetFieldByIndex(2));
393-
ASSERT_EQ(std::nullopt, struct_.GetFieldByIndex(-1));
407+
auto result = struct_.GetFieldByIndex(2);
408+
ASSERT_EQ(result.error().kind, iceberg::ErrorKind::kInvalidArgument);
409+
ASSERT_THAT(result.error().message,
410+
::testing::HasSubstr("index 2 is out of range[0, 2)"));
411+
result = struct_.GetFieldByIndex(-1);
412+
ASSERT_EQ(result.error().kind, iceberg::ErrorKind::kInvalidArgument);
413+
ASSERT_THAT(result.error().message,
414+
::testing::HasSubstr("index -1 is out of range[0, 2)"));
394415
ASSERT_EQ(std::nullopt, struct_.GetFieldByName("element"));
395416
}
396417
}
@@ -459,7 +480,7 @@ TEST(TypeTest, StructDuplicateId) {
459480

460481
auto result = struct_.GetFieldById(5);
461482
ASSERT_FALSE(result.has_value());
462-
ASSERT_EQ(result.error().kind, iceberg::ErrorKind::kNotAllowed);
483+
ASSERT_EQ(result.error().kind, iceberg::ErrorKind::kInvalidSchema);
463484
ASSERT_THAT(result.error().message,
464485
::testing::HasSubstr(
465486
"Duplicate field id found: 5 (prev name: foo, curr name: bar)"));
@@ -472,7 +493,7 @@ TEST(TypeTest, StructDuplicateName) {
472493

473494
auto result = struct_.GetFieldByName("foo", true);
474495
ASSERT_FALSE(result.has_value());
475-
ASSERT_EQ(result.error().kind, iceberg::ErrorKind::kNotAllowed);
496+
ASSERT_EQ(result.error().kind, iceberg::ErrorKind::kInvalidSchema);
476497
ASSERT_THAT(
477498
result.error().message,
478499
::testing::HasSubstr("Duplicate field name found: foo (prev id: 1, curr id: 2)"));
@@ -485,7 +506,7 @@ TEST(TypeTest, StructDuplicateLowerCaseName) {
485506

486507
auto result = struct_.GetFieldByName("foo", false);
487508
ASSERT_FALSE(result.has_value());
488-
ASSERT_EQ(result.error().kind, iceberg::ErrorKind::kNotAllowed);
509+
ASSERT_EQ(result.error().kind, iceberg::ErrorKind::kInvalidSchema);
489510
ASSERT_THAT(
490511
result.error().message,
491512
::testing::HasSubstr("Duplicate field name found: foo (prev id: 1, curr id: 2)"));

0 commit comments

Comments
 (0)