Skip to content

fix: parse boolean properties case-insensitively #847

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

Open
fallintoplace wants to merge 2 commits into
base: main
Choose a base branch
from
+21 −7
Open

fix: parse boolean properties case-insensitively #847

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
afe81a0
fix: parse boolean properties case-insensitively
fallintoplace Jul 25, 2026
2f752ed
fix: parse S3 boolean properties case-insensitively
fallintoplace Jul 25, 2026
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
4 changes: 2 additions & 2 deletions src/iceberg/arrow/s3/arrow_s3_file_io.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,10 +58,10 @@ Result<std::optional<bool>> ParseOptionalBool(
if (value == nullptr) {
return std::nullopt;
}
if (*value == "true") {
if (StringUtils::EqualsIgnoreCase(*value, "true")) {
return true;
}
if (*value == "false") {
if (StringUtils::EqualsIgnoreCase(*value, "false")) {
return false;
}
return InvalidArgument(R"("{}" must be "true" or "false")", key);
Expand Down
8 changes: 4 additions & 4 deletions src/iceberg/test/arrow_s3_file_io_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -269,25 +269,25 @@ TEST_F(ArrowS3FileIOTest, EndpointScheme) {
TEST_F(ArrowS3FileIOTest, SslEnabled) {
auto https =
ConfigureS3Options({{std::string(S3Properties::kEndpoint), "http://localhost:9000"},
{std::string(S3Properties::kSslEnabled), "true"}});
{std::string(S3Properties::kSslEnabled), "TRUE"}});
ASSERT_THAT(https, IsOk());
EXPECT_EQ(https->scheme, "https");

auto http = ConfigureS3Options(
{{std::string(S3Properties::kEndpoint), "https://localhost:9000"},
{std::string(S3Properties::kSslEnabled), "false"}});
{std::string(S3Properties::kSslEnabled), "FaLsE"}});
ASSERT_THAT(http, IsOk());
EXPECT_EQ(http->scheme, "http");
}

TEST_F(ArrowS3FileIOTest, PathStyleAccess) {
auto virtual_addressing =
ConfigureS3Options({{std::string(S3Properties::kPathStyleAccess), "false"}});
ConfigureS3Options({{std::string(S3Properties::kPathStyleAccess), "FALSE"}});
ASSERT_THAT(virtual_addressing, IsOk());
EXPECT_TRUE(virtual_addressing->force_virtual_addressing);

auto path_style =
ConfigureS3Options({{std::string(S3Properties::kPathStyleAccess), "true"}});
ConfigureS3Options({{std::string(S3Properties::kPathStyleAccess), "TrUe"}});
ASSERT_THAT(path_style, IsOk());
EXPECT_FALSE(path_style->force_virtual_addressing);
}
Expand Down
14 changes: 14 additions & 0 deletions src/iceberg/test/config_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -126,4 +126,18 @@ TEST(ConfigTest, BasicOperations) {
ASSERT_EQ(config.Get(TestConfig::kDoubleConfig), 3.14);
}

TEST(ConfigTest, ParseBooleanIgnoringCase) {
auto config = TestConfig::default_properties();

for (const auto& value : {"true", "TRUE", "TrUe"}) {
config.mutable_configs()[TestConfig::kBoolConfig.key()] = value;
EXPECT_TRUE(config.Get(TestConfig::kBoolConfig));
}

for (const auto& value : {"false", "FALSE", "not-a-boolean"}) {
config.mutable_configs()[TestConfig::kBoolConfig.key()] = value;
EXPECT_FALSE(config.Get(TestConfig::kBoolConfig));
}
}

} // namespace iceberg
2 changes: 1 addition & 1 deletion src/iceberg/util/config.h
View file
Open in desktop
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 val == "true";
return StringUtils::EqualsIgnoreCase(val, "true");

@evindj evindj Jul 25, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

pretty sure you did but it would ne nice to check for other places in the code base where similar comparison with literals is made.

@fallintoplace fallintoplace Jul 25, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Good call. I found the same case-sensitive parsing in the S3 properties and pushed a follow-up for s3.ssl.enabled and s3.path-style-access. The other literal comparisons I found are Avro schema attributes rather than configuration properties, so I left those unchanged.

} 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
Loading