Skip to content
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.

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

Config serialization should take into account environment variables #4865

Merged
merged 2 commits into from
Apr 12, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
22 changes: 21 additions & 1 deletion test/src/unit-cppapi-config.cc
Original file line number Diff line number Diff line change
Expand Up @@ -34,9 +34,24 @@

#include <test/support/tdb_catch.h>
#include "test/support/src/helpers.h"
#include "tiledb/api/c_api/config/config_api_internal.h"
#include "tiledb/sm/c_api/tiledb_serialization.h"
#include "tiledb/sm/cpp_api/tiledb"

using namespace tiledb::sm;

class tiledb::sm::WhiteboxConfig {
public:
WhiteboxConfig(tiledb::sm::Config config)
: config_(config){};

const std::map<std::string, std::string>& get_all_params() const {
return config_.param_values();
}

tiledb::sm::Config config_;
};

TEST_CASE("C++ API: Config", "[cppapi][config]") {
tiledb::Config config;
config["foo"] = "bar";
Expand Down Expand Up @@ -159,7 +174,12 @@ TEST_CASE("C++ API: Config Serialization", "[cppapi][config][serialization]") {
CHECK(rc == TILEDB_OK);
tiledb::Config config2(&config2_ptr);

bool config_equal = config1 == config2;
auto cfg1 = config1.ptr().get()->config();
auto cfg2 = config2.ptr().get()->config();
// Check that the deserialized config already contains the values set in
// environment variables
bool config_equal = cfg1.get_all_params_from_config_or_env() ==
WhiteboxConfig(cfg2).get_all_params();
CHECK(config_equal);

// Check for inequality
Expand Down
11 changes: 11 additions & 0 deletions tiledb/sm/config/config.cc
Original file line number Diff line number Diff line change
Expand Up @@ -918,6 +918,17 @@ const char* Config::get_from_config_or_env(
return *found ? value_config : "";
}

const std::map<std::string, std::string>
Config::get_all_params_from_config_or_env() const {
std::map<std::string, std::string> values;
bool found = false;
for (const auto& [key, value] : param_values_) {
std::string val = get_from_config_or_env(key, &found);
values.emplace(key, val);
}
return values;
}

template <class T, bool must_find_>
optional<T> Config::get_internal(const std::string& key) const {
auto value = get_internal_string<must_find_>(key);
Expand Down
18 changes: 15 additions & 3 deletions tiledb/sm/config/config.h
Original file line number Diff line number Diff line change
Expand Up @@ -61,12 +61,20 @@ using namespace tiledb::common;

namespace tiledb::sm {

class WhiteboxConfig;

/**
* This class manages the TileDB configuration options.
* It is implemented as a simple map from string to string.
* Parsing to appropriate types happens on demand.
*/
class Config {
friend class ConfigIter;
/**
* WhiteboxConfig makes available internals of Config for testing.
*/
friend class WhiteboxConfig;

public:
/* ****************************** */
/* CONFIG DEFAULTS */
Expand Down Expand Up @@ -689,9 +697,6 @@ class Config {
Status get_vector(
const std::string& param, std::vector<T>* value, bool* found) const;

/** Returns the param -> value map. */
const std::map<std::string, std::string>& param_values() const;

/** Gets the set parameters. */
const std::set<std::string>& set_params() const;

Expand All @@ -706,6 +711,10 @@ class Config {
/** Compares configs for equality. */
bool operator==(const Config& rhs) const;

/** Get all config params taking into account environment variables */
const std::map<std::string, std::string> get_all_params_from_config_or_env()
const;

Comment on lines +714 to +717
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe a better way would be to populate the config options with the environment variables at the time the Config gets created, but it might introduce complications when unsetting values.

private:
/* ********************************* */
/* PRIVATE ATTRIBUTES */
Expand Down Expand Up @@ -782,6 +791,9 @@ class Config {

template <bool must_find_>
optional<std::string> get_internal_string(const std::string& key) const;

/** Returns the param -> value map. */
const std::map<std::string, std::string>& param_values() const;
};

/**
Expand Down
5 changes: 3 additions & 2 deletions tiledb/sm/serialization/config.cc
Original file line number Diff line number Diff line change
Expand Up @@ -61,9 +61,10 @@ namespace serialization {

Status config_to_capnp(
const Config& config, capnp::Config::Builder* config_builder) {
auto entries = config_builder->initEntries(config.param_values().size());
auto config_params = config.get_all_params_from_config_or_env();
auto entries = config_builder->initEntries(config_params.size());
uint64_t i = 0;
for (const auto& kv : config.param_values()) {
for (const auto& kv : config_params) {
entries[i].setKey(kv.first);
entries[i].setValue(kv.second);
++i;
Expand Down
Loading