Skip to content

refactor: replace hand-rolled string→bool parsing with str_is_truthy/str_to_bool - #8675

Open
wjones127 with Copilot wants to merge 3 commits into
mainfrom
copilot/replace-string-to-boolean-parsing
Open

refactor: replace hand-rolled string→bool parsing with str_is_truthy/str_to_bool#8675
wjones127 with Copilot wants to merge 3 commits into
mainfrom
copilot/replace-string-to-boolean-parsing

Conversation

Copilot AI commented Aug 20, 2026

Copy link
Copy Markdown
Contributor

Consolidates scattered string-to-boolean parsing across the codebase into lance_core::utils::parse utilities, eliminating inconsistent ad-hoc implementations.

Changes

  • New utility: str_to_bool(val: &str) -> Option<bool> — for cases where an unrecognized value should return None so a caller-side default takes effect. Truthy: 1/true/on/yes/y; falsy: 0/false/off/no/n; everything else: None.

  • Replaced hand-rolled patterns in 16 files across lance-core, lance-encoding, lance-index, lance-io, lance-table, lance-namespace-impls, and lance crates:

    • v == "true" / v.to_lowercase() == "true" / v.eq_ignore_ascii_case("true")str_is_truthy(v)
    • matches!(s.to_lowercase().as_str(), "true" | "1" | "yes")str_is_truthy(s)
    • v.parse::<bool>().ok() (config properties) → str_to_bool(v)
    • !matches!(v, "" | "0" | "false" | "off" | "no")str_is_truthy(v.trim())
  • Added unit tests for str_to_bool covering truthy, falsy, and unrecognized inputs.

Intentionally unchanged

  • lance-arrow/src/schema.rslance-core depends on lance-arrow; importing back would create a circular dependency.
  • lance/src/dataset/fragment.rs — checks if a SQL predicate string is literally "true", a semantically distinct use case.
  • lance/src/index/vector/details.rs — uses a generic parse::<T: FromStr> helper, not a string-bool-specific path.

Add `str_to_bool` utility to `lance_core::utils::parse` and replace all
hand-rolled string-to-boolean parsing throughout the codebase with the
appropriate utility functions from `lance_core`.

- Add `str_to_bool(val: &str) -> Option<bool>` to lance-core parse utils
- Add unit tests for `str_to_bool`
- Replace hand-rolled bool parsing in:
  - lance-core/src/datatypes/field.rs (2 places)
  - lance-encoding/src/array_encoding/strategy.rs
  - lance-index/src/scalar/fmindex.rs
  - lance-index/src/vector/bq/storage.rs
  - lance-index/src/vector/v3/shuffler.rs
  - lance-io/src/object_store/providers/aws.rs (2 places)
  - lance-io/src/object_store/providers/azure.rs
  - lance-io/src/object_store/providers/gcp.rs
  - lance-namespace-impls/src/credentials.rs (2 places)
  - lance-namespace-impls/src/dir.rs (7 places)
  - lance-namespace-impls/src/rest.rs (2 places)
  - lance-table/src/transaction/manifest_build.rs
  - lance/benches/concurrent_append.rs
  - lance/benches/manifest_commit.rs (2 places)
  - lance/src/dataset/schema_evolution.rs

Co-authored-by: wjones127 <5488879+wjones127@users.noreply.github.com>
Copilot AI changed the title [WIP] Replace string to boolean parsing with lance_core::utils::parse::str_is_truthy chore: replace hand-rolled string→bool parsing with str_is_truthy/str_to_bool Aug 20, 2026
Copilot AI requested a review from wjones127 August 20, 2026 21:11
@github-actions github-actions Bot added A-index Vector index, linalg, tokenizer A-encoding Encoding, IO, file reader/writer A-namespace Namespace impls labels Aug 20, 2026
@github-actions

Copy link
Copy Markdown
Contributor

ACTION NEEDED
Lance follows the Conventional Commits specification for release automation.

The PR title and description are used as the merge commit message. Please update your PR title and description to match the specification.

For details on the error please inspect the "PR Title Check" action.

@wjones127
wjones127 marked this pull request as ready for review August 20, 2026 21:12
@wjones127 wjones127 changed the title chore: replace hand-rolled string→bool parsing with str_is_truthy/str_to_bool refactor: replace hand-rolled string→bool parsing with str_is_truthy/str_to_bool Aug 20, 2026
@wjones127

Copy link
Copy Markdown
Contributor

@copilot There is a failing test you need to addresss:

thread 'credentials::tests::test_has_credential_vendor_config' (31985) panicked at rust/lance-namespace-impls/src/credentials.rs:770:9:
assertion failed: !has_credential_vendor_config(&props)

…e") to fix failing test

Co-authored-by: wjones127 <5488879+wjones127@users.noreply.github.com>

Copilot AI commented Aug 20, 2026

Copy link
Copy Markdown
Contributor Author

@copilot There is a failing test you need to addresss:

...

Fixed in the latest commit. has_credential_vendor_config was originally using .eq_ignore_ascii_case("true") (only accepts "true"/"TRUE", not "yes" or other truthy aliases). Replacing it with str_is_truthy inadvertently expanded the accepted values, breaking the test. Reverted it back to the original .eq_ignore_ascii_case("true") check, and moved the str_is_truthy import inside the #[cfg(feature = "credential-vendor-aws")] function where it's still needed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

A-encoding Encoding, IO, file reader/writer A-index Vector index, linalg, tokenizer A-namespace Namespace impls

Projects

None yet

Development

Successfully merging this pull request may close these issues.

chore: replace string to boolean parsing with lance_core::utils::parse::str_is_truthy

2 participants