feat(storage-azblob): Add Azure Blob Storage support - #2908
Conversation
|
@CTTY PTAL, thanks. |
xanderbailey
left a comment
There was a problem hiding this comment.
Nice PR, sorry for the delay on review. Apart from the comment left on the config, this seems to closely follow the existing oss/gcs/azdls patterns.
Noting that the integration tests aren't going to run in CI but this is true for other storage traits also.
| use crate::utils::from_opendal_error; | ||
|
|
||
| /// Parse azblob.* prefixed configuration properties. | ||
| pub(crate) fn azblob_config_parse(mut properties: HashMap<String, String>) -> Result<AzblobConfig> { |
There was a problem hiding this comment.
Other backends include their own config struct something like:
pub struct AzblobConfig {
/// Endpoint URL.
#[builder(default, setter(strip_option, into))]
pub endpoint: Option<String>,
/// Account name.
#[builder(default, setter(strip_option, into))]
pub account_name: Option<String>,
/// Account key.
#[builder(default, setter(strip_option, into))]
pub account_key: Option<String>,
/// SAS token.
#[builder(default, setter(strip_option, into))]
pub sas_token: Option<String>,
}Do you think we could follow the same pattern here?
There was a problem hiding this comment.
Thanks for pointing that out, ACK
| /// Azure Blob Storage endpoint URL. | ||
| pub const AZBLOB_ENDPOINT: &str = "azblob.endpoint"; | ||
| /// Azure Blob Storage account name. | ||
| pub const AZBLOB_ACCOUNT_NAME: &str = "azblob.account-name"; |
There was a problem hiding this comment.
Can't find a java implementation for azblob so that isn't incorrect but I did notice something that doesn't quite seem correct about the azdls config (pre-existing not this PR). It seems that we're using adls.account-name compared to java's adls.auth.shared-key.account.name which I think causes issues when we get the config back from the catalog for vended credentials.
Maybe we need to bottom this out before we commit to this?
There was a problem hiding this comment.
Ah but this does match py-iceberg https://github.com/apache/iceberg-python/blob/main/pyiceberg/io/__init__.py#L89
There was a problem hiding this comment.
Okay Claude did a little audit for me and found that py-iceberg and java use GCS_SERVICE_HOST = "gcs.service.host" and we use gcs.service.path so it would seem we're inconsistent here.
There was a problem hiding this comment.
I took another look at this. The configuration required by Azblob is effectively the same as Azdls; the only difference is the property prefix (azblob.* vs. adls.*).
Would you prefer keeping the dedicated azblob.* keys, or following PyIceberg’s convention and reusing the existing adls.* keys?
There was a problem hiding this comment.
I suspect we do really want different clients to expect the same properties, having said that the fact that python accepts the same for both is maybe a little confusing to me but I’ve never used the older blob storage so I’m not sure how reasonable it is to couple the properties?
There was a problem hiding this comment.
In both PyArrow and Arrow C++, the logic is to automatically detect whether HNS is enabled and then use different APIs.
For example, in azurefs.cc:
ARROW_ASSIGN_OR_RAISE(
auto hns_support,
HierarchicalNamespaceSupport(adlfs_client));
However, OpenDAL cannot do this. Therefore, I think it is reasonable to keep the dedicated Azblob configuration here. The caller needs to explicitly specify whether the namespace is DLS or Blob.
Add a dedicated OpenDAL Azblob backend for Azure Blob Storage accounts without hierarchical namespaces. Support azblob://<container>/<path> locations with endpoint, account name, account key, and SAS token configuration. Enable selection through both OpenDalStorageFactory and OpenDalResolvingStorage while keeping existing ABFS and WASB locations routed to Azdls. Add unit coverage and an environment-gated integration test for file operations against a non-HNS Azure account. Signed-off-by: jiaqizho <jiaqi.zhou@zilliz.com>
Signed-off-by: jiaqizho <jiaqi.zhou@zilliz.com>
d36023c to
eb1431d
Compare
Which issue does this PR close?
Add a dedicated OpenDAL Azblob backend for Azure Blob Storage accounts without hierarchical namespaces.
What changes are included in this PR?
Support
azblob://<container>/<path>locations with endpoint, account name, account key, and SAS token configuration. Enable selection through both OpenDalStorageFactory and OpenDalResolvingStorage while keeping existing ABFS and WASB locations routed to Azdls.Are these changes tested?
Add unit coverage and an environment-gated integration test, and verify writes, metadata, full and range reads, deletes, and prefix deletes against a real Azure Blob Storage account with HNS disabled.