Conversation
manuzhang
force-pushed
the
agent/parse-boolean-util
branch
from
September 21, 2026 07:23
b9aa294 to
17b40e4
Compare
manuzhang
force-pushed
the
agent/parse-boolean-util
branch
from
September 21, 2026 07:29
17b40e4 to
e96193c
Compare
manuzhang
force-pushed
the
agent/parse-boolean-util
branch
from
September 21, 2026 07:38
e96193c to
207c159
Compare
manuzhang
force-pushed
the
agent/parse-boolean-util
branch
from
September 21, 2026 07:43
207c159 to
2edf780
Compare
manuzhang
force-pushed
the
agent/parse-boolean-util
branch
from
September 21, 2026 08:03
2edf780 to
d479f23
Compare
manuzhang
changed the title
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) <noreply@anthropic.com>
manuzhang
force-pushed
the
agent/parse-boolean-util
branch
from
September 21, 2026 08:24
d479f23 to
be4cb1d
Compare
wgtmac
approved these changes
Sep 21, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
Adds three boolean helpers, named and shaped after their Java counterparts, and reads every boolean through them:
StringUtils::ParseBoolean(std::string_view) -> bool— mirrorsBoolean.parseBoolean.PropertyUtil::PropertyAsBoolean(properties, key, default_value) -> bool— mirrorsPropertyUtil.propertyAsBoolean.PropertyUtil::PropertyAsOptionalBoolean(properties, key) -> std::optional<bool>— mirrorsPropertyUtil.propertyAsNullableBoolean, keeping an unset property distinct from an explicit"false", which the S3 path needs.Updated call sites:
arrow_s3_file_io.cc,util/config.h,auth/auth_managers.cc,rest_catalog.cc.avro_schema_util.cccompares an Avro node attribute (adjust-to-utc) rather than a user-supplied property, so it is left alone.Why
StringUtilscovered only numbers —ParseNumber<T>even excludesbool— so every site spelled out the"true"/"false"literals itself, and the four sites did not agree on what a malformed value meant:arrow_s3_file_io.cc(ParseOptionalBool)util/config.h(DefaultFromString<bool>)auth/auth_managers.cc(rest.sigv4-enabled)rest_catalog.cc(rest-metrics-reporting-enabled)Java has one contract for all of these, so this settles the codebase on it.
Behavior change
"true"ignoring case is true and every other value is false, everywhere.s3.path-style-accessands3.ssl.enabledare the sites that change. A malformed value such ass3.ssl.enabled=tureused to fail the FileIO build withkInvalidArgument; it now reads as false. That matches Java, whereS3FileIOPropertiesreads both properties withPropertyUtil.propertyAsBoolean. A typo in an S3 connection property is therefore no longer reported.The other three sites were already lenient and are unchanged.
Testing
StringUtilsTest.ParseBoolean— accepted spellings, and everything else reading as false.property_util_test.cc— defaulting when absent, case-insensitivity, and non-boolean values reading as false rather than falling back to the default.ArrowS3FileIOTest.InvalidBooleanPropertyReadsAsFalse— replacesRejectsInvalidBooleanProperties, which pinned the old behavior.ConfigTest.ParseBooleanIgnoringCase— unchanged and still passing, pinning the lenient contract for everyEntry<bool>.🤖 Generated with Claude Code