Skip to content

Commit 37bc389

Browse files
lishuxulishuxu
authored
refactor: Use RestCatalogProperties by value instead of unique_ptr (#575)
1 parent 9826147 commit 37bc389

File tree

6 files changed

+65
-73
lines changed

6 files changed

+65
-73
lines changed

‎src/iceberg/catalog/rest/catalog_properties.cc‎

Lines changed: 6 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -23,15 +23,14 @@
2323

2424
namespace iceberg::rest {
2525

26-
std::unique_ptr<RestCatalogProperties> RestCatalogProperties::default_properties() {
27-
return std::unique_ptr<RestCatalogProperties>(new RestCatalogProperties());
26+
RestCatalogProperties RestCatalogProperties::default_properties() {
27+
return RestCatalogProperties();
2828
}
2929

30-
std::unique_ptr<RestCatalogProperties> RestCatalogProperties::FromMap(
31-
const std::unordered_map<std::string, std::string>& properties) {
32-
auto rest_catalog_config =
33-
std::unique_ptr<RestCatalogProperties>(new RestCatalogProperties());
34-
rest_catalog_config->configs_ = properties;
30+
RestCatalogProperties RestCatalogProperties::FromMap(
31+
std::unordered_map<std::string, std::string> properties) {
32+
RestCatalogProperties rest_catalog_config;
33+
rest_catalog_config.configs_ = std::move(properties);
3534
return rest_catalog_config;
3635
}
3736

‎src/iceberg/catalog/rest/catalog_properties.h‎

Lines changed: 3 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -19,7 +19,6 @@
1919

2020
#pragma once
2121

22-
#include <memory>
2322
#include <string>
2423
#include <unordered_map>
2524

@@ -51,21 +50,18 @@ class ICEBERG_REST_EXPORT RestCatalogProperties
5150
inline static constexpr std::string_view kHeaderPrefix = "header.";
5251

5352
/// \brief Create a default RestCatalogProperties instance.
54-
static std::unique_ptr<RestCatalogProperties> default_properties();
53+
static RestCatalogProperties default_properties();
5554

5655
/// \brief Create a RestCatalogProperties instance from a map of key-value pairs.
57-
static std::unique_ptr<RestCatalogProperties> FromMap(
58-
const std::unordered_map<std::string, std::string>& properties);
56+
static RestCatalogProperties FromMap(
57+
std::unordered_map<std::string, std::string> properties);
5958

6059
/// \brief Returns HTTP headers to be added to every request.
6160
std::unordered_map<std::string, std::string> ExtractHeaders() const;
6261

6362
/// \brief Get the URI of the REST catalog server.
6463
/// \return The URI if configured, or an error if not set or empty.
6564
Result<std::string_view> Uri() const;
66-
67-
private:
68-
RestCatalogProperties() = default;
6965
};
7066

7167
} // namespace iceberg::rest

‎src/iceberg/catalog/rest/rest_catalog.cc‎

Lines changed: 7 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -130,7 +130,7 @@ Result<std::shared_ptr<RestCatalog>> RestCatalog::Make(
130130
ICEBERG_ASSIGN_OR_RAISE(auto server_config,
131131
FetchServerConfig(*paths, config, *init_session));
132132

133-
std::unique_ptr<RestCatalogProperties> final_config = RestCatalogProperties::FromMap(
133+
RestCatalogProperties final_config = RestCatalogProperties::FromMap(
134134
MergeConfigs(server_config.defaults, config.configs(), server_config.overrides));
135135

136136
std::unordered_set<Endpoint> endpoints;
@@ -145,22 +145,21 @@ Result<std::shared_ptr<RestCatalog>> RestCatalog::Make(
145145
}
146146

147147
// Update resource paths based on the final config
148-
ICEBERG_ASSIGN_OR_RAISE(auto final_uri, final_config->Uri());
148+
ICEBERG_ASSIGN_OR_RAISE(auto final_uri, final_config.Uri());
149149
ICEBERG_ASSIGN_OR_RAISE(
150150
paths, ResourcePaths::Make(std::string(TrimTrailingSlash(final_uri)),
151-
final_config->Get(RestCatalogProperties::kPrefix)));
151+
final_config.Get(RestCatalogProperties::kPrefix)));
152152

153-
auto client = std::make_unique<HttpClient>(final_config->ExtractHeaders());
153+
auto client = std::make_unique<HttpClient>(final_config.ExtractHeaders());
154154
ICEBERG_ASSIGN_OR_RAISE(auto catalog_session,
155-
auth_manager->CatalogSession(*client, final_config->configs()));
155+
auth_manager->CatalogSession(*client, final_config.configs()));
156156

157157
return std::shared_ptr<RestCatalog>(new RestCatalog(
158158
std::move(final_config), std::move(file_io), std::move(client), std::move(paths),
159159
std::move(endpoints), std::move(auth_manager), std::move(catalog_session)));
160160
}
161161

