Skip to content

refactor: Use RestCatalogProperties by value instead of unique_ptr #575

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
wgtmac merged 1 commit into from
Feb 25, 2026
Merged

refactor: Use RestCatalogProperties by value instead of unique_ptr #575

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 6 additions & 7 deletions src/iceberg/catalog/rest/catalog_properties.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,15 +23,14 @@

namespace iceberg::rest {

std::unique_ptr<RestCatalogProperties> RestCatalogProperties::default_properties() {
return std::unique_ptr<RestCatalogProperties>(new RestCatalogProperties());
RestCatalogProperties RestCatalogProperties::default_properties() {
return RestCatalogProperties();
}

std::unique_ptr<RestCatalogProperties> RestCatalogProperties::FromMap(
const std::unordered_map<std::string, std::string>& properties) {
auto rest_catalog_config =
std::unique_ptr<RestCatalogProperties>(new RestCatalogProperties());
rest_catalog_config->configs_ = properties;
RestCatalogProperties RestCatalogProperties::FromMap(
std::unordered_map<std::string, std::string> properties) {
RestCatalogProperties rest_catalog_config;
rest_catalog_config.configs_ = std::move(properties);
return rest_catalog_config;
}

Expand Down
10 changes: 3 additions & 7 deletions src/iceberg/catalog/rest/catalog_properties.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,6 @@

#pragma once

#include <memory>
#include <string>
#include <unordered_map>

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

/// \brief Create a default RestCatalogProperties instance.
static std::unique_ptr<RestCatalogProperties> default_properties();
static RestCatalogProperties default_properties();

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

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

/// \brief Get the URI of the REST catalog server.
/// \return The URI if configured, or an error if not set or empty.
Result<std::string_view> Uri() const;

private:
RestCatalogProperties() = default;
};

} // namespace iceberg::rest
15 changes: 7 additions & 8 deletions src/iceberg/catalog/rest/rest_catalog.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -130,7 +130,7 @@ Result<std::shared_ptr<RestCatalog>> RestCatalog::Make(
ICEBERG_ASSIGN_OR_RAISE(auto server_config,
FetchServerConfig(*paths, config, *init_session));

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

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

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

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

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

RestCatalog::RestCatalog(std::unique_ptr<RestCatalogProperties> config,
std::shared_ptr<FileIO> file_io,
RestCatalog::RestCatalog(RestCatalogProperties config, std::shared_ptr<FileIO> file_io,
std::unique_ptr<HttpClient> client,
std::unique_ptr<ResourcePaths> paths,
std::unordered_set<Endpoint> endpoints,
Expand All @@ -170,7 +169,7 @@ RestCatalog::RestCatalog(std::unique_ptr<RestCatalogProperties> config,
file_io_(std::move(file_io)),
client_(std::move(client)),
paths_(std::move(paths)),
name_(config_->Get(RestCatalogProperties::kName)),
name_(config_.Get(RestCatalogProperties::kName)),
supported_endpoints_(std::move(endpoints)),
auth_manager_(std::move(auth_manager)),
catalog_session_(std::move(catalog_session)) {
Expand Down
8 changes: 4 additions & 4 deletions src/iceberg/catalog/rest/rest_catalog.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@
#include <unordered_set>

#include "iceberg/catalog.h"
#include "iceberg/catalog/rest/catalog_properties.h"
#include "iceberg/catalog/rest/endpoint.h"
#include "iceberg/catalog/rest/iceberg_rest_export.h"
#include "iceberg/catalog/rest/type_fwd.h"
Expand Down Expand Up @@ -104,9 +105,8 @@ class ICEBERG_REST_EXPORT RestCatalog : public Catalog,
const std::string& metadata_file_location) override;

private:
RestCatalog(std::unique_ptr<RestCatalogProperties> config,
std::shared_ptr<FileIO> file_io, std::unique_ptr<HttpClient> client,
std::unique_ptr<ResourcePaths> paths,
RestCatalog(RestCatalogProperties config, std::shared_ptr<FileIO> file_io,
std::unique_ptr<HttpClient> client, std::unique_ptr<ResourcePaths> paths,
std::unordered_set<Endpoint> endpoints,
std::unique_ptr<auth::AuthManager> auth_manager,
std::shared_ptr<auth::AuthSession> catalog_session);
Expand All @@ -119,7 +119,7 @@ class ICEBERG_REST_EXPORT RestCatalog : public Catalog,
const std::string& location,
const std::unordered_map<std::string, std::string>& properties, bool stage_create);

std::unique_ptr<RestCatalogProperties> config_;
RestCatalogProperties config_;
std::shared_ptr<FileIO> file_io_;
std::unique_ptr<HttpClient> client_;
std::unique_ptr<ResourcePaths> paths_;
Expand Down
86 changes: 42 additions & 44 deletions src/iceberg/test/config_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,9 +65,7 @@ class TestConfig : public ConfigBase<TestConfig> {
EnumToString, StringToEnum};
inline static const Entry<double> kDoubleConfig{"double_config", 3.14};

static std::unique_ptr<TestConfig> default_properties() {
return std::unique_ptr<TestConfig>(new TestConfig());
}
static TestConfig default_properties() { return TestConfig(); }

private:
TestConfig() = default;
Expand All @@ -77,55 +75,55 @@ TEST(ConfigTest, BasicOperations) {
auto config = TestConfig::default_properties();

// Test default values
ASSERT_EQ(config->Get(TestConfig::kStringConfig), std::string("default_value"));
ASSERT_EQ(config->Get(TestConfig::kIntConfig), 25);
ASSERT_EQ(config->Get(TestConfig::kBoolConfig), false);
ASSERT_EQ(config->Get(TestConfig::kEnumConfig), TestEnum::VALUE1);
ASSERT_EQ(config->Get(TestConfig::kDoubleConfig), 3.14);
ASSERT_EQ(config.Get(TestConfig::kStringConfig), std::string("default_value"));
ASSERT_EQ(config.Get(TestConfig::kIntConfig), 25);
ASSERT_EQ(config.Get(TestConfig::kBoolConfig), false);
ASSERT_EQ(config.Get(TestConfig::kEnumConfig), TestEnum::VALUE1);
ASSERT_EQ(config.Get(TestConfig::kDoubleConfig), 3.14);

// Test setting values
config->Set(TestConfig::kStringConfig, std::string("new_value"));
config->Set(TestConfig::kIntConfig, 100);
config->Set(TestConfig::kBoolConfig, true);
config->Set(TestConfig::kEnumConfig, TestEnum::VALUE2);
config->Set(TestConfig::kDoubleConfig, 2.99);

ASSERT_EQ(config->Get(TestConfig::kStringConfig), "new_value");
ASSERT_EQ(config->Get(TestConfig::kIntConfig), 100);
ASSERT_EQ(config->Get(TestConfig::kBoolConfig), true);
ASSERT_EQ(config->Get(TestConfig::kEnumConfig), TestEnum::VALUE2);
ASSERT_EQ(config->Get(TestConfig::kDoubleConfig), 2.99);
config.Set(TestConfig::kStringConfig, std::string("new_value"));
config.Set(TestConfig::kIntConfig, 100);
config.Set(TestConfig::kBoolConfig, true);
config.Set(TestConfig::kEnumConfig, TestEnum::VALUE2);
config.Set(TestConfig::kDoubleConfig, 2.99);

ASSERT_EQ(config.Get(TestConfig::kStringConfig), "new_value");
ASSERT_EQ(config.Get(TestConfig::kIntConfig), 100);
ASSERT_EQ(config.Get(TestConfig::kBoolConfig), true);
ASSERT_EQ(config.Get(TestConfig::kEnumConfig), TestEnum::VALUE2);
ASSERT_EQ(config.Get(TestConfig::kDoubleConfig), 2.99);

// Test setting values again
config->Set(TestConfig::kStringConfig, std::string("newer_value"));
config->Set(TestConfig::kIntConfig, 200);
config->Set(TestConfig::kBoolConfig, false);
config->Set(TestConfig::kEnumConfig, TestEnum::VALUE1);
config->Set(TestConfig::kDoubleConfig, 3.99);

ASSERT_EQ(config->Get(TestConfig::kStringConfig), "newer_value");
ASSERT_EQ(config->Get(TestConfig::kIntConfig), 200);
ASSERT_EQ(config->Get(TestConfig::kBoolConfig), false);
ASSERT_EQ(config->Get(TestConfig::kEnumConfig), TestEnum::VALUE1);
ASSERT_EQ(config->Get(TestConfig::kDoubleConfig), 3.99);
config.Set(TestConfig::kStringConfig, std::string("newer_value"));
config.Set(TestConfig::kIntConfig, 200);
config.Set(TestConfig::kBoolConfig, false);
config.Set(TestConfig::kEnumConfig, TestEnum::VALUE1);
config.Set(TestConfig::kDoubleConfig, 3.99);

ASSERT_EQ(config.Get(TestConfig::kStringConfig), "newer_value");
ASSERT_EQ(config.Get(TestConfig::kIntConfig), 200);
ASSERT_EQ(config.Get(TestConfig::kBoolConfig), false);
ASSERT_EQ(config.Get(TestConfig::kEnumConfig), TestEnum::VALUE1);
ASSERT_EQ(config.Get(TestConfig::kDoubleConfig), 3.99);

// Test unsetting a value
config->Unset(TestConfig::kIntConfig);
config->Unset(TestConfig::kEnumConfig);
config->Unset(TestConfig::kDoubleConfig);
ASSERT_EQ(config->Get(TestConfig::kIntConfig), 25);
ASSERT_EQ(config->Get(TestConfig::kStringConfig), "newer_value");
ASSERT_EQ(config->Get(TestConfig::kBoolConfig), false);
ASSERT_EQ(config->Get(TestConfig::kEnumConfig), TestEnum::VALUE1);
ASSERT_EQ(config->Get(TestConfig::kDoubleConfig), 3.14);
config.Unset(TestConfig::kIntConfig);
config.Unset(TestConfig::kEnumConfig);
config.Unset(TestConfig::kDoubleConfig);
ASSERT_EQ(config.Get(TestConfig::kIntConfig), 25);
ASSERT_EQ(config.Get(TestConfig::kStringConfig), "newer_value");
ASSERT_EQ(config.Get(TestConfig::kBoolConfig), false);
ASSERT_EQ(config.Get(TestConfig::kEnumConfig), TestEnum::VALUE1);
ASSERT_EQ(config.Get(TestConfig::kDoubleConfig), 3.14);

// Test resetting all values
config->Reset();
ASSERT_EQ(config->Get(TestConfig::kStringConfig), "default_value");
ASSERT_EQ(config->Get(TestConfig::kIntConfig), 25);
ASSERT_EQ(config->Get(TestConfig::kBoolConfig), false);
ASSERT_EQ(config->Get(TestConfig::kEnumConfig), TestEnum::VALUE1);
ASSERT_EQ(config->Get(TestConfig::kDoubleConfig), 3.14);
config.Reset();
ASSERT_EQ(config.Get(TestConfig::kStringConfig), "default_value");
ASSERT_EQ(config.Get(TestConfig::kIntConfig), 25);
ASSERT_EQ(config.Get(TestConfig::kBoolConfig), false);
ASSERT_EQ(config.Get(TestConfig::kEnumConfig), TestEnum::VALUE1);
ASSERT_EQ(config.Get(TestConfig::kDoubleConfig), 3.14);
}

} // namespace iceberg
6 changes: 3 additions & 3 deletions src/iceberg/test/rest_catalog_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -133,12 +133,12 @@ class RestCatalogIntegrationTest : public ::testing::Test {
Result<std::shared_ptr<RestCatalog>> CreateCatalog() {
auto config = RestCatalogProperties::default_properties();
config
->Set(RestCatalogProperties::kUri,
std::format("{}:{}", kLocalhostUri, kRestCatalogPort))
.Set(RestCatalogProperties::kUri,
std::format("{}:{}", kLocalhostUri, kRestCatalogPort))
.Set(RestCatalogProperties::kName, std::string(kCatalogName))
.Set(RestCatalogProperties::kWarehouse, std::string(kWarehouseName));
auto file_io = std::make_shared<test::StdFileIO>();
return RestCatalog::Make(*config, std::make_shared<test::StdFileIO>());
return RestCatalog::Make(config, std::make_shared<test::StdFileIO>());
}

// Helper function to create a default schema for testing
Expand Down
Loading