Skip to content

Commit 9575c54

Browse files
author
nullccxsy
committed
fix comments
1 parent 4f7d860 commit 9575c54

File tree

5 files changed

+56
-53
lines changed

5 files changed

+56
-53
lines changed

‎src/iceberg/type.cc‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -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 InvalidSchema("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(
125+
"Duplicate lowercase field name found: {} (prev id: {}, curr id: {})",
126+
it.first->first, it.first->second.get().field_id(), field.field_id());
127127
}
128128
}
129129
return {};

‎src/iceberg/type.h‎

Lines changed: 8 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -70,10 +70,12 @@ class ICEBERG_EXPORT PrimitiveType : public Type {
7070

7171
/// \brief A data type that has child fields.
7272
class ICEBERG_EXPORT NestedType : public Type {
73+
protected:
74+
using SchemaFieldConstRef = std::reference_wrapper<const SchemaField>;
75+
7376
public:
7477
bool is_primitive() const override { return false; }
7578
bool is_nested() const override { return true; }
76-
using SchemaFieldConstRef = std::reference_wrapper<const SchemaField>;
7779

7880
/// \brief Get a view of the child fields.
7981
[[nodiscard]] virtual std::span<const SchemaField> fields() const = 0;
@@ -91,10 +93,10 @@ class ICEBERG_EXPORT NestedType : public Type {
9193
/// the field name is not unique; prefer GetFieldById or GetFieldByIndex
9294
/// when possible.
9395
///
94-
/// \note This is currently O(1) complexity.
96+
/// \note This is O(1) complexity.
9597
[[nodiscard]] virtual Result<std::optional<SchemaFieldConstRef>> GetFieldByName(
9698
std::string_view name, bool case_sensitive) const = 0;
97-
/// \brief Get a field by name(case-sensitive).
99+
/// \brief Get a field by name (case-sensitive).
98100
[[nodiscard]] Result<std::optional<SchemaFieldConstRef>> GetFieldByName(
99101
std::string_view name) const;
100102
};
@@ -106,7 +108,6 @@ class ICEBERG_EXPORT NestedType : public Type {
106108
/// \brief A data type representing a struct with nested fields.
107109
class ICEBERG_EXPORT StructType : public NestedType {
108110
public:
109-
using NestedType::GetFieldByName;
110111
constexpr static TypeId kTypeId = TypeId::kStruct;
111112
explicit StructType(std::vector<SchemaField> fields);
112113
~StructType() override = default;
@@ -121,6 +122,7 @@ class ICEBERG_EXPORT StructType : public NestedType {
121122
int32_t index) const override;
122123
Result<std::optional<SchemaFieldConstRef>> GetFieldByName(
123124
std::string_view name, bool case_sensitive) const override;
125+
using NestedType::GetFieldByName;
124126

125127
protected:
126128
bool Equals(const Type& other) const override;
@@ -140,7 +142,6 @@ class ICEBERG_EXPORT StructType : public NestedType {
140142
/// \brief A data type representing a list of values.
141143
class ICEBERG_EXPORT ListType : public NestedType {
142144
public:
143-
using NestedType::GetFieldByName;
144145
constexpr static const TypeId kTypeId = TypeId::kList;
145146
constexpr static const std::string_view kElementName = "element";
146147

@@ -161,6 +162,7 @@ class ICEBERG_EXPORT ListType : public NestedType {
161162
int32_t index) const override;
162163
Result<std::optional<SchemaFieldConstRef>> GetFieldByName(
163164
std::string_view name, bool case_sensitive) const override;
165+
using NestedType::GetFieldByName;
164166

165167
protected:
166168
bool Equals(const Type& other) const override;
@@ -171,7 +173,6 @@ class ICEBERG_EXPORT ListType : public NestedType {
171173
/// \brief A data type representing a dictionary of values.
172174
class ICEBERG_EXPORT MapType : public NestedType {
173175
public:
174-
using NestedType::GetFieldByName;
175176
constexpr static const TypeId kTypeId = TypeId::kMap;
176177
constexpr static const std::string_view kKeyName = "key";
177178
constexpr static const std::string_view kValueName = "value";
@@ -194,6 +195,7 @@ class ICEBERG_EXPORT MapType : public NestedType {
194195
int32_t index) const override;
195196
Result<std::optional<SchemaFieldConstRef>> GetFieldByName(
196197
std::string_view name, bool case_sensitive) const override;
198+
using NestedType::GetFieldByName;
197199

198200
protected:
199201
bool Equals(const Type& other) const override;

‎src/iceberg/util/macros.h‎

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -19,12 +19,12 @@
1919

2020
#pragma once
2121

22-
#define ICEBERG_RETURN_UNEXPECTED(result) \
23-
do { \
24-
auto&& iceberg_temp_result = (result); \
25-
if (!iceberg_temp_result) [[unlikely]] { \
26-
return std::unexpected<Error>(iceberg_temp_result.error()); \
27-
} \
22+
#define ICEBERG_RETURN_UNEXPECTED(result) \
23+
do { \
24+
auto&& result_name = (result); \
25+
if (!result_name) [[unlikely]] { \
26+
return std::unexpected<Error>(result_name.error()); \
27+
} \
2828
} while (false);
2929

3030
#define ICEBERG_ASSIGN_OR_RAISE_IMPL(result_name, lhs, rexpr) \

‎test/schema_test.cc‎

Lines changed: 7 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -27,6 +27,7 @@
2727

2828
#include "iceberg/schema_field.h"
2929
#include "iceberg/util/formatter.h" // IWYU pragma: keep
30+
#include "matchers.h"
3031

3132
TEST(SchemaTest, Basics) {
3233
{
@@ -48,13 +49,13 @@ TEST(SchemaTest, Basics) {
4849

4950
ASSERT_EQ(std::nullopt, schema.GetFieldById(0));
5051
auto result = schema.GetFieldByIndex(2);
51-
ASSERT_EQ(result.error().kind, iceberg::ErrorKind::kInvalidArgument);
52-
ASSERT_THAT(result.error().message,
53-
::testing::HasSubstr("Invalid index 2 to get field from struct"));
52+
ASSERT_THAT(result, IsError(iceberg::ErrorKind::kInvalidArgument));
53+
ASSERT_THAT(result,
54+
iceberg::HasErrorMessage("Invalid index 2 to get field from struct"));
5455
result = schema.GetFieldByIndex(-1);
55-
ASSERT_EQ(result.error().kind, iceberg::ErrorKind::kInvalidArgument);
56-
ASSERT_THAT(result.error().message,
57-
::testing::HasSubstr("Invalid index -1 to get field from struct"));
56+
ASSERT_THAT(result, IsError(iceberg::ErrorKind::kInvalidArgument));
57+
ASSERT_THAT(result,
58+
iceberg::HasErrorMessage("Invalid index -1 to get field from struct"));
5859
ASSERT_EQ(std::nullopt, schema.GetFieldByName("element"));
5960
}
6061
}

‎test/type_test.cc‎

Lines changed: 32 additions & 32 deletions
Original file line numberDiff line numberDiff line change
@@ -28,6 +28,7 @@
2828

2929
#include "iceberg/exception.h"
3030
#include "iceberg/util/formatter.h" // IWYU pragma: keep
31+
#include "matchers.h"
3132

3233
struct TypeTestCase {
3334
/// Test case name, must be safe for Googletest (alphanumeric + underscore)
@@ -316,21 +317,21 @@ TEST(TypeTest, List) {
316317
ASSERT_EQ(1, fields.size());
317318
ASSERT_EQ(field, fields[0]);
318319
auto result = list.GetFieldByIndex(5);
319-
ASSERT_EQ(result.error().kind, iceberg::ErrorKind::kInvalidArgument);
320-
ASSERT_THAT(result.error().message,
321-
::testing::HasSubstr("Invalid index 5 to get field from list"));
320+
ASSERT_THAT(result, IsError(iceberg::ErrorKind::kInvalidArgument));
321+
ASSERT_THAT(result,
322+
iceberg::HasErrorMessage("Invalid index 5 to get field from list"));
322323
ASSERT_THAT(list.GetFieldByIndex(0), ::testing::Optional(field));
323324
ASSERT_THAT(list.GetFieldByName("element"), ::testing::Optional(field));
324325

325326
ASSERT_EQ(std::nullopt, list.GetFieldById(0));
326327
result = list.GetFieldByIndex(1);
327-
ASSERT_EQ(result.error().kind, iceberg::ErrorKind::kInvalidArgument);
328-
ASSERT_THAT(result.error().message,
329-
::testing::HasSubstr("Invalid index 1 to get field from list"));
328+
ASSERT_THAT(result, IsError(iceberg::ErrorKind::kInvalidArgument));
329+
ASSERT_THAT(result,
330+
iceberg::HasErrorMessage("Invalid index 1 to get field from list"));
330331
result = list.GetFieldByIndex(-1);
331-
ASSERT_EQ(result.error().kind, iceberg::ErrorKind::kInvalidArgument);
332-
ASSERT_THAT(result.error().message,
333-
::testing::HasSubstr("Invalid index -1 to get field from list"));
332+
ASSERT_THAT(result, IsError(iceberg::ErrorKind::kInvalidArgument));
333+
ASSERT_THAT(result,
334+
iceberg::HasErrorMessage("Invalid index -1 to get field from list"));
334335
ASSERT_EQ(std::nullopt, list.GetFieldByName("foo"));
335336
}
336337
ASSERT_THAT(
@@ -360,13 +361,13 @@ TEST(TypeTest, Map) {
360361

361362
ASSERT_EQ(std::nullopt, map.GetFieldById(0));
362363
auto result = map.GetFieldByIndex(2);
363-
ASSERT_EQ(result.error().kind, iceberg::ErrorKind::kInvalidArgument);
364-
ASSERT_THAT(result.error().message,
365-
::testing::HasSubstr("Invalid index 2 to get field from map"));
364+
ASSERT_THAT(result, IsError(iceberg::ErrorKind::kInvalidArgument));
365+
ASSERT_THAT(result,
366+
iceberg::HasErrorMessage("Invalid index 2 to get field from map"));
366367
result = map.GetFieldByIndex(-1);
367-
ASSERT_EQ(result.error().kind, iceberg::ErrorKind::kInvalidArgument);
368-
ASSERT_THAT(result.error().message,
369-
::testing::HasSubstr("Invalid index -1 to get field from map"));
368+
ASSERT_THAT(result, IsError(iceberg::ErrorKind::kInvalidArgument));
369+
ASSERT_THAT(result,
370+
iceberg::HasErrorMessage("Invalid index -1 to get field from map"));
370371
ASSERT_EQ(std::nullopt, map.GetFieldByName("element"));
371372
}
372373
ASSERT_THAT(
@@ -405,13 +406,13 @@ TEST(TypeTest, Struct) {
405406

406407
ASSERT_EQ(std::nullopt, struct_.GetFieldById(0));
407408
auto result = struct_.GetFieldByIndex(2);
408-
ASSERT_EQ(result.error().kind, iceberg::ErrorKind::kInvalidArgument);
409-
ASSERT_THAT(result.error().message,
410-
::testing::HasSubstr("Invalid index 2 to get field from struct"));
409+
ASSERT_THAT(result, IsError(iceberg::ErrorKind::kInvalidArgument));
410+
ASSERT_THAT(result,
411+
iceberg::HasErrorMessage("Invalid index 2 to get field from struct"));
411412
result = struct_.GetFieldByIndex(-1);
412-
ASSERT_EQ(result.error().kind, iceberg::ErrorKind::kInvalidArgument);
413-
ASSERT_THAT(result.error().message,
414-
::testing::HasSubstr("Invalid index -1 to get field from struct"));
413+
ASSERT_THAT(result, IsError(iceberg::ErrorKind::kInvalidArgument));
414+
ASSERT_THAT(result,
415+
iceberg::HasErrorMessage("Invalid index -1 to get field from struct"));
415416
ASSERT_EQ(std::nullopt, struct_.GetFieldByName("element"));
416417
}
417418
}
@@ -480,9 +481,9 @@ TEST(TypeTest, StructDuplicateId) {
480481

481482
auto result = struct_.GetFieldById(5);
482483
ASSERT_FALSE(result.has_value());
483-
ASSERT_EQ(result.error().kind, iceberg::ErrorKind::kInvalidSchema);
484-
ASSERT_THAT(result.error().message,
485-
::testing::HasSubstr(
484+
ASSERT_THAT(result, IsError(iceberg::ErrorKind::kInvalidSchema));
485+
ASSERT_THAT(result,
486+
iceberg::HasErrorMessage(
486487
"Duplicate field id found: 5 (prev name: foo, curr name: bar)"));
487488
}
488489

@@ -493,10 +494,9 @@ TEST(TypeTest, StructDuplicateName) {
493494

494495
auto result = struct_.GetFieldByName("foo", true);
495496
ASSERT_FALSE(result.has_value());
496-
ASSERT_EQ(result.error().kind, iceberg::ErrorKind::kInvalidSchema);
497-
ASSERT_THAT(
498-
result.error().message,
499-
::testing::HasSubstr("Duplicate field name found: foo (prev id: 1, curr id: 2)"));
497+
ASSERT_THAT(result, IsError(iceberg::ErrorKind::kInvalidSchema));
498+
ASSERT_THAT(result, iceberg::HasErrorMessage(
499+
"Duplicate field name found: foo (prev id: 1, curr id: 2)"));
500500
}
501501

502502
TEST(TypeTest, StructDuplicateLowerCaseName) {
@@ -506,8 +506,8 @@ TEST(TypeTest, StructDuplicateLowerCaseName) {
506506

507507
auto result = struct_.GetFieldByName("foo", false);
508508
ASSERT_FALSE(result.has_value());
509-
ASSERT_EQ(result.error().kind, iceberg::ErrorKind::kInvalidSchema);
510-
ASSERT_THAT(
511-
result.error().message,
512-
::testing::HasSubstr("Duplicate field name found: foo (prev id: 1, curr id: 2)"));
509+
ASSERT_THAT(result, IsError(iceberg::ErrorKind::kInvalidSchema));
510+
ASSERT_THAT(result,
511+
iceberg::HasErrorMessage(
512+
"Duplicate lowercase field name found: foo (prev id: 1, curr id: 2)"));
513513
}

0 commit comments

Comments
 (0)