162-
RestCatalog::RestCatalog(std::unique_ptr<RestCatalogProperties> config,
163-
std::shared_ptr<FileIO> file_io,
162+
RestCatalog::RestCatalog(RestCatalogProperties config, std::shared_ptr<FileIO> file_io,
164163
std::unique_ptr<HttpClient> client,
165164
std::unique_ptr<ResourcePaths> paths,
166165
std::unordered_set<Endpoint> endpoints,
@@ -170,7 +169,7 @@ RestCatalog::RestCatalog(std::unique_ptr<RestCatalogProperties> config,
170169
file_io_(std::move(file_io)),
171170
client_(std::move(client)),
172171
paths_(std::move(paths)),
173-
name_(config_->Get(RestCatalogProperties::kName)),
172+
name_(config_.Get(RestCatalogProperties::kName)),
174173
supported_endpoints_(std::move(endpoints)),
175174
auth_manager_(std::move(auth_manager)),
176175
catalog_session_(std::move(catalog_session)) {

‎src/iceberg/catalog/rest/rest_catalog.h‎

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -24,6 +24,7 @@
2424
#include <unordered_set>
2525

2626
#include "iceberg/catalog.h"
27+
#include "iceberg/catalog/rest/catalog_properties.h"
2728
#include "iceberg/catalog/rest/endpoint.h"
2829
#include "iceberg/catalog/rest/iceberg_rest_export.h"
2930
#include "iceberg/catalog/rest/type_fwd.h"
@@ -104,9 +105,8 @@ class ICEBERG_REST_EXPORT RestCatalog : public Catalog,
104105
const std::string& metadata_file_location) override;
105106

106107
private:
107-
RestCatalog(std::unique_ptr<RestCatalogProperties> config,
108-
std::shared_ptr<FileIO> file_io, std::unique_ptr<HttpClient> client,
109-
std::unique_ptr<ResourcePaths> paths,
108+
RestCatalog(RestCatalogProperties config, std::shared_ptr<FileIO> file_io,
109+
std::unique_ptr<HttpClient> client, std::unique_ptr<ResourcePaths> paths,
110110
std::unordered_set<Endpoint> endpoints,
111111
std::unique_ptr<auth::AuthManager> auth_manager,
112112
std::shared_ptr<auth::AuthSession> catalog_session);
@@ -119,7 +119,7 @@ class ICEBERG_REST_EXPORT RestCatalog : public Catalog,
119119
const std::string& location,
120120
const std::unordered_map<std::string, std::string>& properties, bool stage_create);
121121

122-
std::unique_ptr<RestCatalogProperties> config_;
122+
RestCatalogProperties config_;
123123
std::shared_ptr<FileIO> file_io_;
124124
std::unique_ptr<HttpClient> client_;
125125
std::unique_ptr<ResourcePaths> paths_;

‎src/iceberg/test/config_test.cc‎

Lines changed: 42 additions & 44 deletions
Original file line numberDiff line numberDiff line change
@@ -65,9 +65,7 @@ class TestConfig : public ConfigBase<TestConfig> {
6565
EnumToString, StringToEnum};
6666
inline static const Entry<double> kDoubleConfig{"double_config", 3.14};
6767

68-
static std::unique_ptr<TestConfig> default_properties() {
69-
return std::unique_ptr<TestConfig>(new TestConfig());
70-
}
68+
static TestConfig default_properties() { return TestConfig(); }
7169

7270
private:
7371
TestConfig() = default;
@@ -77,55 +75,55 @@ TEST(ConfigTest, BasicOperations) {
7775
auto config = TestConfig::default_properties();
7876

7977
// Test default values
80-
ASSERT_EQ(config->Get(TestConfig::kStringConfig), std::string("default_value"));
81-
ASSERT_EQ(config->Get(TestConfig::kIntConfig), 25);
82-
ASSERT_EQ(config->Get(TestConfig::kBoolConfig), false);
83-
ASSERT_EQ(config->Get(TestConfig::kEnumConfig), TestEnum::VALUE1);
84-
ASSERT_EQ(config->Get(TestConfig::kDoubleConfig), 3.14);
78+
ASSERT_EQ(config.Get(TestConfig::kStringConfig), std::string("default_value"));
79+
ASSERT_EQ(config.Get(TestConfig::kIntConfig), 25);
80+
ASSERT_EQ(config.Get(TestConfig::kBoolConfig), false);
81+
ASSERT_EQ(config.Get(TestConfig::kEnumConfig), TestEnum::VALUE1);
82+
ASSERT_EQ(config.Get(TestConfig::kDoubleConfig), 3.14);
8583

8684
// Test setting values
87-
config->Set(TestConfig::kStringConfig, std::string("new_value"));
88-
config->Set(TestConfig::kIntConfig, 100);
89-
config->Set(TestConfig::kBoolConfig, true);
90-
config->Set(TestConfig::kEnumConfig, TestEnum::VALUE2);
91-
config->Set(TestConfig::kDoubleConfig, 2.99);
92-
93-
ASSERT_EQ(config->Get(TestConfig::kStringConfig), "new_value");
94-
ASSERT_EQ(config->Get(TestConfig::kIntConfig), 100);
95-
ASSERT_EQ(config->Get(TestConfig::kBoolConfig), true);
96-
ASSERT_EQ(config->Get(TestConfig::kEnumConfig), TestEnum::VALUE2);
97-
ASSERT_EQ(config->Get(TestConfig::kDoubleConfig), 2.99);
85+
config.Set(TestConfig::kStringConfig, std::string("new_value"));
86+
config.Set(TestConfig::kIntConfig, 100);
87+
config.Set(TestConfig::kBoolConfig, true);
88+
config.Set(TestConfig::kEnumConfig, TestEnum::VALUE2);
89+
config.Set(TestConfig::kDoubleConfig, 2.99);
90+
91+
ASSERT_EQ(config.Get(TestConfig::kStringConfig), "new_value");
92+
ASSERT_EQ(config.Get(TestConfig::kIntConfig), 100);
93+
ASSERT_EQ(config.Get(TestConfig::kBoolConfig), true);
94+
ASSERT_EQ(config.Get(TestConfig::kEnumConfig), TestEnum::VALUE2);
95+
ASSERT_EQ(config.Get(TestConfig::kDoubleConfig), 2.99);
9896

9997
// Test setting values again
100-
config->Set(TestConfig::kStringConfig, std::string("newer_value"));
101-
config->Set(TestConfig::kIntConfig, 200);
102-
config->Set(TestConfig::kBoolConfig, false);
103-
config->Set(TestConfig::kEnumConfig, TestEnum::VALUE1);
104-
config->Set(TestConfig::kDoubleConfig, 3.99);
105-
106-
ASSERT_EQ(config->Get(TestConfig::kStringConfig), "newer_value");
107-
ASSERT_EQ(config->Get(TestConfig::kIntConfig), 200);
108-
ASSERT_EQ(config->Get(TestConfig::kBoolConfig), false);
109-
ASSERT_EQ(config->Get(TestConfig::kEnumConfig), TestEnum::VALUE1);
110-
ASSERT_EQ(config->Get(TestConfig::kDoubleConfig), 3.99);
98+
config.Set(TestConfig::kStringConfig, std::string("newer_value"));
99+
config.Set(TestConfig::kIntConfig, 200);
100+
config.Set(TestConfig::kBoolConfig, false);
101+
config.Set(TestConfig::kEnumConfig, TestEnum::VALUE1);
102+
config.Set(TestConfig::kDoubleConfig, 3.99);
103+
104+
ASSERT_EQ(config.Get(TestConfig::kStringConfig), "newer_value");
105+
ASSERT_EQ(config.Get(TestConfig::kIntConfig), 200);
106+
ASSERT_EQ(config.Get(TestConfig::kBoolConfig), false);
107+
ASSERT_EQ(config.Get(TestConfig::kEnumConfig), TestEnum::VALUE1);
108+
ASSERT_EQ(config.Get(TestConfig::kDoubleConfig), 3.99);
111109

112110
// Test unsetting a value
113-
config->Unset(TestConfig::kIntConfig);
114-
config->Unset(TestConfig::kEnumConfig);
115-
config->Unset(TestConfig::kDoubleConfig);
116-
ASSERT_EQ(config->Get(TestConfig::kIntConfig), 25);
117-
ASSERT_EQ(config->Get(TestConfig::kStringConfig), "newer_value");
118-
ASSERT_EQ(config->Get(TestConfig::kBoolConfig), false);
119-
ASSERT_EQ(config->Get(TestConfig::kEnumConfig), TestEnum::VALUE1);
120-
ASSERT_EQ(config->Get(TestConfig::kDoubleConfig), 3.14);
111+
config.Unset(TestConfig::kIntConfig);
112+
config.Unset(TestConfig::kEnumConfig);
113+
config.Unset(TestConfig::kDoubleConfig);
114+
ASSERT_EQ(config.Get(TestConfig::kIntConfig), 25);
115+
ASSERT_EQ(config.Get(TestConfig::kStringConfig), "newer_value");
116+
ASSERT_EQ(config.Get(TestConfig::kBoolConfig), false);
117+
ASSERT_EQ(config.Get(TestConfig::kEnumConfig), TestEnum::VALUE1);
118+
ASSERT_EQ(config.Get(TestConfig::kDoubleConfig), 3.14);
121119

122120
// Test resetting all values
123-
config->Reset();
124-
ASSERT_EQ(config->Get(TestConfig::kStringConfig), "default_value");
125-
ASSERT_EQ(config->Get(TestConfig::kIntConfig), 25);
126-
ASSERT_EQ(config->Get(TestConfig::kBoolConfig), false);
127-
ASSERT_EQ(config->Get(TestConfig::kEnumConfig), TestEnum::VALUE1);
128-
ASSERT_EQ(config->Get(TestConfig::kDoubleConfig), 3.14);
121+
config.Reset();
122+
ASSERT_EQ(config.Get(TestConfig::kStringConfig), "default_value");
123+
ASSERT_EQ(config.Get(TestConfig::kIntConfig), 25);
124+
ASSERT_EQ(config.Get(TestConfig::kBoolConfig), false);
125+
ASSERT_EQ(config.Get(TestConfig::kEnumConfig), TestEnum::VALUE1);
126+
ASSERT_EQ(config.Get(TestConfig::kDoubleConfig), 3.14);
129127
}
130128

131129
} // namespace iceberg

‎src/iceberg/test/rest_catalog_test.cc‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -133,12 +133,12 @@ class RestCatalogIntegrationTest : public ::testing::Test {
133133
Result<std::shared_ptr<RestCatalog>> CreateCatalog() {
134134
auto config = RestCatalogProperties::default_properties();
135135
config
136-
->Set(RestCatalogProperties::kUri,
137-
std::format("{}:{}", kLocalhostUri, kRestCatalogPort))
136+
.Set(RestCatalogProperties::kUri,
137+
std::format("{}:{}", kLocalhostUri, kRestCatalogPort))
138138
.Set(RestCatalogProperties::kName, std::string(kCatalogName))
139139
.Set(RestCatalogProperties::kWarehouse, std::string(kWarehouseName));
140140
auto file_io = std::make_shared<test::StdFileIO>();
141-
return RestCatalog::Make(*config, std::make_shared<test::StdFileIO>());
141+
return RestCatalog::Make(config, std::make_shared<test::StdFileIO>());
142142
}
143143

144144
// Helper function to create a default schema for testing

0 commit comments

Comments
 (0)