Repository navigation
fix(cloud): parse OpenDAL boolean properties the way object_store does - #9782
Merged
robert3005 merged 2 commits intoSep 10, 2026
Merged
Conversation
`cos` and `oss` compared the raw property string against `"true"`, so `True`, `1` and `yes` — all accepted by `object_store` for its own configuration, and by the `allow_http` sibling in this crate — read as `false` without a warning. Add a shared `property_as_bool` next to `property_or_env` that trims, lowercases and matches `object_store`'s set, and warns on a value that is not a boolean instead of silently answering `false`. Signed-off-by: jackylee-ch <qcsd2011@gmail.com>
robert3005
reviewed
Sep 9, 2026
The `property_as_bool` cases cover the parsing; the cos and oss tests only asserted that each call site reaches it, which is a one-line change per service. Signed-off-by: jackylee-ch <qcsd2011@gmail.com>
robert3005
approved these changes
Sep 10, 2026
robert3005
enabled auto-merge (squash)
September 10, 2026 11:17
Merging this PR will improve performance by 12.97%
|
| Mode | Benchmark | BASE |
HEAD |
Efficiency | |
|---|---|---|---|---|---|
| ⚡ | WallTime | mul_u32_nonnull_avx512 |
6.2 µs | 5.5 µs | +12.97% |
Tip
Curious why performance improved? Comment @codspeedbot explain why performance improved on this PR, or directly use the CodSpeed MCP with your agent.
Comparing jackylee-ch:fix/opendal-boolean-properties (553b455) with develop (e3b8eb2)
Footnotes
-
218 benchmarks were skipped, so the baseline results were used instead. If they were deleted from the codebase, click here and archive them to remove them from the performance reports. ↩
robert3005
pushed a commit
that referenced
this pull request
Oct 9, 2026
The Hugging Face store read `ALLOW_HTTP` as true only for `true`/`1` (`hf/mod.rs:390`), while object_store itself — and vortex-cloud's own `property_as_bool` for the OpenDAL stores (`opendal/mod.rs:213`) — accept `1`/`true`/`on`/`yes`/`y` case-insensitively. So `ALLOW_HTTP=on`, which works for every other scheme, left HF requiring HTTPS and a plain-HTTP Hub endpoint unreachable. #9782 aligned the OpenDAL side to object_store but left HF behind. ## Fix Pull the object_store boolean spellings into one `parse_object_store_bool` helper and route both `allow_http` and `property_as_bool` through it, so the two cannot drift. `property_as_bool`'s behaviour (including the warn-on-non-boolean) is unchanged. ## Tests Adds HF cases in `hf/tests.rs` for the previously-rejected spellings; the `on`/`yes`/`y` cases fail before the change and pass after. `cargo test -p vortex-cloud --features hf`, clippy, and `cargo +nightly fmt --check` are clean. ## AI assistance Written with agentic AI assistance; I read all three boolean-parsing sites and reproduced the `on`/`yes` rejection before and after. Signed-off-by: jackylee-ch <qcsd2011@gmail.com>
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
2 participants
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.
cos.rsandoss.rscompared the property string against"true"exactly, soTrue,1andyesread asfalse.object_storeaccepts all of those for its own configuration(
config.rs,impl Parse for bool:1/true/on/yes/y, lowercased), and so does theallow_httpsibling inhf/mod.rs— only the two OpenDAL services did not.Values reach
make_opendal_storeverbatim from the caller's property map, andwarn_on_unknown_propertiesonly warns about unknown keys, so an unusable value was silent:skip_signature = "True"keeps signing a public bucket (403), anddisable_config_load = "True"leaves OpenDAL loading the ambient credentials the caller asked to turn off.
The new shared
property_as_booltrims, lowercases, matchesobject_store's set, and warnsinstead of answering
false.goosefshas no boolean property.Tests
cargo test --release -p vortex-cloud --features cos,oss,goosefs,hf,registry: 101 passed, 84 ondevelop; feature matrix checked one flag at a time. Restoring the exact"true"compare fails7 of the new cases.
AI assistance
Written with agentic AI assistance; I read
object_store'sParse for boolin the vendored0.13.2 source before matching it.