Skip to content

Commit 647ada2

Browse files
HeartLinkedHeartLinked
committed
1
1 parent d08ac85 commit 647ada2

6 files changed

Lines changed: 63 additions & 79 deletions

File tree

‎src/iceberg/meson.build‎

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -197,8 +197,6 @@ install_headers(
197197
'transform.h',
198198
'type_fwd.h',
199199
'type.h',
200-
'update/replace_sort_order.h',
201-
'update/update_properties.h',
202200
],
203201
subdir: 'iceberg',
204202
)
@@ -207,6 +205,7 @@ subdir('catalog')
207205
subdir('expression')
208206
subdir('manifest')
209207
subdir('row')
208+
subdir('update')
210209
subdir('util')
211210

212211
if get_option('tests').enabled()

‎src/iceberg/test/replace_sort_order_test.cc‎

Lines changed: 13 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -73,7 +73,7 @@ TEST_F(ReplaceSortOrderTest, AscendingSingleField) {
7373
auto ref = NamedReference::Make("id").value();
7474
auto term = UnboundTransform::Make(std::move(ref), Transform::Identity()).value();
7575

76-
update.Asc(std::move(term), NullOrder::kFirst);
76+
update.AddSortField(std::move(term), SortDirection::kAscending, NullOrder::kFirst);
7777

7878
auto result = update.Apply();
7979
EXPECT_THAT(result, IsOk());
@@ -94,7 +94,7 @@ TEST_F(ReplaceSortOrderTest, DescendingSingleField) {
9494
auto ref = NamedReference::Make("name").value();
9595
auto term = UnboundTransform::Make(std::move(ref), Transform::Identity()).value();
9696

97-
update.Desc(std::move(term), NullOrder::kLast);
97+
update.AddSortField(std::move(term), SortDirection::kDescending, NullOrder::kLast);
9898

9999
auto result = update.Apply();
100100
EXPECT_THAT(result, IsOk());
@@ -118,8 +118,8 @@ TEST_F(ReplaceSortOrderTest, MultipleFields) {
118118
auto ref2 = NamedReference::Make("id").value();
119119
auto term2 = UnboundTransform::Make(std::move(ref2), Transform::Identity()).value();
120120

121-
update.Asc(std::move(term1), NullOrder::kFirst)
122-
.Desc(std::move(term2), NullOrder::kLast);
121+
update.AddSortField(std::move(term1), SortDirection::kAscending, NullOrder::kFirst)
122+
.AddSortField(std::move(term2), SortDirection::kDescending, NullOrder::kLast);
123123

124124
auto result = update.Apply();
125125
EXPECT_THAT(result, IsOk());
@@ -147,7 +147,7 @@ TEST_F(ReplaceSortOrderTest, WithTransform) {
147147
auto ref = NamedReference::Make("ts").value();
148148
auto term = UnboundTransform::Make(std::move(ref), Transform::Day()).value();
149149

150-
update.Asc(std::move(term), NullOrder::kFirst);
150+
update.AddSortField(std::move(term), SortDirection::kAscending, NullOrder::kFirst);
151151

152152
auto result = update.Apply();
153153
EXPECT_THAT(result, IsOk());
@@ -169,7 +169,7 @@ TEST_F(ReplaceSortOrderTest, WithBucketTransform) {
169169
auto ref = NamedReference::Make("name").value();
170170
auto term = UnboundTransform::Make(std::move(ref), Transform::Bucket(10)).value();
171171

172-
update.Desc(std::move(term), NullOrder::kLast);
172+
update.AddSortField(std::move(term), SortDirection::kDescending, NullOrder::kLast);
173173

174174
auto result = update.Apply();
175175
EXPECT_THAT(result, IsOk());
@@ -188,7 +188,7 @@ TEST_F(ReplaceSortOrderTest, WithBucketTransform) {
188188
TEST_F(ReplaceSortOrderTest, NullTerm) {
189189
ReplaceSortOrder update(identifier_, catalog_, metadata_);
190190

191-
update.Asc(nullptr, NullOrder::kFirst);
191+
update.AddSortField(nullptr, SortDirection::kAscending, NullOrder::kFirst);
192192

193193
auto result = update.Apply();
194194
EXPECT_THAT(result, IsError(ErrorKind::kValidationFailed));
@@ -202,7 +202,7 @@ TEST_F(ReplaceSortOrderTest, InvalidTransformForType) {
202202
auto ref = NamedReference::Make("name").value();
203203
auto term = UnboundTransform::Make(std::move(ref), Transform::Day()).value();
204204

205-
update.Asc(std::move(term), NullOrder::kFirst);
205+
update.AddSortField(std::move(term), SortDirection::kAscending, NullOrder::kFirst);
206206

207207
auto result = update.Apply();
208208
EXPECT_THAT(result, IsError(ErrorKind::kValidationFailed));
@@ -215,7 +215,7 @@ TEST_F(ReplaceSortOrderTest, NonExistentField) {
215215
auto ref = NamedReference::Make("nonexistent").value();
216216
auto term = UnboundTransform::Make(std::move(ref), Transform::Identity()).value();
217217

218-
update.Asc(std::move(term), NullOrder::kFirst);
218+
update.AddSortField(std::move(term), SortDirection::kAscending, NullOrder::kFirst);
219219

220220
auto result = update.Apply();
221221
EXPECT_THAT(result, IsError(ErrorKind::kValidationFailed));
@@ -228,7 +228,8 @@ TEST_F(ReplaceSortOrderTest, CaseSensitiveTrue) {
228228
auto ref = NamedReference::Make("ID").value(); // Uppercase
229229
auto term = UnboundTransform::Make(std::move(ref), Transform::Identity()).value();
230230

231-
update.CaseSensitive(true).Asc(std::move(term), NullOrder::kFirst);
231+
update.CaseSensitive(true).AddSortField(std::move(term), SortDirection::kAscending,
232+
NullOrder::kFirst);
232233

233234
auto result = update.Apply();
234235
// Should fail because schema has "id" (lowercase)
@@ -241,7 +242,8 @@ TEST_F(ReplaceSortOrderTest, CaseSensitiveFalse) {
241242
auto ref = NamedReference::Make("ID").value(); // Uppercase
242243
auto term = UnboundTransform::Make(std::move(ref), Transform::Identity()).value();
243244

244-
update.CaseSensitive(false).Asc(std::move(term), NullOrder::kFirst);
245+
update.CaseSensitive(false).AddSortField(std::move(term), SortDirection::kAscending,
246+
NullOrder::kFirst);
245247

246248
auto result = update.Apply();
247249
// Should succeed because case-insensitive matching

‎src/iceberg/type_fwd.h‎

Lines changed: 0 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -160,8 +160,6 @@ class TableMetadataBuilder;
160160
class TableUpdateContext;
161161

162162
class PendingUpdate;
163-
template <typename T>
164-
class PendingUpdateTyped;
165163
class UpdateProperties;
166164
class ReplaceSortOrder;
167165

‎src/iceberg/update/meson.build‎

Lines changed: 21 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,21 @@
1+
# Licensed to the Apache Software Foundation (ASF) under one
2+
# or more contributor license agreements. See the NOTICE file
3+
# distributed with this work for additional information
4+
# regarding copyright ownership. The ASF licenses this file
5+
# to you under the Apache License, Version 2.0 (the
6+
# "License"); you may not use this file except in compliance
7+
# with the License. You may obtain a copy of the License at
8+
#
9+
# http://www.apache.org/licenses/LICENSE-2.0
10+
#
11+
# Unless required by applicable law or agreed to in writing,
12+
# software distributed under the License is distributed on an
13+
# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
14+
# KIND, either express or implied. See the License for the
15+
# specific language governing permissions and limitations
16+
# under the License.
17+
18+
install_headers(
19+
['replace_sort_order.h', 'update_properties.h'],
20+
subdir: 'iceberg/update',
21+
)

‎src/iceberg/update/replace_sort_order.cc‎

Lines changed: 18 additions & 45 deletions
Original file line numberDiff line numberDiff line change
@@ -46,12 +46,13 @@ ReplaceSortOrder::ReplaceSortOrder(TableIdentifier identifier,
4646
catalog_(std::move(catalog)),
4747
base_metadata_(std::move(base)) {}
4848

49-
ReplaceSortOrder& ReplaceSortOrder::Asc(std::shared_ptr<Term> term,
50-
NullOrder null_order) {
49+
ReplaceSortOrder& ReplaceSortOrder::AddSortField(std::shared_ptr<Term> term,
50+
SortDirection direction,
51+
NullOrder null_order) {
5152
if (!term) {
5253
return AddError(ErrorKind::kInvalidArgument, "Term cannot be null");
5354
}
54-
if (term->kind() != BoundTerm::Kind::kTransform) {
55+
if (term->kind() != Term::Kind::kTransform) {
5556
return AddError(ErrorKind::kInvalidArgument, "Term must be a transform term");
5657
}
5758
if (!term->is_unbound()) {
@@ -60,65 +61,30 @@ ReplaceSortOrder& ReplaceSortOrder::Asc(std::shared_ptr<Term> term,
6061
// use checked-cast to get UnboundTransform
6162
auto unbound_transform = internal::checked_pointer_cast<UnboundTransform>(term);
6263

63-
BUILDER_ASSIGN_OR_RETURN(auto schema, base_metadata_->Schema());
64+
BUILDER_ASSIGN_OR_RETURN(auto schema, GetSchema());
6465
BUILDER_ASSIGN_OR_RETURN(auto bound_term,
6566
unbound_transform->Bind(*schema, case_sensitive_));
66-
return AddSortField(bound_term->reference(), unbound_transform->transform(),
67-
SortDirection::kAscending, null_order);
68-
}
6967

70-
ReplaceSortOrder& ReplaceSortOrder::Desc(std::shared_ptr<Term> term,
71-
NullOrder null_order) {
72-
if (!term) {
73-
return AddError(ErrorKind::kInvalidArgument, "Transform term cannot be null");
74-
}
75-
if (term->kind() != BoundTerm::Kind::kTransform) {
76-
return AddError(ErrorKind::kInvalidArgument, "Term must be a transform term");
77-
}
78-
if (!term->is_unbound()) {
79-
return AddError(ErrorKind::kInvalidArgument, "Term must be unbound");
80-
}
81-
// use checked-cast to get UnboundTransform
82-
auto unbound_transform = internal::checked_pointer_cast<UnboundTransform>(term);
83-
BUILDER_ASSIGN_OR_RETURN(auto schema, base_metadata_->Schema());
84-
BUILDER_ASSIGN_OR_RETURN(auto bound_term,
85-
unbound_transform->Bind(*schema, case_sensitive_));
86-
return AddSortField(bound_term->reference(), unbound_transform->transform(),
87-
SortDirection::kDescending, null_order);
68+
int32_t source_id = bound_term->reference()->field_id();
69+
sort_fields_.emplace_back(source_id, unbound_transform->transform(), direction,
70+
null_order);
71+
return *this;
8872
}
8973

9074
ReplaceSortOrder& ReplaceSortOrder::CaseSensitive(bool case_sensitive) {
9175
case_sensitive_ = case_sensitive;
9276
return *this;
9377
}
9478

95-
ReplaceSortOrder& ReplaceSortOrder::AddSortField(std::shared_ptr<BoundReference> ref,
96-
std::shared_ptr<Transform> transform,
97-
SortDirection direction,
98-
NullOrder null_order) {
99-
int32_t source_id = ref->field_id();
100-
const auto& field = ref->field();
101-
102-
// Validate that the transform can be applied to the field type 、
103-
if (!transform->CanTransform(*field.type())) {
104-
return AddError(
105-
ErrorKind::kInvalidArgument,
106-
std::format("Invalid transform {} for field '{}' with type {}",
107-
transform->ToString(), field.name(), field.type()->ToString()));
108-
}
109-
110-
sort_fields_.emplace_back(source_id, std::move(transform), direction, null_order);
111-
return *this;
112-
}
113-
11479
Status ReplaceSortOrder::Apply() {
11580
ICEBERG_RETURN_UNEXPECTED(CheckErrors());
11681
// Note: We use kInitialSortOrderId (1) here like the Java implementation.
11782
// The actual sort order ID will be assigned by TableMetadataBuilder when
11883
// the AddSortOrder update is applied.
11984
ICEBERG_ASSIGN_OR_RAISE(auto order,
12085
SortOrder::Make(SortOrder::kInitialSortOrderId, sort_fields_));
121-
ICEBERG_RETURN_UNEXPECTED(order->Validate(*base_metadata_->Schema().value()));
86+
ICEBERG_ASSIGN_OR_RAISE(auto schema, GetSchema());
87+
ICEBERG_RETURN_UNEXPECTED(order->Validate(*schema));
12288
built_sort_order_ = std::move(order);
12389
return {};
12490
}
@@ -147,4 +113,11 @@ std::shared_ptr<SortOrder> ReplaceSortOrder::GetBuiltSortOrder() const {
147113
return built_sort_order_;
148114
}
149115

116+
Result<std::shared_ptr<Schema>> ReplaceSortOrder::GetSchema() {
117+
if (!cached_schema_) {
118+
ICEBERG_ASSIGN_OR_RAISE(cached_schema_, base_metadata_->Schema());
119+
}
120+
return cached_schema_;
121+
}
122+
150123
} // namespace iceberg

‎src/iceberg/update/replace_sort_order.h‎

Lines changed: 10 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -20,6 +20,7 @@
2020
#pragma once
2121

2222
#include <memory>
23+
#include <optional>
2324
#include <vector>
2425

2526
#include "iceberg/expression/term.h"
@@ -43,19 +44,14 @@ class ICEBERG_EXPORT ReplaceSortOrder : public PendingUpdate {
4344
ReplaceSortOrder(TableIdentifier identifier, std::shared_ptr<Catalog> catalog,
4445
std::shared_ptr<TableMetadata> base);
4546

46-
/// \brief Add a field to the sort by field name, ascending with the given null order.
47-
///
48-
/// \param term A term referencing the field
49-
/// \param null_order The null order (first or last)
50-
/// \return Reference to this ReplaceSortOrder for chaining
51-
ReplaceSortOrder& Asc(std::shared_ptr<Term> term, NullOrder null_order);
52-
53-
/// \brief Add a field to the sort by field name, descending with the given null order.
47+
/// \brief Add a sort field to the sort order.
5448
///
5549
/// \param term A transform term referencing the field
50+
/// \param direction The sort direction (ascending or descending)
5651
/// \param null_order The null order (first or last)
5752
/// \return Reference to this ReplaceSortOrder for chaining
58-
ReplaceSortOrder& Desc(std::shared_ptr<Term> term, NullOrder null_order);
53+
ReplaceSortOrder& AddSortField(std::shared_ptr<Term> term, SortDirection direction,
54+
NullOrder null_order);
5955

6056
/// \brief Set case sensitivity of sort column name resolution.
6157
///
@@ -84,16 +80,8 @@ class ICEBERG_EXPORT ReplaceSortOrder : public PendingUpdate {
8480
std::shared_ptr<SortOrder> GetBuiltSortOrder() const;
8581

8682
private:
87-
/// \brief Helper to add a sort field after binding the term.
88-
///
89-
/// \param ref The bound reference to the field
90-
/// \param transform The transform to apply
91-
/// \param direction The sort direction
92-
/// \param null_order The null order
93-
/// \return Reference to this ReplaceSortOrder for chaining
94-
ReplaceSortOrder& AddSortField(std::shared_ptr<BoundReference> ref,
95-
std::shared_ptr<Transform> transform,
96-
SortDirection direction, NullOrder null_order);
83+
/// \brief Get the schema, loading and caching it on first access.
84+
Result<std::shared_ptr<Schema>> GetSchema();
9785

9886
TableIdentifier identifier_;
9987
std::shared_ptr<Catalog> catalog_;
@@ -102,6 +90,9 @@ class ICEBERG_EXPORT ReplaceSortOrder : public PendingUpdate {
10290
std::vector<SortField> sort_fields_;
10391
bool case_sensitive_ = true;
10492
std::shared_ptr<SortOrder> built_sort_order_;
93+
94+
// Cached schema to avoid repeated lookups
95+
std::shared_ptr<Schema> cached_schema_;
10596
};
10697

10798
} // namespace iceberg

0 commit comments

Comments
 (0)