wgtmac commented on code in PR #194:
URL: https://github.com/apache/iceberg-cpp/pull/194#discussion_r2306094329
##########
src/iceberg/type.cc:
##########
@@ -105,23 +149,28 @@ std::string ListType::ToString() const {
return repr;
}
std::span<const SchemaField> ListType::fields() const { return {&element_, 1};
}
-std::optional<std::reference_wrapper<const SchemaField>>
ListType::GetFieldById(
+Result<std::optional<SchemaFieldConstRef>> ListType::GetFieldById(
int32_t field_id) const {
if (field_id == element_.field_id()) {
return std::cref(element_);
}
return std::nullopt;
}
-std::optional<std::reference_wrapper<const SchemaField>>
ListType::GetFieldByIndex(
- int index) const {
+Result<std::optional<SchemaFieldConstRef>> ListType::GetFieldByIndex(int
index) const {
if (index == 0) {
return std::cref(element_);
}
- return std::nullopt;
+ return InvalidArgument("index {} is out of range[0, {})", index, 1);
Review Comment:
```suggestion
return InvalidArgument("Invalid index {} to get field from list", index);
```
##########
src/iceberg/type.cc:
##########
@@ -159,29 +208,35 @@ std::string MapType::ToString() const {
return repr;
}
std::span<const SchemaField> MapType::fields() const { return fields_; }
-std::optional<std::reference_wrapper<const SchemaField>> MapType::GetFieldById(
- int32_t field_id) const {
+Result<std::optional<SchemaFieldConstRef>> MapType::GetFieldById(int32_t
field_id) const {
if (field_id == key().field_id()) {
return key();
} else if (field_id == value().field_id()) {
return value();
}
return std::nullopt;
}
-std::optional<std::reference_wrapper<const SchemaField>>
MapType::GetFieldByIndex(
- int32_t index) const {
+Result<std::optional<SchemaFieldConstRef>> MapType::GetFieldByIndex(int32_t
index) const {
if (index == 0) {
return key();
} else if (index == 1) {
return value();
}
- return std::nullopt;
+ return InvalidArgument("index {} is out of range[0, {})", index, 2);
Review Comment:
```suggestion
return InvalidArgument("Invalid index {} to get field from map", index);
```
##########
src/iceberg/type.cc:
##########
@@ -159,29 +208,35 @@ std::string MapType::ToString() const {
return repr;
}
std::span<const SchemaField> MapType::fields() const { return fields_; }
-std::optional<std::reference_wrapper<const SchemaField>> MapType::GetFieldById(
- int32_t field_id) const {
+Result<std::optional<SchemaFieldConstRef>> MapType::GetFieldById(int32_t
field_id) const {
if (field_id == key().field_id()) {
return key();
} else if (field_id == value().field_id()) {
return value();
}
return std::nullopt;
}
-std::optional<std::reference_wrapper<const SchemaField>>
MapType::GetFieldByIndex(
- int32_t index) const {
+Result<std::optional<SchemaFieldConstRef>> MapType::GetFieldByIndex(int32_t
index) const {
if (index == 0) {
return key();
} else if (index == 1) {
return value();
}
- return std::nullopt;
+ return InvalidArgument("index {} is out of range[0, {})", index, 2);
}
-std::optional<std::reference_wrapper<const SchemaField>>
MapType::GetFieldByName(
- std::string_view name) const {
- if (name == kKeyName) {
+Result<std::optional<SchemaFieldConstRef>> MapType::GetFieldByName(
+ std::string_view name, bool case_sensitive) const {
+ if (case_sensitive) {
+ if (name == kKeyName) {
+ return key();
+ } else if (name == kValueName) {
+ return value();
+ }
+ return std::nullopt;
+ }
+ if (StringUtils::ToLower(name) == kKeyName) {
Review Comment:
```suggestion
const auto lower_case_name = StringUtils::ToLower(name);
if (lower_case_name == kKeyName) {
```
##########
src/iceberg/type.cc:
##########
@@ -159,29 +208,35 @@ std::string MapType::ToString() const {
return repr;
}
std::span<const SchemaField> MapType::fields() const { return fields_; }
-std::optional<std::reference_wrapper<const SchemaField>> MapType::GetFieldById(
- int32_t field_id) const {
+Result<std::optional<SchemaFieldConstRef>> MapType::GetFieldById(int32_t
field_id) const {
if (field_id == key().field_id()) {
return key();
} else if (field_id == value().field_id()) {
return value();
}
return std::nullopt;
}
-std::optional<std::reference_wrapper<const SchemaField>>
MapType::GetFieldByIndex(
- int32_t index) const {
+Result<std::optional<SchemaFieldConstRef>> MapType::GetFieldByIndex(int32_t
index) const {
if (index == 0) {
return key();
} else if (index == 1) {
return value();
}
- return std::nullopt;
+ return InvalidArgument("index {} is out of range[0, {})", index, 2);
}
-std::optional<std::reference_wrapper<const SchemaField>>
MapType::GetFieldByName(
- std::string_view name) const {
- if (name == kKeyName) {
+Result<std::optional<SchemaFieldConstRef>> MapType::GetFieldByName(
+ std::string_view name, bool case_sensitive) const {
+ if (case_sensitive) {
+ if (name == kKeyName) {
+ return key();
+ } else if (name == kValueName) {
+ return value();
+ }
+ return std::nullopt;
+ }
+ if (StringUtils::ToLower(name) == kKeyName) {
return key();
- } else if (name == kValueName) {
+ } else if (StringUtils::ToLower(name) == kValueName) {
Review Comment:
```suggestion
} else if (lower_case_name == kValueName) {
```
##########
src/iceberg/type.h:
##########
@@ -67,6 +68,7 @@ class ICEBERG_EXPORT PrimitiveType : public Type {
bool is_nested() const override { return false; }
};
+using SchemaFieldConstRef = std::reference_wrapper<const SchemaField>;
Review Comment:
Can we put it inside the definition of `NestedType`?
##########
src/iceberg/type.cc:
##########
@@ -159,29 +208,35 @@ std::string MapType::ToString() const {
return repr;
}
std::span<const SchemaField> MapType::fields() const { return fields_; }
-std::optional<std::reference_wrapper<const SchemaField>> MapType::GetFieldById(
- int32_t field_id) const {
+Result<std::optional<SchemaFieldConstRef>> MapType::GetFieldById(int32_t
field_id) const {
if (field_id == key().field_id()) {
return key();
} else if (field_id == value().field_id()) {
return value();
}
return std::nullopt;
}
-std::optional<std::reference_wrapper<const SchemaField>>
MapType::GetFieldByIndex(
- int32_t index) const {
+Result<std::optional<SchemaFieldConstRef>> MapType::GetFieldByIndex(int32_t
index) const {
if (index == 0) {
return key();
} else if (index == 1) {
return value();
}
- return std::nullopt;
+ return InvalidArgument("index {} is out of range[0, {})", index, 2);
}
-std::optional<std::reference_wrapper<const SchemaField>>
MapType::GetFieldByName(
- std::string_view name) const {
- if (name == kKeyName) {
+Result<std::optional<SchemaFieldConstRef>> MapType::GetFieldByName(
+ std::string_view name, bool case_sensitive) const {
+ if (case_sensitive) {
+ if (name == kKeyName) {
+ return key();
+ } else if (name == kValueName) {
+ return value();
+ }
+ return std::nullopt;
+ }
+ if (StringUtils::ToLower(name) == kKeyName) {
Review Comment:
Let's avoid converting it twice in the worst case.
##########
src/iceberg/type.h:
##########
@@ -109,18 +114,26 @@ class ICEBERG_EXPORT StructType : public NestedType {
std::string ToString() const override;
std::span<const SchemaField> fields() const override;
- std::optional<std::reference_wrapper<const SchemaField>> GetFieldById(
+ Result<std::optional<SchemaFieldConstRef>> GetFieldById(
int32_t field_id) const override;
- std::optional<std::reference_wrapper<const SchemaField>> GetFieldByIndex(
+ Result<std::optional<SchemaFieldConstRef>> GetFieldByIndex(
int32_t index) const override;
- std::optional<std::reference_wrapper<const SchemaField>> GetFieldByName(
- std::string_view name) const override;
+ Result<std::optional<SchemaFieldConstRef>> GetFieldByName(
+ std::string_view name, bool case_sensitive) const override;
+
+ using NestedType::GetFieldByName;
protected:
bool Equals(const Type& other) const override;
+ Status InitFieldById() const;
+ Status InitFieldByName() const;
+ Status InitFieldByLowerCaseName() const;
+ protected:
std::vector<SchemaField> fields_;
- std::unordered_map<int32_t, size_t> field_id_to_index_;
+ mutable std::unordered_map<int32_t, SchemaFieldConstRef> field_by_id_;
Review Comment:
Add a TODO comment to make it thread-safe as well.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
