wgtmac commented on code in PR #517:
URL: https://github.com/apache/iceberg-cpp/pull/517#discussion_r2704059482
##########
src/iceberg/update/update_schema.cc:
##########
@@ -629,4 +726,55 @@ std::string
UpdateSchema::CaseSensitivityAwareName(std::string_view name) const
return StringUtils::ToLower(name);
}
+std::optional<int32_t> UpdateSchema::FindFieldIdForMove(std::string_view name)
const {
+ // First check if it's a newly added field
Review Comment:
```suggestion
```
##########
src/iceberg/update/update_schema.cc:
##########
@@ -629,4 +726,55 @@ std::string
UpdateSchema::CaseSensitivityAwareName(std::string_view name) const
return StringUtils::ToLower(name);
}
+std::optional<int32_t> UpdateSchema::FindFieldIdForMove(std::string_view name)
const {
+ // First check if it's a newly added field
+ auto added_it = added_name_to_id_.find(CaseSensitivityAwareName(name));
+ if (added_it != added_name_to_id_.end()) {
+ return added_it->second;
+ }
+
+ // Then check existing schema fields
+ auto field_result = FindField(name);
Review Comment:
How about preserving and returning the error from `FindField`?
##########
src/iceberg/update/update_schema.cc:
##########
@@ -205,11 +215,64 @@ class ApplyChangesVisitor {
}
}
+ /// \brief Helper function to apply move operations to a list of fields
+ static std::vector<SchemaField> MoveFields(
+ std::vector<SchemaField>&& fields, const
std::vector<UpdateSchema::Move>& moves);
+
const std::unordered_set<int32_t>& deletes_;
const std::unordered_map<int32_t, std::shared_ptr<SchemaField>>& updates_;
const std::unordered_map<int32_t, std::vector<int32_t>>&
parent_to_added_ids_;
+ const std::unordered_map<int32_t, std::vector<UpdateSchema::Move>>& moves_;
};
+std::vector<SchemaField> ApplyChangesVisitor::MoveFields(
+ std::vector<SchemaField>&& fields, const std::vector<UpdateSchema::Move>&
moves) {
+ std::vector<SchemaField> reordered = std::move(fields);
+
+ for (const auto& move : moves) {
+ auto it = std::ranges::find_if(reordered, [&move](const SchemaField&
field) {
+ return field.field_id() == move.field_id;
+ });
+
+ if (it == reordered.end()) {
+ continue;
+ }
+
+ SchemaField to_move = *it;
+ reordered.erase(it);
+
+ switch (move.type) {
+ case UpdateSchema::Move::MoveType::kFirst:
+ reordered.insert(reordered.begin(), to_move);
Review Comment:
```suggestion
reordered.insert(reordered.begin(), std::move(to_move));
```
Same for below.
##########
src/iceberg/update/update_schema.cc:
##########
@@ -629,4 +726,55 @@ std::string
UpdateSchema::CaseSensitivityAwareName(std::string_view name) const
return StringUtils::ToLower(name);
}
+std::optional<int32_t> UpdateSchema::FindFieldIdForMove(std::string_view name)
const {
+ // First check if it's a newly added field
+ auto added_it = added_name_to_id_.find(CaseSensitivityAwareName(name));
+ if (added_it != added_name_to_id_.end()) {
+ return added_it->second;
+ }
+
+ // Then check existing schema fields
Review Comment:
```suggestion
```
##########
src/iceberg/update/update_schema.cc:
##########
@@ -421,23 +503,38 @@ UpdateSchema& UpdateSchema::DeleteColumn(std::string_view
name) {
}
UpdateSchema& UpdateSchema::MoveFirst(std::string_view name) {
- // TODO(Guotao Yu): Implement MoveFirst
- AddError(NotImplemented("UpdateSchema::MoveFirst not implemented"));
- return *this;
+ auto field_id = FindFieldIdForMove(name);
+ ICEBERG_BUILDER_CHECK(field_id.has_value(), "Cannot move missing column:
{}", name);
+
+ return MoveInternal(name, Move::First(*field_id));
}
UpdateSchema& UpdateSchema::MoveBefore(std::string_view name,
std::string_view before_name) {
- // TODO(Guotao Yu): Implement MoveBefore
- AddError(NotImplemented("UpdateSchema::MoveBefore not implemented"));
- return *this;
+ auto field_id = FindFieldIdForMove(name);
Review Comment:
nit: we can directly return `Result<int32_t>` to get rid of std::optional
since we expect the field to move must exist.
--
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]
