feat: add TOS object store support via OpenDAL#7019
Conversation
|
ACTION NEEDED 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. |
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
0796a4d to
cf4e283
Compare
a72a70e to
a9f9678
Compare
|
@Xuanwo Could you please help review this? Appreciate it. |
|
@claude review |
|
|
||
| const DEFAULT_LOCAL_BLOCK_SIZE: usize = 4 * 1024; // 4KB block size | ||
| #[cfg(any(feature = "aws", feature = "gcp", feature = "azure"))] | ||
| #[cfg(any(feature = "aws", feature = "gcp", feature = "azure", feature = "tos"))] |
There was a problem hiding this comment.
🟣 [Pre-existing] The #[cfg] gate for DEFAULT_CLOUD_BLOCK_SIZE at rust/lance-io/src/object_store.rs:64 only enables it for aws/gcp/azure/tos, but providers/oss.rs, providers/tencent.rs, and providers/huggingface.rs also import and use this constant. Building with --no-default-features --features oss (or just tencent or just huggingface) fails to compile with E0432: unresolved import crate::object_store::DEFAULT_CLOUD_BLOCK_SIZE. Since this PR touches this exact line to add tos, it'''s a good opportunity to backfill oss, tencent, and huggingface into the same gate.
Extended reasoning...
What the bug is
At rust/lance-io/src/object_store.rs:64, the constant DEFAULT_CLOUD_BLOCK_SIZE is defined under the cfg gate:
#[cfg(any(feature = "aws", feature = "gcp", feature = "azure", feature = "tos"))]
const DEFAULT_CLOUD_BLOCK_SIZE: usize = 64 * 1024;However, three other provider modules also import and use this constant from their new_store implementations:
rust/lance-io/src/object_store/providers/oss.rs:14(use crate::object_store::{..., DEFAULT_CLOUD_BLOCK_SIZE, ...}), used at line ~108 asparams.block_size.unwrap_or(DEFAULT_CLOUD_BLOCK_SIZE).rust/lance-io/src/object_store/providers/tencent.rs:12, used at line ~23.rust/lance-io/src/object_store/providers/huggingface.rs:16, used at line ~179.
Each of these provider modules is itself gated on its own feature in providers.rs (#[cfg(feature = "oss")] pub mod oss; etc.).
Why existing code doesn'''t prevent it
In CI and default builds, aws, gcp, or azure is almost always enabled, which keeps DEFAULT_CLOUD_BLOCK_SIZE defined and masks the issue. The cfg gate intersection becomes vacuous: in practice, anyone using oss/tencent/huggingface is also using one of the gated features.
Step-by-step proof
- Edit
rust/lance-io/Cargo.tomlto remove the default of["aws", "azure", "gcp"]or simply run with--no-default-features. - Run:
cargo check --no-default-features --features ossinrust/lance-io. - Compilation fails with
error[E0432]: unresolved import crate::object_store::DEFAULT_CLOUD_BLOCK_SIZE. The compiler points atproviders/oss.rs:14(theusesite) and the cfg-disabled definition atobject_store.rs:64. - The same failure occurs with
--features tencentand--features huggingface.
Impact
Anyone packaging Lance with a minimal feature set — for instance, only oss to support Alibaba Cloud users, or only huggingface for HF integrations — will hit a hard build failure. This is exactly the kind of compile-time matrix gap that the cfg system is meant to prevent.
How to fix
Extend the cfg list to include the other three opendal-based features that already use the constant:
#[cfg(any(
feature = "aws",
feature = "gcp",
feature = "azure",
feature = "oss",
feature = "tencent",
feature = "huggingface",
feature = "tos",
))]
const DEFAULT_CLOUD_BLOCK_SIZE: usize = 64 * 1024;Severity rationale
This is pre-existing — oss, tencent, and huggingface were already omitted from the gate before this PR. The PR only added tos. However, since the PR touches this exact line and copies the same incomplete pattern (adding tos but missing the other three opendal providers), it'''s a natural opportunity to fix in passing.
a9f9678 to
fa0c9c8
Compare
b77cbaa to
6ea2998
Compare
What changed
Add Volcengine TOS (
tos://) object store support through OpenDAL.tos://bucket/path.tosfeature tolance-ioand enable it by default throughrust/lance.TOS_/VOLCENGINE_environment variables andtos_*storage options.Testing
Validated against a real Volcengine TOS object store.