wgtmac commented on code in PR #194:
URL: https://github.com/apache/iceberg-cpp/pull/194#discussion_r2303260634
##########
src/iceberg/type.cc:
##########
@@ -25,23 +25,18 @@
#include "iceberg/exception.h"
#include "iceberg/util/formatter.h" // IWYU pragma: keep
+#include "iceberg/util/macros.h"
+#include "iceberg/util/string_utils.h"
namespace iceberg {
-StructType::StructType(std::vector<SchemaField> fields) :
fields_(std::move(fields)) {
- size_t index = 0;
- for (const auto& field : fields_) {
- auto [it, inserted] = field_id_to_index_.try_emplace(field.field_id(),
index);
- if (!inserted) {
- throw IcebergError(
- std::format("StructType: duplicate field ID {} (field indices {} and
{})",
- field.field_id(), it->second, index));
- }
-
- ++index;
- }
+Result<std::optional<std::reference_wrapper<const SchemaField>>>
+NestedType::GetFieldByName(std::string_view name) const {
+ return GetFieldByName(name, true);
Review Comment:
```suggestion
return GetFieldByName(name, /*case_sensitive=*/true);
```
##########
src/iceberg/type.h:
##########
@@ -78,20 +79,23 @@ class ICEBERG_EXPORT NestedType : public Type {
/// \brief Get a field by field ID.
///
/// \note This is O(1) complexity.
- [[nodiscard]] virtual std::optional<std::reference_wrapper<const
SchemaField>>
+ [[nodiscard]] virtual Result<std::optional<std::reference_wrapper<const
SchemaField>>>
Review Comment:
nit: `using SchemaFieldConstRef = std::reference_wrapper<const
SchemaField>;` to make it shorter and replace all return types below?
##########
src/iceberg/type.cc:
##########
@@ -53,27 +48,34 @@ std::string StructType::ToString() const {
return repr;
}
std::span<const SchemaField> StructType::fields() const { return fields_; }
-std::optional<std::reference_wrapper<const SchemaField>>
StructType::GetFieldById(
+Result<std::optional<std::reference_wrapper<const SchemaField>>>
StructType::GetFieldById(
int32_t field_id) const {
- auto it = field_id_to_index_.find(field_id);
- if (it == field_id_to_index_.end()) return std::nullopt;
- return fields_[it->second];
+ ICEBERG_RETURN_UNEXPECTED(InitFieldById());
+ auto it = field_by_id_.find(field_id);
+ if (it == field_by_id_.end()) return std::nullopt;
+ return it->second;
}
-std::optional<std::reference_wrapper<const SchemaField>>
StructType::GetFieldByIndex(
- int32_t index) const {
- if (index < 0 || index >= static_cast<int32_t>(fields_.size())) {
+Result<std::optional<std::reference_wrapper<const SchemaField>>>
+StructType::GetFieldByIndex(int32_t index) const {
+ if (index < 0 || static_cast<size_t>(index) >= fields_.size()) {
Review Comment:
Perhaps we should return a `InvalidArgument` error now?
##########
src/iceberg/type.cc:
##########
@@ -84,6 +86,48 @@ bool StructType::Equals(const Type& other) const {
const auto& struct_ = static_cast<const StructType&>(other);
return fields_ == struct_.fields_;
}
+Status StructType::InitFieldById() const {
+ if (!field_by_id_.empty()) {
+ return {};
+ }
+ for (const auto& field : fields_) {
+ auto it = field_by_id_.try_emplace(field.field_id(), field);
+ if (!it.second) {
+ return NotAllowed("Duplicate field id found: {} (prev name: {}, curr
name: {})",
+ field.field_id(), it.first->second.get().name(),
field.name());
+ }
+ }
+ return {};
+}
+Status StructType::InitFieldByName() const {
+ if (!field_by_name_.empty()) {
+ return {};
+ }
+ for (const auto& field : fields_) {
+ auto it = field_by_name_.try_emplace(field.name(), field);
+ if (!it.second) {
+ return NotAllowed("Duplicate field name found: {} (prev id: {}, curr id:
{})",
+ it.first->first, it.first->second.get().field_id(),
+ field.field_id());
+ }
+ }
+ return {};
+}
+Status StructType::InitFieldByLowerCaseName() const {
+ if (!field_by_lowercase_name_.empty()) {
+ return {};
+ }
+ for (const auto& field : fields_) {
+ auto it =
+
field_by_lowercase_name_.try_emplace(StringUtils::ToLower(field.name()), field);
+ if (!it.second) {
+ return NotAllowed("Duplicate field name found: {} (prev id: {}, curr id:
{})",
Review Comment:
```suggestion
return InvalidSchema("Duplicate field name found: {} (prev id: {},
curr id: {})",
```
##########
src/iceberg/type.cc:
##########
@@ -84,6 +86,48 @@ bool StructType::Equals(const Type& other) const {
const auto& struct_ = static_cast<const StructType&>(other);
return fields_ == struct_.fields_;
}
+Status StructType::InitFieldById() const {
+ if (!field_by_id_.empty()) {
+ return {};
+ }
+ for (const auto& field : fields_) {
+ auto it = field_by_id_.try_emplace(field.field_id(), field);
+ if (!it.second) {
+ return NotAllowed("Duplicate field id found: {} (prev name: {}, curr
name: {})",
+ field.field_id(), it.first->second.get().name(),
field.name());
+ }
+ }
+ return {};
+}
+Status StructType::InitFieldByName() const {
+ if (!field_by_name_.empty()) {
+ return {};
+ }
+ for (const auto& field : fields_) {
+ auto it = field_by_name_.try_emplace(field.name(), field);
+ if (!it.second) {
+ return NotAllowed("Duplicate field name found: {} (prev id: {}, curr id:
{})",
Review Comment:
```suggestion
return InvalidSchema("Duplicate field name found: {} (prev id: {},
curr id: {})",
```
##########
test/schema_test.cc:
##########
@@ -51,14 +51,6 @@ TEST(SchemaTest, Basics) {
ASSERT_EQ(std::nullopt, schema.GetFieldByIndex(-1));
ASSERT_EQ(std::nullopt, schema.GetFieldByName("element"));
}
- ASSERT_THAT(
- []() {
- iceberg::SchemaField field1(5, "foo", iceberg::int32(), true);
- iceberg::SchemaField field2(5, "bar", iceberg::string(), true);
- iceberg::Schema schema({field1, field2}, 100);
- },
- ::testing::ThrowsMessage<std::runtime_error>(
Review Comment:
Why this gets removed?
##########
src/iceberg/type.cc:
##########
@@ -177,11 +227,19 @@ std::optional<std::reference_wrapper<const SchemaField>>
MapType::GetFieldByInde
}
return std::nullopt;
}
-std::optional<std::reference_wrapper<const SchemaField>>
MapType::GetFieldByName(
- std::string_view name) const {
- if (name == kKeyName) {
+Result<std::optional<std::reference_wrapper<const SchemaField>>>
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) == StringUtils::ToLower(kKeyName)) {
Review Comment:
```suggestion
if (StringUtils::ToLower(name) == kKeyName) {
```
##########
src/iceberg/type.cc:
##########
@@ -84,6 +86,48 @@ bool StructType::Equals(const Type& other) const {
const auto& struct_ = static_cast<const StructType&>(other);
return fields_ == struct_.fields_;
}
+Status StructType::InitFieldById() const {
+ if (!field_by_id_.empty()) {
+ return {};
+ }
+ for (const auto& field : fields_) {
+ auto it = field_by_id_.try_emplace(field.field_id(), field);
+ if (!it.second) {
+ return NotAllowed("Duplicate field id found: {} (prev name: {}, curr
name: {})",
Review Comment:
```suggestion
return InvalidSchema("Duplicate field id found: {} (prev name: {},
curr name: {})",
```
##########
src/iceberg/type.cc:
##########
@@ -105,23 +149,29 @@ 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<std::reference_wrapper<const SchemaField>>>
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<std::reference_wrapper<const SchemaField>>>
+ListType::GetFieldByIndex(int index) const {
if (index == 0) {
return std::cref(element_);
}
return std::nullopt;
Review Comment:
Return `InvalidArgument`?
--
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: issues-unsubscr...@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscr...@iceberg.apache.org
For additional commands, e-mail: issues-h...@iceberg.apache.org
