Skip to content

Commit 4f7d860

Browse files
author
nullccxsy
committed
fix comments
1 parent 0a2b037 commit 4f7d860

File tree

4 files changed

+34
-31
lines changed

4 files changed

+34
-31
lines changed

‎src/iceberg/type.cc‎

Lines changed: 19 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -30,7 +30,7 @@
3030

3131
namespace iceberg {
3232

33-
Result<std::optional<SchemaFieldConstRef>> NestedType::GetFieldByName(
33+
Result<std::optional<NestedType::SchemaFieldConstRef>> NestedType::GetFieldByName(
3434
std::string_view name) const {
3535
return GetFieldByName(name, /*case_sensitive=*/true);
3636
}
@@ -48,21 +48,21 @@ std::string StructType::ToString() const {
4848
return repr;
4949
}
5050
std::span<const SchemaField> StructType::fields() const { return fields_; }
51-
Result<std::optional<SchemaFieldConstRef>> StructType::GetFieldById(
51+
Result<std::optional<NestedType::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<SchemaFieldConstRef>> StructType::GetFieldByIndex(
58+
Result<std::optional<NestedType::SchemaFieldConstRef>> StructType::GetFieldByIndex(
5959
int32_t index) const {
6060
if (index < 0 || static_cast<size_t>(index) >= fields_.size()) {
61-
return InvalidArgument("index {} is out of range[0, {})", index, fields_.size());
61+
return InvalidArgument("Invalid index {} to get field from struct", index);
6262
}
6363
return fields_[index];
6464
}
65-
Result<std::optional<SchemaFieldConstRef>> StructType::GetFieldByName(
65+
Result<std::optional<NestedType::SchemaFieldConstRef>> StructType::GetFieldByName(
6666
std::string_view name, bool case_sensitive) const {
6767
if (case_sensitive) {
6868
ICEBERG_RETURN_UNEXPECTED(InitFieldByName());
@@ -149,20 +149,21 @@ std::string ListType::ToString() const {
149149
return repr;
150150
}
151151
std::span<const SchemaField> ListType::fields() const { return {&element_, 1}; }
152-
Result<std::optional<SchemaFieldConstRef>> ListType::GetFieldById(
152+
Result<std::optional<NestedType::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<SchemaFieldConstRef>> ListType::GetFieldByIndex(int index) const {
159+
Result<std::optional<NestedType::SchemaFieldConstRef>> ListType::GetFieldByIndex(
160+
int index) const {
160161
if (index == 0) {
161162
return std::cref(element_);
162163
}
163-
return InvalidArgument("index {} is out of range[0, {})", index, 1);
164+
return InvalidArgument("Invalid index {} to get field from list", index);
164165
}
165-
Result<std::optional<SchemaFieldConstRef>> ListType::GetFieldByName(
166+
Result<std::optional<NestedType::SchemaFieldConstRef>> ListType::GetFieldByName(
166167
std::string_view name, bool case_sensitive) const {
167168
if (case_sensitive) {
168169
if (name == kElementName) {
@@ -208,23 +209,25 @@ std::string MapType::ToString() const {
208209
return repr;
209210
}
210211
std::span<const SchemaField> MapType::fields() const { return fields_; }
211-
Result<std::optional<SchemaFieldConstRef>> MapType::GetFieldById(int32_t field_id) const {
212+
Result<std::optional<NestedType::SchemaFieldConstRef>> MapType::GetFieldById(
213+
int32_t field_id) const {
212214
if (field_id == key().field_id()) {
213215
return key();
214216
} else if (field_id == value().field_id()) {
215217
return value();
216218
}
217219
return std::nullopt;
218220
}
219-
Result<std::optional<SchemaFieldConstRef>> MapType::GetFieldByIndex(int32_t index) const {
221+
Result<std::optional<NestedType::SchemaFieldConstRef>> MapType::GetFieldByIndex(
222+
int32_t index) const {
220223
if (index == 0) {
221224
return key();
222225
} else if (index == 1) {
223226
return value();
224227
}
225-
return InvalidArgument("index {} is out of range[0, {})", index, 2);
228+
return InvalidArgument("Invalid index {} to get field from map", index);
226229
}
227-
Result<std::optional<SchemaFieldConstRef>> MapType::GetFieldByName(
230+
Result<std::optional<NestedType::SchemaFieldConstRef>> MapType::GetFieldByName(
228231
std::string_view name, bool case_sensitive) const {
229232
if (case_sensitive) {
230233
if (name == kKeyName) {
@@ -234,9 +237,10 @@ Result<std::optional<SchemaFieldConstRef>> MapType::GetFieldByName(
234237
}
235238
return std::nullopt;
236239
}
237-
if (StringUtils::ToLower(name) == kKeyName) {
240+
const auto lower_case_name = StringUtils::ToLower(name);
241+
if (lower_case_name == kKeyName) {
238242
return key();
239-
} else if (StringUtils::ToLower(name) == kValueName) {
243+
} else if (lower_case_name == kValueName) {
240244
return value();
241245
}
242246
return std::nullopt;

‎src/iceberg/type.h‎

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

71-
using SchemaFieldConstRef = std::reference_wrapper<const SchemaField>;
7271
/// \brief A data type that has child fields.
7372
class ICEBERG_EXPORT NestedType : public Type {
7473
public:
7574
bool is_primitive() const override { return false; }
7675
bool is_nested() const override { return true; }
76+
using SchemaFieldConstRef = std::reference_wrapper<const SchemaField>;
7777

7878
/// \brief Get a view of the child fields.
7979
[[nodiscard]] virtual std::span<const SchemaField> fields() const = 0;
@@ -106,6 +106,7 @@ class ICEBERG_EXPORT NestedType : public Type {
106106
/// \brief A data type representing a struct with nested fields.
107107
class ICEBERG_EXPORT StructType : public NestedType {
108108
public:
109+
using NestedType::GetFieldByName;
109110
constexpr static TypeId kTypeId = TypeId::kStruct;
110111
explicit StructType(std::vector<SchemaField> fields);
111112
~StructType() override = default;
@@ -121,10 +122,10 @@ class ICEBERG_EXPORT StructType : public NestedType {
121122
Result<std::optional<SchemaFieldConstRef>> GetFieldByName(
122123
std::string_view name, bool case_sensitive) const override;
123124

124-
using NestedType::GetFieldByName;
125-
126125
protected:
127126
bool Equals(const Type& other) const override;
127+
// TODO(nullccxsy): Lazy initialization has concurrency issues, need to add proper
128+
// synchronization mechanism
128129
Status InitFieldById() const;
129130
Status InitFieldByName() const;
130131
Status InitFieldByLowerCaseName() const;
@@ -139,6 +140,7 @@ class ICEBERG_EXPORT StructType : public NestedType {
139140
/// \brief A data type representing a list of values.
140141
class ICEBERG_EXPORT ListType : public NestedType {
141142
public:
143+
using NestedType::GetFieldByName;
142144
constexpr static const TypeId kTypeId = TypeId::kList;
143145
constexpr static const std::string_view kElementName = "element";
144146

@@ -160,8 +162,6 @@ class ICEBERG_EXPORT ListType : public NestedType {
160162
Result<std::optional<SchemaFieldConstRef>> GetFieldByName(
161163
std::string_view name, bool case_sensitive) const override;
162164

163-
using NestedType::GetFieldByName;
164-
165165
protected:
166166
bool Equals(const Type& other) const override;
167167

@@ -171,6 +171,7 @@ class ICEBERG_EXPORT ListType : public NestedType {
171171
/// \brief A data type representing a dictionary of values.
172172
class ICEBERG_EXPORT MapType : public NestedType {
173173
public:
174+
using NestedType::GetFieldByName;
174175
constexpr static const TypeId kTypeId = TypeId::kMap;
175176
constexpr static const std::string_view kKeyName = "key";
176177
constexpr static const std::string_view kValueName = "value";
@@ -194,8 +195,6 @@ class ICEBERG_EXPORT MapType : public NestedType {
194195
Result<std::optional<SchemaFieldConstRef>> GetFieldByName(
195196
std::string_view name, bool case_sensitive) const override;
196197

197-
using NestedType::GetFieldByName;
198-
199198
protected:
200199
bool Equals(const Type& other) const override;
201200

‎test/schema_test.cc‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -50,11 +50,11 @@ TEST(SchemaTest, Basics) {
5050
auto result = schema.GetFieldByIndex(2);
5151
ASSERT_EQ(result.error().kind, iceberg::ErrorKind::kInvalidArgument);
5252
ASSERT_THAT(result.error().message,
53-
::testing::HasSubstr("index 2 is out of range[0, 2)"));
53+
::testing::HasSubstr("Invalid index 2 to get field from struct"));
5454
result = schema.GetFieldByIndex(-1);
5555
ASSERT_EQ(result.error().kind, iceberg::ErrorKind::kInvalidArgument);
5656
ASSERT_THAT(result.error().message,
57-
::testing::HasSubstr("index -1 is out of range[0, 2)"));
57+
::testing::HasSubstr("Invalid index -1 to get field from struct"));
5858
ASSERT_EQ(std::nullopt, schema.GetFieldByName("element"));
5959
}
6060
}

‎test/type_test.cc‎

Lines changed: 7 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -318,19 +318,19 @@ TEST(TypeTest, List) {
318318
auto result = list.GetFieldByIndex(5);
319319
ASSERT_EQ(result.error().kind, iceberg::ErrorKind::kInvalidArgument);
320320
ASSERT_THAT(result.error().message,
321-
::testing::HasSubstr("index 5 is out of range[0, 1)"));
321+
::testing::HasSubstr("Invalid index 5 to get field from list"));
322322
ASSERT_THAT(list.GetFieldByIndex(0), ::testing::Optional(field));
323323
ASSERT_THAT(list.GetFieldByName("element"), ::testing::Optional(field));
324324

325325
ASSERT_EQ(std::nullopt, list.GetFieldById(0));
326326
result = list.GetFieldByIndex(1);
327327
ASSERT_EQ(result.error().kind, iceberg::ErrorKind::kInvalidArgument);
328328
ASSERT_THAT(result.error().message,
329-
::testing::HasSubstr("index 1 is out of range[0, 1)"));
329+
::testing::HasSubstr("Invalid index 1 to get field from list"));
330330
result = list.GetFieldByIndex(-1);
331331
ASSERT_EQ(result.error().kind, iceberg::ErrorKind::kInvalidArgument);
332332
ASSERT_THAT(result.error().message,
333-
::testing::HasSubstr("index -1 is out of range[0, 1)"));
333+
::testing::HasSubstr("Invalid index -1 to get field from list"));
334334
ASSERT_EQ(std::nullopt, list.GetFieldByName("foo"));
335335
}
336336
ASSERT_THAT(
@@ -362,11 +362,11 @@ TEST(TypeTest, Map) {
362362
auto result = map.GetFieldByIndex(2);
363363
ASSERT_EQ(result.error().kind, iceberg::ErrorKind::kInvalidArgument);
364364
ASSERT_THAT(result.error().message,
365-
::testing::HasSubstr("index 2 is out of range[0, 2)"));
365+
::testing::HasSubstr("Invalid index 2 to get field from map"));
366366
result = map.GetFieldByIndex(-1);
367367
ASSERT_EQ(result.error().kind, iceberg::ErrorKind::kInvalidArgument);
368368
ASSERT_THAT(result.error().message,
369-
::testing::HasSubstr("index -1 is out of range[0, 2)"));
369+
::testing::HasSubstr("Invalid index -1 to get field from map"));
370370
ASSERT_EQ(std::nullopt, map.GetFieldByName("element"));
371371
}
372372
ASSERT_THAT(
@@ -407,11 +407,11 @@ TEST(TypeTest, Struct) {
407407
auto result = struct_.GetFieldByIndex(2);
408408
ASSERT_EQ(result.error().kind, iceberg::ErrorKind::kInvalidArgument);
409409
ASSERT_THAT(result.error().message,
410-
::testing::HasSubstr("index 2 is out of range[0, 2)"));
410+
::testing::HasSubstr("Invalid index 2 to get field from struct"));
411411
result = struct_.GetFieldByIndex(-1);
412412
ASSERT_EQ(result.error().kind, iceberg::ErrorKind::kInvalidArgument);
413413
ASSERT_THAT(result.error().message,
414-
::testing::HasSubstr("index -1 is out of range[0, 2)"));
414+
::testing::HasSubstr("Invalid index -1 to get field from struct"));
415415
ASSERT_EQ(std::nullopt, struct_.GetFieldByName("element"));
416416
}
417417
}

0 commit comments

Comments
 (0)