Skip to content
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
27 changes: 7 additions & 20 deletions src/iceberg/arrow/s3/arrow_s3_file_io.cc
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,7 @@
#include "iceberg/arrow/s3/s3_properties.h"
#include "iceberg/logging/log_macros.h"
#include "iceberg/util/macros.h"
#include "iceberg/util/property_util.h"
#include "iceberg/util/string_util.h"

namespace iceberg::arrow {
Expand All @@ -52,22 +53,6 @@ const std::string* FindProperty(
return it == properties.end() ? nullptr : &it->second;
}

Result<std::optional<bool>> ParseOptionalBool(
const std::unordered_map<std::string, std::string>& properties,
std::string_view key) {
const auto* value = FindProperty(properties, key);
if (value == nullptr) {
return std::nullopt;
}
if (StringUtils::EqualsIgnoreCase(*value, "true")) {
return true;
}
if (StringUtils::EqualsIgnoreCase(*value, "false")) {
return false;
}
return InvalidArgument(R"("{}" must be "true" or "false")", key);
}

Status EnsureS3Initialized() {
static const ::arrow::Status init_status = []() {
auto options = ::arrow::fs::S3GlobalOptions::Defaults();
Expand Down Expand Up @@ -136,15 +121,17 @@ Result<::arrow::fs::S3Options> ConfigureS3Options(
options.endpoint_override = SplitEndpointScheme(endpoint_env, options);
}

ICEBERG_ASSIGN_OR_RAISE(const auto path_style_access,
ParseOptionalBool(properties, S3Properties::kPathStyleAccess));
// Both boolean properties below read through PropertyUtil, so a value that does not
// spell a boolean reads as false instead of failing the build, as in Java.
const auto path_style_access =
PropertyUtil::PropertyAsOptionalBoolean(properties, S3Properties::kPathStyleAccess);
if (path_style_access.has_value()) {
options.force_virtual_addressing = !*path_style_access;
}

// Explicit `s3.ssl.enabled` overrides any endpoint-derived scheme.
ICEBERG_ASSIGN_OR_RAISE(const auto ssl_enabled,
ParseOptionalBool(properties, S3Properties::kSslEnabled));
const auto ssl_enabled =
PropertyUtil::PropertyAsOptionalBoolean(properties, S3Properties::kSslEnabled);
if (ssl_enabled.has_value()) {
options.scheme = *ssl_enabled ? "https" : "http";
}
Expand Down
5 changes: 3 additions & 2 deletions src/iceberg/catalog/rest/auth/auth_managers.cc
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@

#include "iceberg/catalog/rest/auth/auth_manager_internal.h"
#include "iceberg/catalog/rest/auth/auth_properties.h"
#include "iceberg/util/property_util.h"
#include "iceberg/util/string_util.h"

namespace iceberg::rest::auth {
Expand All @@ -47,8 +48,8 @@ const std::unordered_set<std::string, StringHash, StringEqual>& KnownAuthTypes()
std::string InferAuthType(
const std::unordered_map<std::string, std::string>& properties) {
// Deprecated alias: rest.sigv4-enabled=true forces SigV4.
if (auto it = properties.find(AuthProperties::kSigV4Enabled);
it != properties.end() && StringUtils::EqualsIgnoreCase(it->second, "true")) {
if (PropertyUtil::PropertyAsBoolean(properties, AuthProperties::kSigV4Enabled,
/*default_value=*/false)) {
return AuthProperties::kAuthTypeSigV4;
}

Expand Down
2 changes: 1 addition & 1 deletion src/iceberg/catalog/rest/rest_catalog.cc
Original file line number Diff line number Diff line change
Expand Up @@ -523,7 +523,7 @@ Result<std::shared_ptr<MetricsReporter>> RestCatalog::MakeTableReporter(
const TableIdentifier& identifier,
const std::shared_ptr<auth::AuthSession>& table_session) const {
auto metrics_enabled = config_.Get(RestCatalogProperties::kMetricsReportingEnabled);
if (StringUtils::ToLower(metrics_enabled) == "true" &&
if (StringUtils::ParseBoolean(metrics_enabled) &&
supported_endpoints_.contains(Endpoint::ReportMetrics())) {
ICEBERG_ASSIGN_OR_RAISE(auto path, paths_->Metrics(identifier));
auto post = [client = client_](const std::string& endpoint, const std::string& body,
Expand Down
1 change: 1 addition & 0 deletions src/iceberg/test/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -143,6 +143,7 @@ add_iceberg_test(util_test
roaring_position_bitmap_test.cc
position_delete_index_test.cc
position_delete_range_consumer_test.cc
property_util_test.cc
resolving_file_io_test.cc
retry_util_test.cc
string_util_test.cc
Expand Down
13 changes: 7 additions & 6 deletions src/iceberg/test/arrow_s3_file_io_test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -245,12 +245,6 @@ TEST_F(ArrowS3FileIOTest, RejectsIncompleteStaticCredentials) {
"S3 client access key ID and secret access key must be set"));
}

TEST_F(ArrowS3FileIOTest, RejectsInvalidBooleanProperties) {
auto result =
MakeS3FileIO({{std::string(S3Properties::kPathStyleAccess), "not-a-bool"}});
EXPECT_THAT(result, IsError(ErrorKind::kInvalidArgument));
}

TEST_F(ArrowS3FileIOTest, ReadWrite) {
if (!HasIntegrationEnv()) {
GTEST_SKIP() << "Set ICEBERG_TEST_S3_URI to enable S3 IO test";
Expand Down Expand Up @@ -401,6 +395,13 @@ TEST_F(ArrowS3FileIOTest, PathStyleAccess) {
EXPECT_FALSE(path_style->force_virtual_addressing);
}

TEST_F(ArrowS3FileIOTest, InvalidBooleanPropertyReadsAsFalse) {
auto options =
ConfigureS3Options({{std::string(S3Properties::kPathStyleAccess), "not-a-bool"}});
ASSERT_THAT(options, IsOk());
EXPECT_TRUE(options->force_virtual_addressing);
}

TEST_F(ArrowS3FileIOTest, Timeouts) {
auto result =
ConfigureS3Options({{std::string(S3Properties::kConnectTimeoutMs), "5000"},
Expand Down
69 changes: 69 additions & 0 deletions src/iceberg/test/property_util_test.cc
Original file line number Diff line number Diff line change
@@ -0,0 +1,69 @@
/*
* Licensed to the Apache Software Foundation (ASF) under one
* or more contributor license agreements. See the NOTICE file
* distributed with this work for additional information
* regarding copyright ownership. The ASF licenses this file
* to you under the Apache License, Version 2.0 (the
* "License"); you may not use this file except in compliance
* with the License. You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing,
* software distributed under the License is distributed on an
* "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
* KIND, either express or implied. See the License for the
* specific language governing permissions and limitations
* under the License.
*/

#include "iceberg/util/property_util.h"

#include <optional>
#include <string>
#include <string_view>
#include <unordered_map>

#include <gtest/gtest.h>

namespace iceberg {

namespace {

constexpr std::string_view kKey = "test.enabled";

std::unordered_map<std::string, std::string> Properties(std::string_view value) {
return {{std::string(kKey), std::string(value)}};
}

} // namespace

TEST(PropertyUtilTest, BooleanDefaultsWhenPropertyIsAbsent) {
const std::unordered_map<std::string, std::string> empty;
EXPECT_TRUE(PropertyUtil::PropertyAsBoolean(empty, kKey, true));
EXPECT_FALSE(PropertyUtil::PropertyAsBoolean(empty, kKey, false));
EXPECT_EQ(PropertyUtil::PropertyAsOptionalBoolean(empty, kKey), std::nullopt);
}

TEST(PropertyUtilTest, BooleanIgnoresCase) {
for (const auto* value : {"true", "TRUE", "TrUe"}) {
EXPECT_TRUE(PropertyUtil::PropertyAsBoolean(Properties(value), kKey, false)) << value;
EXPECT_EQ(PropertyUtil::PropertyAsOptionalBoolean(Properties(value), kKey), true)
<< value;
}
for (const auto* value : {"false", "FALSE", "FaLsE"}) {
EXPECT_FALSE(PropertyUtil::PropertyAsBoolean(Properties(value), kKey, true)) << value;
EXPECT_EQ(PropertyUtil::PropertyAsOptionalBoolean(Properties(value), kKey), false)
<< value;
}
}

TEST(PropertyUtilTest, NonBooleanValueReadsAsFalse) {
for (const auto* value : {"", " true", "yes", "1", "ture"}) {
EXPECT_FALSE(PropertyUtil::PropertyAsBoolean(Properties(value), kKey, true)) << value;
EXPECT_EQ(PropertyUtil::PropertyAsOptionalBoolean(Properties(value), kKey), false)
<< value;
}
}

} // namespace iceberg
10 changes: 10 additions & 0 deletions src/iceberg/test/string_util_test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,16 @@ TEST(StringUtilsTest, ToLower) {
ASSERT_EQ(StringUtils::ToLower("123"), "123");
}

TEST(StringUtilsTest, ParseBoolean) {
for (const auto* value : {"true", "TRUE", "TrUe"}) {
EXPECT_TRUE(StringUtils::ParseBoolean(value)) << value;
}
for (const auto* value :
{"false", "FALSE", "FaLsE", "", " true", "true ", "yes", "1", "ture"}) {
EXPECT_FALSE(StringUtils::ParseBoolean(value)) << value;
}
}

TEST(StringUtilsTest, ToUpper) {
ASSERT_EQ(StringUtils::ToUpper("abc"), "ABC");
ASSERT_EQ(StringUtils::ToUpper("A-bC"), "A-BC");
Expand Down
2 changes: 1 addition & 1 deletion src/iceberg/util/config.h
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,7 @@ U DefaultFromString(const std::string& val) {
if constexpr (std::is_same_v<U, std::string>) {
return val;
} else if constexpr (std::is_same_v<U, bool>) {
return StringUtils::EqualsIgnoreCase(val, "true");
return StringUtils::ParseBoolean(val);
} else if constexpr ((std::is_signed_v<U> && std::is_integral_v<U>) ||
std::is_floating_point_v<U>) {
ICEBERG_ASSIGN_OR_THROW(auto res, StringUtils::ParseNumber<U>(val));
Expand Down
17 changes: 17 additions & 0 deletions src/iceberg/util/property_util.cc
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@
#include "iceberg/util/property_util.h"

#include <cstdint>
#include <string>

#include "iceberg/table_properties.h"
#include "iceberg/util/string_util.h"
Expand All @@ -46,4 +47,20 @@ Status PropertyUtil::ValidateCommitProperties(
return {};
}

bool PropertyUtil::PropertyAsBoolean(
const std::unordered_map<std::string, std::string>& properties, std::string_view key,
bool default_value) {
return PropertyAsOptionalBoolean(properties, key).value_or(default_value);
}

std::optional<bool> PropertyUtil::PropertyAsOptionalBoolean(
const std::unordered_map<std::string, std::string>& properties,
std::string_view key) {
auto it = properties.find(std::string(key));
if (it == properties.end()) {
return std::nullopt;
}
return StringUtils::ParseBoolean(it->second);
}

} // namespace iceberg
29 changes: 29 additions & 0 deletions src/iceberg/util/property_util.h
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,9 @@
/// \file iceberg/util/property_util.h
/// \brief Provide property conversion helpers.

#include <optional>
#include <string>
#include <string_view>
#include <unordered_map>

#include "iceberg/iceberg_export.h"
Expand All @@ -34,6 +36,33 @@ class ICEBERG_EXPORT PropertyUtil {
public:
static Status ValidateCommitProperties(
const std::unordered_map<std::string, std::string>& properties);

/// \brief Read a boolean property from a property map.
///
/// Mirrors Java's PropertyUtil.propertyAsBoolean: the value is parsed with
/// StringUtils::ParseBoolean, so anything that is not "true" ignoring case reads as
/// false rather than being rejected.
///
/// \param properties The property map to read from.
/// \param key The property key.
/// \param default_value Returned when the property is absent.
/// \return The parsed value, or default_value if the property is absent.
static bool PropertyAsBoolean(
const std::unordered_map<std::string, std::string>& properties,
std::string_view key, bool default_value);

/// \brief Read a boolean property that may be unset.
///
/// Like PropertyAsBoolean, but returns std::nullopt when the property is absent so
/// callers can distinguish an unset property from an explicit "false". Mirrors Java's
/// PropertyUtil.propertyAsNullableBoolean.
///
/// \param properties The property map to read from.
/// \param key The property key.
/// \return The parsed value, or std::nullopt if the property is absent.
static std::optional<bool> PropertyAsOptionalBoolean(
const std::unordered_map<std::string, std::string>& properties,
std::string_view key);
};

} // namespace iceberg
9 changes: 9 additions & 0 deletions src/iceberg/util/string_util.h
Original file line number Diff line number Diff line change
Expand Up @@ -168,6 +168,15 @@ class ICEBERG_EXPORT StringUtils {
return value;
}

/// \brief Parse a boolean from its string representation, ignoring case.
///
/// Mirrors Iceberg Java's Boolean.parseBoolean, which every boolean property in the
/// Java implementation is read through: "true" in any case reads as true, and every
/// other value reads as false, including "yes", "1" and typos such as "ture". Parsing
/// therefore cannot fail, so a caller that wants to reject a malformed value has to
/// check for it separately.
static bool ParseBoolean(std::string_view str) { return EqualsIgnoreCase(str, "true"); }

private:
// ASCII-only case mappings. These avoid std::toupper/std::tolower, which are
// locale-dependent and have undefined behavior for negative char values.
Expand Down
Loading