From be4cb1dc5b8945014dc3cd5dfc7b3cfe29c669da Mon Sep 17 00:00:00 2001 From: Manu Zhang Date: Mon, 21 Sep 2026 15:14:32 +0800 Subject: [PATCH] fix: align boolean parsing with Iceberg Java Boolean values were parsed ad hoc in four places, and not consistently: `arrow_s3_file_io.cc` rejected anything that was not "true" or "false", while `config.h`, `auth_managers.cc` and `rest_catalog.cc` read any unrecognized value as false. Each site spelled out the "true"/"false" literals itself, and `StringUtils` covered only numbers. Add `StringUtils::ParseBoolean()`, which mirrors Java's Boolean.parseBoolean, and `PropertyUtil::PropertyAsBoolean()` / `PropertyUtil::PropertyAsOptionalBoolean()`, which mirror Java's PropertyUtil.propertyAsBoolean and propertyAsNullableBoolean, and read every boolean through them. That settles the codebase on Java's contract: "true" ignoring case reads as true, and every other value reads as false. `s3.path-style-access` and `s3.ssl.enabled` change behavior accordingly: a malformed value used to fail the FileIO build and now reads as false, as it does in Java, whose S3FileIOProperties reads both with PropertyUtil.propertyAsBoolean. The test that pinned the old behavior is rewritten to pin the new one. Co-Authored-By: Claude Opus 5 (1M context) --- src/iceberg/arrow/s3/arrow_s3_file_io.cc | 27 ++------ .../catalog/rest/auth/auth_managers.cc | 5 +- src/iceberg/catalog/rest/rest_catalog.cc | 2 +- src/iceberg/test/CMakeLists.txt | 1 + src/iceberg/test/arrow_s3_file_io_test.cc | 13 ++-- src/iceberg/test/property_util_test.cc | 69 +++++++++++++++++++ src/iceberg/test/string_util_test.cc | 10 +++ src/iceberg/util/config.h | 2 +- src/iceberg/util/property_util.cc | 17 +++++ src/iceberg/util/property_util.h | 29 ++++++++ src/iceberg/util/string_util.h | 9 +++ 11 files changed, 154 insertions(+), 30 deletions(-) create mode 100644 src/iceberg/test/property_util_test.cc diff --git a/src/iceberg/arrow/s3/arrow_s3_file_io.cc b/src/iceberg/arrow/s3/arrow_s3_file_io.cc index 7b9f1d4d6..7ce0d0fda 100644 --- a/src/iceberg/arrow/s3/arrow_s3_file_io.cc +++ b/src/iceberg/arrow/s3/arrow_s3_file_io.cc @@ -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 { @@ -52,22 +53,6 @@ const std::string* FindProperty( return it == properties.end() ? nullptr : &it->second; } -Result> ParseOptionalBool( - const std::unordered_map& 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(); @@ -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"; } diff --git a/src/iceberg/catalog/rest/auth/auth_managers.cc b/src/iceberg/catalog/rest/auth/auth_managers.cc index 6ee2637b3..0e914e6c3 100644 --- a/src/iceberg/catalog/rest/auth/auth_managers.cc +++ b/src/iceberg/catalog/rest/auth/auth_managers.cc @@ -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 { @@ -47,8 +48,8 @@ const std::unordered_set& KnownAuthTypes() std::string InferAuthType( const std::unordered_map& 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; } diff --git a/src/iceberg/catalog/rest/rest_catalog.cc b/src/iceberg/catalog/rest/rest_catalog.cc index 4a4f990ea..9635aebef 100644 --- a/src/iceberg/catalog/rest/rest_catalog.cc +++ b/src/iceberg/catalog/rest/rest_catalog.cc @@ -523,7 +523,7 @@ Result> RestCatalog::MakeTableReporter( const TableIdentifier& identifier, const std::shared_ptr& 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, diff --git a/src/iceberg/test/CMakeLists.txt b/src/iceberg/test/CMakeLists.txt index f23bc9181..6f6ff7603 100644 --- a/src/iceberg/test/CMakeLists.txt +++ b/src/iceberg/test/CMakeLists.txt @@ -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 diff --git a/src/iceberg/test/arrow_s3_file_io_test.cc b/src/iceberg/test/arrow_s3_file_io_test.cc index 721c9ae2b..7235ea367 100644 --- a/src/iceberg/test/arrow_s3_file_io_test.cc +++ b/src/iceberg/test/arrow_s3_file_io_test.cc @@ -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"; @@ -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"}, diff --git a/src/iceberg/test/property_util_test.cc b/src/iceberg/test/property_util_test.cc new file mode 100644 index 000000000..6fefb5625 --- /dev/null +++ b/src/iceberg/test/property_util_test.cc @@ -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 +#include +#include +#include + +#include + +namespace iceberg { + +namespace { + +constexpr std::string_view kKey = "test.enabled"; + +std::unordered_map Properties(std::string_view value) { + return {{std::string(kKey), std::string(value)}}; +} + +} // namespace + +TEST(PropertyUtilTest, BooleanDefaultsWhenPropertyIsAbsent) { + const std::unordered_map 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 diff --git a/src/iceberg/test/string_util_test.cc b/src/iceberg/test/string_util_test.cc index 65d5c23cc..5f9a9a1cd 100644 --- a/src/iceberg/test/string_util_test.cc +++ b/src/iceberg/test/string_util_test.cc @@ -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"); diff --git a/src/iceberg/util/config.h b/src/iceberg/util/config.h index 36e92237d..8f81b6baf 100644 --- a/src/iceberg/util/config.h +++ b/src/iceberg/util/config.h @@ -54,7 +54,7 @@ U DefaultFromString(const std::string& val) { if constexpr (std::is_same_v) { return val; } else if constexpr (std::is_same_v) { - return StringUtils::EqualsIgnoreCase(val, "true"); + return StringUtils::ParseBoolean(val); } else if constexpr ((std::is_signed_v && std::is_integral_v) || std::is_floating_point_v) { ICEBERG_ASSIGN_OR_THROW(auto res, StringUtils::ParseNumber(val)); diff --git a/src/iceberg/util/property_util.cc b/src/iceberg/util/property_util.cc index 32d9e3ba1..7fbd23d44 100644 --- a/src/iceberg/util/property_util.cc +++ b/src/iceberg/util/property_util.cc @@ -20,6 +20,7 @@ #include "iceberg/util/property_util.h" #include +#include #include "iceberg/table_properties.h" #include "iceberg/util/string_util.h" @@ -46,4 +47,20 @@ Status PropertyUtil::ValidateCommitProperties( return {}; } +bool PropertyUtil::PropertyAsBoolean( + const std::unordered_map& properties, std::string_view key, + bool default_value) { + return PropertyAsOptionalBoolean(properties, key).value_or(default_value); +} + +std::optional PropertyUtil::PropertyAsOptionalBoolean( + const std::unordered_map& 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 diff --git a/src/iceberg/util/property_util.h b/src/iceberg/util/property_util.h index c46d3a6ab..72bbf9f04 100644 --- a/src/iceberg/util/property_util.h +++ b/src/iceberg/util/property_util.h @@ -22,7 +22,9 @@ /// \file iceberg/util/property_util.h /// \brief Provide property conversion helpers. +#include #include +#include #include #include "iceberg/iceberg_export.h" @@ -34,6 +36,33 @@ class ICEBERG_EXPORT PropertyUtil { public: static Status ValidateCommitProperties( const std::unordered_map& 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& 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 PropertyAsOptionalBoolean( + const std::unordered_map& properties, + std::string_view key); }; } // namespace iceberg diff --git a/src/iceberg/util/string_util.h b/src/iceberg/util/string_util.h index 8202e60a3..2559b47a3 100644 --- a/src/iceberg/util/string_util.h +++ b/src/iceberg/util/string_util.h @@ -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.