tustvold commented on code in PR #4573:
URL: https://github.com/apache/arrow-rs/pull/4573#discussion_r1289143355
##########
object_store/tests/azure_object_store.rs:
##########
@@ -0,0 +1,114 @@
+use async_trait::async_trait;
Review Comment:
I think we should probably remove this before merge, but thank you for
creating it to show how you are expecting to interact with this system
##########
object_store/tests/azure_object_store.rs:
##########
@@ -0,0 +1,114 @@
+use async_trait::async_trait;
+use bytes::Bytes;
+use futures::stream::BoxStream;
+use object_store::azure::{MicrosoftAzureBuilder, MicrosoftAzure};
+use object_store::path::Path;
+use object_store::{
+ GetOptions, GetResult, ListResult, MultipartId, ObjectMeta, ObjectStore,
+};
+use std::fmt::Formatter;
+use tokio::io::AsyncWrite;
+
+#[derive(Debug)]
+struct AzureStore(MicrosoftAzure);
+
+impl std::fmt::Display for AzureStore {
+ fn fmt(&self, _: &mut Formatter<'_>) -> std::fmt::Result {
+ todo!()
+ }
+}
+
+#[async_trait]
+impl ObjectStore for AzureStore {
+ async fn put(&self, path: &Path, data: Bytes) -> object_store::Result<()> {
+ self.0.put(path, data).await
+ }
+
+ async fn put_multipart(
+ &self,
+ _: &Path,
+ ) -> object_store::Result<(MultipartId, Box<dyn AsyncWrite + Unpin +
Send>)> {
+ todo!()
+ }
+
+ async fn abort_multipart(
+ &self,
+ _: &Path,
+ _: &MultipartId,
+ ) -> object_store::Result<()> {
+ todo!()
+ }
+
+ async fn get_opts(
+ &self,
+ location: &Path,
+ options: GetOptions,
+ ) -> object_store::Result<GetResult> {
+ self.0.get_opts(location, options).await
+ }
+
+ async fn head(&self, _: &Path) -> object_store::Result<ObjectMeta> {
+ todo!()
+ }
+
+ async fn delete(&self, _: &Path) -> object_store::Result<()> {
+ todo!()
+ }
+
+ async fn list(
+ &self,
+ _: Option<&Path>,
+ ) -> object_store::Result<BoxStream<'_, object_store::Result<ObjectMeta>>>
{
+ todo!()
+ }
+
+ async fn list_with_delimiter(
+ &self,
+ _: Option<&Path>,
+ ) -> object_store::Result<ListResult> {
+ todo!()
+ }
+
+ async fn copy(&self, _: &Path, _: &Path) -> object_store::Result<()> {
+ todo!()
+ }
+
+ async fn copy_if_not_exists(&self, _: &Path, _: &Path) ->
object_store::Result<()> {
+ todo!()
+ }
+}
+
+
+#[tokio::test]
+async fn test_fabric() {
+
//Format:https://onelake.dfs.fabric.microsoft.com/<workspaceGUID>/<itemGUID>/Files/test.csv
+
//Example:https://onelake.dfs.fabric.microsoft.com/86bc63cf-5086-42e0-b16d-6bc580d1dc87/17d3977c-d46e-4bae-8fed-ff467e674aed/Files/SampleCustomerList.csv
+ //Account Name : onelake
+ //Container Name : workspaceGUID
+
+ let daily_store = AzureStore(
+ MicrosoftAzureBuilder::new()
+ .with_container_name("86bc63cf-5086-42e0-b16d-6bc580d1dc87")
+ .with_account("onelake")
+ .with_bearer_token_authorization("jwt-token")
+ .build()
+ .unwrap());
+
+ let path =
Path::from("17d3977c-d46e-4bae-8fed-ff467e674aed/Files/SampleCustomerList.csv");
Review Comment:
This is the bit that I found rather surprising, I would have naively thought
that the corollary for a container would be the files within an item, i.e. the
user would just specify `SampleCustomerList.csv` in the path, with everything
else provided at construction time of the builder.
This does appear to be what the docs suggest, so :shrug: To confirm if I
were to perform a List of `17d3977c-d46e-4bae-8fed-ff467e674aed/Files/` it
would return the full path, i.e.
`17d3977c-d46e-4bae-8fed-ff467e674aed/Files/SampleCustomerList.csv` and not
`SampleCustomerList.csv`?
##########
object_store/src/azure/mod.rs:
##########
@@ -911,7 +928,13 @@ impl MicrosoftAzureBuilder {
(true, url, credential, account_name)
} else {
let account_name = self.account_name.ok_or(Error::MissingAccount
{})?;
- let account_url = format!("https://{}.blob.core.windows.net";,
&account_name);
+ let account_url = if account_name.contains("onelake") {
Review Comment:
What happens if a user has a regular storage account called onelake or
containing within its name onelake, this will break it. I think we probably
need some sort of `with_use_fabric_endpoint`, to explicitly gate this. This
could then be set appropriately on URL extraction. What do you think?
##########
object_store/src/azure/mod.rs:
##########
@@ -889,7 +896,17 @@ impl MicrosoftAzureBuilder {
self.parse_url(&url)?;
}
- let container = self.container_name.ok_or(Error::MissingContainerName
{})?;
+ let use_ok_or = match &self.account_name {
+ Some(account_name) => !account_name.contains("onelake"),
+ None => true,
+ };
+
+ let container = if use_ok_or {
+ self.container_name.ok_or(Error::MissingContainerName {})?
+ } else {
+ self.container_name.unwrap_or_default()
+ };
Review Comment:
What is the motivation for this change, AFAICT a container name is necessary
to specify the workspace name?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
