Skip to content

Commit 9ea3cf4

Browse files
zhjwpkuzhjwpku
committed
use: std::string
1 parent 403e53b commit 9ea3cf4

4 files changed

Lines changed: 38 additions & 62 deletions

File tree

‎src/iceberg/test/update_partition_spec_test.cc‎

Lines changed: 16 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -272,7 +272,7 @@ TEST_P(UpdatePartitionSpecTest, TestAddTruncate) {
272272

273273
TEST_P(UpdatePartitionSpecTest, TestAddNamedPartition) {
274274
auto update = CreateUpdatePartitionSpec(format_version_, unpartitioned_);
275-
update->AddField("shard", Expressions::Bucket("id", 16));
275+
update->AddField(Expressions::Bucket("id", 16), "shard");
276276
ASSERT_THAT(update->Apply(), IsOk());
277277

278278
ICEBERG_UNWRAP_OR_FAIL(auto updated_spec, update->GetAppliedSpec());
@@ -312,8 +312,8 @@ TEST_P(UpdatePartitionSpecTest, TestMultipleAdds) {
312312
auto update = CreateUpdatePartitionSpec(format_version_, unpartitioned_);
313313
update->AddField("category")
314314
.AddField(Expressions::Day("ts"))
315-
.AddField("shard", Expressions::Bucket("id", 16))
316-
.AddField("prefix", Expressions::Truncate("data", 4));
315+
.AddField(Expressions::Bucket("id", 16), "shard")
316+
.AddField(Expressions::Truncate("data", 4), "prefix");
317317
ASSERT_THAT(update->Apply(), IsOk());
318318

319319
ICEBERG_UNWRAP_OR_FAIL(auto updated_spec, update->GetAppliedSpec());
@@ -571,7 +571,7 @@ TEST_P(UpdatePartitionSpecTest, TestMultipleChanges) {
571571
auto update = CreateUpdatePartitionSpec(format_version_, partitioned_);
572572
update->RenameField("shard", "id_bucket")
573573
.RemoveField(Expressions::Day("ts"))
574-
.AddField("prefix", Expressions::Truncate("data", 4));
574+
.AddField(Expressions::Truncate("data", 4), "prefix");
575575
ASSERT_THAT(update->Apply(), IsOk());
576576

577577
ICEBERG_UNWRAP_OR_FAIL(auto updated_spec, update->GetAppliedSpec());
@@ -635,32 +635,32 @@ TEST_P(UpdatePartitionSpecTest, TestAddDeletedName) {
635635

636636
TEST_P(UpdatePartitionSpecTest, TestRemoveNewlyAddedFieldByName) {
637637
auto update = CreateUpdatePartitionSpec(format_version_, partitioned_);
638-
update->AddField("prefix", Expressions::Truncate("data", 4));
638+
update->AddField(Expressions::Truncate("data", 4), "prefix");
639639
update->RemoveField("prefix");
640640
EXPECT_THAT(update->Apply(), IsError(ErrorKind::kValidationFailed));
641641
EXPECT_THAT(update->Apply(), HasErrorMessage("Cannot delete newly added field"));
642642
}
643643

644644
TEST_P(UpdatePartitionSpecTest, TestRemoveNewlyAddedFieldByTransform) {
645645
auto update = CreateUpdatePartitionSpec(format_version_, partitioned_);
646-
update->AddField("prefix", Expressions::Truncate("data", 4));
646+
update->AddField(Expressions::Truncate("data", 4), "prefix");
647647
update->RemoveField(Expressions::Truncate("data", 4));
648648
EXPECT_THAT(update->Apply(), IsError(ErrorKind::kValidationFailed));
649649
EXPECT_THAT(update->Apply(), HasErrorMessage("Cannot delete newly added field"));
650650
}
651651

652652
TEST_P(UpdatePartitionSpecTest, TestAddAlreadyAddedFieldByTransform) {
653653
auto update = CreateUpdatePartitionSpec(format_version_, partitioned_);
654-
update->AddField("prefix", Expressions::Truncate("data", 4));
654+
update->AddField(Expressions::Truncate("data", 4), "prefix");
655655
update->AddField(Expressions::Truncate("data", 4));
656656
EXPECT_THAT(update->Apply(), IsError(ErrorKind::kValidationFailed));
657657
EXPECT_THAT(update->Apply(), HasErrorMessage("Cannot add duplicate partition field"));
658658
}
659659

660660
TEST_P(UpdatePartitionSpecTest, TestAddAlreadyAddedFieldByName) {
661661
auto update = CreateUpdatePartitionSpec(format_version_, partitioned_);
662-
update->AddField("prefix", Expressions::Truncate("data", 4));
663-
update->AddField("prefix", Expressions::Truncate("data", 6));
662+
update->AddField(Expressions::Truncate("data", 4), "prefix");
663+
update->AddField(Expressions::Truncate("data", 6), "prefix");
664664
EXPECT_THAT(update->Apply(), IsError(ErrorKind::kValidationFailed));
665665
EXPECT_THAT(update->Apply(), HasErrorMessage("Cannot add duplicate partition field"));
666666
}
@@ -684,7 +684,7 @@ TEST_P(UpdatePartitionSpecTest, TestAddRedundantTimePartition) {
684684
TEST_P(UpdatePartitionSpecTest, TestNoEffectAddDeletedSameFieldWithSameName) {
685685
auto update1 = CreateUpdatePartitionSpec(format_version_, partitioned_);
686686
update1->RemoveField("shard");
687-
update1->AddField("shard", Expressions::Bucket("id", 16));
687+
update1->AddField(Expressions::Bucket("id", 16), "shard");
688688
ASSERT_THAT(update1->Apply(), IsOk());
689689
ICEBERG_UNWRAP_OR_FAIL(auto spec1, update1->GetAppliedSpec());
690690
AssertPartitionSpecEquals(*partitioned_, *spec1);
@@ -700,7 +700,7 @@ TEST_P(UpdatePartitionSpecTest, TestNoEffectAddDeletedSameFieldWithSameName) {
700700
TEST_P(UpdatePartitionSpecTest, TestGenerateNewSpecAddDeletedSameFieldWithDifferentName) {
701701
auto update = CreateUpdatePartitionSpec(format_version_, partitioned_);
702702
update->RemoveField("shard");
703-
update->AddField("new_shard", Expressions::Bucket("id", 16));
703+
update->AddField(Expressions::Bucket("id", 16), "new_shard");
704704
ASSERT_THAT(update->Apply(), IsOk());
705705

706706
ICEBERG_UNWRAP_OR_FAIL(auto updated_spec, update->GetAppliedSpec());
@@ -738,7 +738,7 @@ TEST_P(UpdatePartitionSpecTest, TestAddDuplicateTransform) {
738738

739739
TEST_P(UpdatePartitionSpecTest, TestAddNamedDuplicate) {
740740
auto update = CreateUpdatePartitionSpec(format_version_, partitioned_);
741-
update->AddField("b16", Expressions::Bucket("id", 16));
741+
update->AddField(Expressions::Bucket("id", 16), "b16");
742742
EXPECT_THAT(update->Apply(), IsError(ErrorKind::kValidationFailed));
743743
EXPECT_THAT(update->Apply(), HasErrorMessage("Cannot add duplicate partition field"));
744744
}
@@ -767,7 +767,7 @@ TEST_P(UpdatePartitionSpecTest, TestRenameUnknownField) {
767767

768768
TEST_P(UpdatePartitionSpecTest, TestRenameAfterAdd) {
769769
auto update = CreateUpdatePartitionSpec(format_version_, partitioned_);
770-
update->AddField("data_trunc", Expressions::Truncate("data", 4));
770+
update->AddField(Expressions::Truncate("data", 4), "data_trunc");
771771
update->RenameField("data_trunc", "prefix");
772772
EXPECT_THAT(update->Apply(), IsError(ErrorKind::kValidationFailed));
773773
EXPECT_THAT(update->Apply(),
@@ -795,7 +795,7 @@ TEST_P(UpdatePartitionSpecTest, TestDeleteAndRename) {
795795
TEST_P(UpdatePartitionSpecTest, TestRemoveAndAddMultiTimes) {
796796
// Add first time
797797
auto update1 = CreateUpdatePartitionSpec(format_version_, unpartitioned_);
798-
update1->AddField("ts_date", Expressions::Day("ts"));
798+
update1->AddField(Expressions::Day("ts"), "ts_date");
799799
ASSERT_THAT(update1->Apply(), IsOk());
800800
ICEBERG_UNWRAP_OR_FAIL(auto add_first_time_spec, update1->GetAppliedSpec());
801801

@@ -813,7 +813,7 @@ TEST_P(UpdatePartitionSpecTest, TestRemoveAndAddMultiTimes) {
813813
auto catalog2 = std::make_shared<MockCatalog>();
814814
TableIdentifier identifier2{.ns = Namespace{.levels = {"test"}}, .name = "test_table"};
815815
auto update3 = std::make_unique<UpdatePartitionSpec>(identifier2, catalog2, metadata2);
816-
update3->AddField("ts_date", Expressions::Day("ts"));
816+
update3->AddField(Expressions::Day("ts"), "ts_date");
817817
ASSERT_THAT(update3->Apply(), IsOk());
818818
ICEBERG_UNWRAP_OR_FAIL(auto add_second_time_spec, update3->GetAppliedSpec());
819819

@@ -876,7 +876,7 @@ TEST_P(UpdatePartitionSpecTest, TestRemoveAndUpdateWithDifferentTransformation)
876876
TableIdentifier identifier{.ns = Namespace{.levels = {"test"}}, .name = "test_table"};
877877
auto update = std::make_unique<UpdatePartitionSpec>(identifier, catalog, metadata);
878878
update->RemoveField("ts_transformed");
879-
update->AddField("ts_transformed", Expressions::Day("ts"));
879+
update->AddField(Expressions::Day("ts"), "ts_transformed");
880880
ASSERT_THAT(update->Apply(), IsOk());
881881

882882
ICEBERG_UNWRAP_OR_FAIL(auto updated_spec, update->GetAppliedSpec());

‎src/iceberg/type_fwd.h‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -128,6 +128,8 @@ class BoundReference;
128128
class BoundTransform;
129129
template <typename B>
130130
class UnboundTerm;
131+
class NamedReference;
132+
class UnboundTransform;
131133

132134
class DataTableScan;
133135
class FileScanTask;

‎src/iceberg/update/update_partition_spec.cc‎

Lines changed: 8 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -113,18 +113,8 @@ UpdatePartitionSpec& UpdatePartitionSpec::AddField(const std::string& source_nam
113113
return AddFieldInternal(nullptr, source_id, Transform::Identity());
114114
}
115115

116-
UpdatePartitionSpec& UpdatePartitionSpec::AddField(
117-
std::shared_ptr<UnboundTerm<BoundReference>> term) {
118-
return AddField(std::nullopt, std::move(term));
119-
}
120-
121-
UpdatePartitionSpec& UpdatePartitionSpec::AddField(
122-
std::shared_ptr<UnboundTerm<BoundTransform>> term) {
123-
return AddField(std::nullopt, std::move(term));
124-
}
125-
126-
UpdatePartitionSpec& UpdatePartitionSpec::AddField(
127-
std::optional<std::string> name, std::shared_ptr<UnboundTerm<BoundReference>> term) {
116+
UpdatePartitionSpec& UpdatePartitionSpec::AddField(std::shared_ptr<NamedReference> term,
117+
std::string part_name) {
128118
// Bind the term to get the source field
129119
auto bound_result = term->Bind(*schema_, case_sensitive_);
130120
if (!bound_result.has_value()) {
@@ -137,12 +127,12 @@ UpdatePartitionSpec& UpdatePartitionSpec::AddField(
137127
int32_t source_id = bound_ref->field().field_id();
138128

139129
// Reference terms use identity transform
140-
return AddFieldInternal(name ? &name.value() : nullptr, source_id,
130+
return AddFieldInternal(part_name.empty() ? nullptr : &part_name, source_id,
141131
Transform::Identity());
142132
}
143133

144-
UpdatePartitionSpec& UpdatePartitionSpec::AddField(
145-
std::optional<std::string> name, std::shared_ptr<UnboundTerm<BoundTransform>> term) {
134+
UpdatePartitionSpec& UpdatePartitionSpec::AddField(std::shared_ptr<UnboundTransform> term,
135+
std::string part_name) {
146136
// Bind the term to get the source field and transform
147137
auto bound_result = term->Bind(*schema_, case_sensitive_);
148138
if (!bound_result.has_value()) {
@@ -155,7 +145,7 @@ UpdatePartitionSpec& UpdatePartitionSpec::AddField(
155145
int32_t source_id = bound_transform->reference()->field().field_id();
156146
auto transform = bound_transform->transform();
157147

158-
return AddFieldInternal(name ? &name.value() : nullptr, source_id, transform);
148+
return AddFieldInternal(part_name.empty() ? nullptr : &part_name, source_id, transform);
159149
}
160150

161151
UpdatePartitionSpec& UpdatePartitionSpec::AddFieldInternal(
@@ -290,7 +280,7 @@ UpdatePartitionSpec& UpdatePartitionSpec::RemoveField(const std::string& name) {
290280
}
291281

292282
UpdatePartitionSpec& UpdatePartitionSpec::RemoveField(
293-
std::shared_ptr<UnboundTerm<BoundReference>> term) {
283+
std::shared_ptr<NamedReference> term) {
294284
// Bind the term to get the source field
295285
auto bound_result = term->Bind(*schema_, case_sensitive_);
296286
if (!bound_result.has_value()) {
@@ -308,7 +298,7 @@ UpdatePartitionSpec& UpdatePartitionSpec::RemoveField(
308298
}
309299

310300
UpdatePartitionSpec& UpdatePartitionSpec::RemoveField(
311-
std::shared_ptr<UnboundTerm<BoundTransform>> term) {
301+
std::shared_ptr<UnboundTransform> term) {
312302
// Bind the term to get the source field and transform
313303
auto bound_result = term->Bind(*schema_, case_sensitive_);
314304
if (!bound_result.has_value()) {

‎src/iceberg/update/update_partition_spec.h‎

Lines changed: 12 additions & 28 deletions
Original file line numberDiff line numberDiff line change
@@ -23,7 +23,6 @@
2323
/// API for partition spec evolution.
2424

2525
#include <memory>
26-
#include <optional>
2726
#include <string>
2827
#include <unordered_map>
2928
#include <unordered_set>
@@ -65,36 +64,21 @@ class ICEBERG_EXPORT UpdatePartitionSpec : public PendingUpdate {
6564
/// \return Reference to this for method chaining.
6665
UpdatePartitionSpec& AddField(const std::string& source_name);
6766

68-
/// \brief Add a new partition field from an unbound term.
69-
///
70-
/// The partition field will use the term's transform or the identity transform if
71-
/// the term is a reference.
72-
///
73-
/// \param term The unbound term representing the partition transform.
74-
/// \return Reference to this for method chaining.
75-
UpdatePartitionSpec& AddField(std::shared_ptr<UnboundTerm<BoundReference>> term);
76-
77-
/// \brief Add a new partition field from an unbound transform term.
78-
///
79-
/// \param term The unbound transform term.
80-
/// \return Reference to this for method chaining.
81-
UpdatePartitionSpec& AddField(std::shared_ptr<UnboundTerm<BoundTransform>> term);
82-
8367
/// \brief Add a new partition field with a custom name.
8468
///
85-
/// \param name Name for the partition field.
86-
/// \param term The unbound term representing the partition transform.
69+
/// \param term The named reference representing the source column.
70+
/// \param part_name Name for the partition field.
8771
/// \return Reference to this for method chaining.
88-
UpdatePartitionSpec& AddField(std::optional<std::string> name,
89-
std::shared_ptr<UnboundTerm<BoundReference>> term);
72+
UpdatePartitionSpec& AddField(std::shared_ptr<NamedReference> term,
73+
std::string part_name = "");
9074

9175
/// \brief Add a new partition field with a custom name from an unbound transform.
9276
///
93-
/// \param name Name for the partition field.
94-
/// \param term The unbound transform term.
77+
/// \param term The unbound transform representing the partition transform.
78+
/// \param part_name Name for the partition field.
9579
/// \return Reference to this for method chaining.
96-
UpdatePartitionSpec& AddField(std::optional<std::string> name,
97-
std::shared_ptr<UnboundTerm<BoundTransform>> term);
80+
UpdatePartitionSpec& AddField(std::shared_ptr<UnboundTransform> term,
81+
std::string part_name = "");
9882

9983
/// \brief Remove a partition field by name.
10084
///
@@ -108,17 +92,17 @@ class ICEBERG_EXPORT UpdatePartitionSpec : public PendingUpdate {
10892
/// If the term is a reference and does not have a transform, the identity transform
10993
/// is used.
11094
///
111-
/// \param term The unbound term representing the partition transform to remove.
95+
/// \param term The named reference representing the partition transform to remove.
11296
/// \return Reference to this for method chaining.
113-
UpdatePartitionSpec& RemoveField(std::shared_ptr<UnboundTerm<BoundReference>> term);
97+
UpdatePartitionSpec& RemoveField(std::shared_ptr<NamedReference> term);
11498

11599
/// \brief Remove a partition field by its transform term.
116100
///
117101
/// The partition field with the same transform and source reference will be removed.
118102
///
119-
/// \param term The unbound transform term.
103+
/// \param term The unbound transform representing the partition transform to remove.
120104
/// \return Reference to this for method chaining.
121-
UpdatePartitionSpec& RemoveField(std::shared_ptr<UnboundTerm<BoundTransform>> term);
105+
UpdatePartitionSpec& RemoveField(std::shared_ptr<UnboundTransform> term);
122106

123107
/// \brief Rename a field in the partition spec.
124108
///

0 commit comments

Comments
 (0)