You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by GitBox <gi...@apache.org> on 2021/08/26 15:13:22 UTC
[GitHub] [arrow-datafusion] yjshen opened a new pull request #950: ObjectStore API to read from remote storage systems
yjshen opened a new pull request #950:
URL: https://github.com/apache/arrow-datafusion/pull/950
# Which issue does this PR close?
Closes #616 .
# Rationale for this change
Currently, we can only read files from LocalFS since we use std::fs in ParquetExec. It would be nice to add support to read files that reside on storage sources such as HDFS, Amazon S3, etc.
This PR substitute #811 by only adding in the ObjectStore interfaces, leave hooking these APIs up into the rest of the system / rewrite the existing data sources (like Parquet, etc) as separate follow-ups.
<!--
Why are you proposing this change? If this is already explained clearly in the issue then this section is not needed.
Explaining clearly why changes are proposed helps reviewers understand your changes and offer better suggestions for fixes.
-->
# What changes are included in this PR?
<!--
There is no need to duplicate the description in the issue here but it is sometimes worth providing a summary of the individual changes in this PR.
-->
Introduce `ObjectStore` API as an abstraction of the underlying storage systems, such as local filesystem, HDFS, S3, etc. And make the `ObjectStore` implementation pluggable through the `ObjectStoreRegistery` in Datafusion's `ExecutionContext`.
# Are there any user-facing changes?
<!--
If there are user-facing changes then we may require documentation to be updated before approving the PR.
-->
Users can provide implementations for the `ObjectStore` trait, register it into `ExecutionContext`'s registry, and run queries against data that resides in remote storage systems as well as local fs (the *only default implementation* of `ObjectStore`).
<!--
If there are any breaking changes to public APIs, please add the `api change` label.
-->
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] houqp edited a comment on pull request #950: ObjectStore API to read from remote storage systems
Posted by GitBox <gi...@apache.org>.
houqp edited a comment on pull request #950:
URL: https://github.com/apache/arrow-datafusion/pull/950#issuecomment-907984810
> I am still wondering how we could replicate the hive partitioning support for TableProviders relative to different file formats (ParquetTable, CsvFile, NdJsonFile).
For raw file tables like a folder of parquet, csv and ndjson files, I think the partition discovery and pruning logic should be agnostic to both file formats and object storages. We could implement this logic as a shared module, which gets invoked in ParquetTable, CsvFile and NdJsonFile table providers to resolve the filtered down object list. Then these table providers will pass down the object paths to corresponding format specific physical execution plans.
> Actually, same would apply for a HiveCatalogTable that could actually reference various file formats. It makes me feel that something is wrong with how the different abstractions are organized. This is being discussed in #133.
Given that hive catalog manages a collection of databases and tables with their corresponding metadata, I think CatalogProvider is probably the right abstraction for it. The Hive catalog provider could query hive metastore for a given schema and table name. It can then construct a format specific table provider based on hive metastore response.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] houqp commented on a change in pull request #950: ObjectStore API to read from remote storage systems
Posted by GitBox <gi...@apache.org>.
houqp commented on a change in pull request #950:
URL: https://github.com/apache/arrow-datafusion/pull/950#discussion_r698946471
##########
File path: datafusion/src/datasource/object_store/local.rs
##########
@@ -0,0 +1,173 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+//! Object store that represents the Local File System.
+
+use std::fs::Metadata;
+use std::sync::Arc;
+
+use async_trait::async_trait;
+use futures::{stream, AsyncRead, StreamExt};
+
+use crate::datasource::object_store::{
+ FileMeta, FileMetaStream, ObjectReader, ObjectStore,
+};
+use crate::error::DataFusionError;
+use crate::error::Result;
+
+#[derive(Debug)]
+/// Local File System as Object Store.
+pub struct LocalFileSystem;
+
+#[async_trait]
+impl ObjectStore for LocalFileSystem {
+ async fn list(
+ &self,
+ prefix: &str,
+ _delimiter: Option<String>,
+ ) -> Result<FileMetaStream> {
+ list_all(prefix.to_owned()).await
Review comment:
There might be value in having two list apis as well, one for listing objects without delimiters and returns stream of `FileMeta`, the other for listing "directories" with delimiters that could return either `FileMeta` or prefix strings.
I am thinking there are use-case where we only need to perform listing without delimiters. For example after we have partition "folders" fully discovered, we will just use list api call to get the final list of objects using partition prefixes. Returning a stream of `FileMeta` directly in this case could avoid enum matching overhead on the call site.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] rdettai commented on a change in pull request #950: ObjectStore API to read from remote storage systems
Posted by GitBox <gi...@apache.org>.
rdettai commented on a change in pull request #950:
URL: https://github.com/apache/arrow-datafusion/pull/950#discussion_r697226770
##########
File path: datafusion/src/datasource/object_store/mod.rs
##########
@@ -0,0 +1,127 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+//! Object Store abstracts access to an underlying file/object storage.
+
+pub mod local;
+
+use std::collections::HashMap;
+use std::fmt::Debug;
+use std::pin::Pin;
+use std::sync::{Arc, RwLock};
+
+use async_trait::async_trait;
+use futures::{AsyncRead, Stream};
+
+use local::LocalFileSystem;
+
+use crate::error::{DataFusionError, Result};
+use chrono::Utc;
+
+/// Object Reader for one file in a object store
+#[async_trait]
+pub trait ObjectReader {
+ /// Get reader for a part [start, start + length] in the file asynchronously
+ async fn get_reader(&self, start: u64, length: usize) -> Result<Arc<dyn AsyncRead>>;
+
+ /// Get length for the file asynchronously
+ async fn length(&self) -> Result<u64>;
+}
+
+/// File meta we got from object store
+pub struct FileMeta {
+ /// Path of the file
+ pub path: String,
+ /// Last time the file was modified in UTC
+ pub last_modified: Option<chrono::DateTime<Utc>>,
+ /// File size in total
+ pub size: Option<u64>,
Review comment:
You are right that technically the S3 API says the `size` field is optional in the list response. I couldn't find why. It might make sense when the `Object` is a prefix and not an actual object (for exemple if you list the memsql-tpch-dataset bucket, you will get entries for `sf_100/`, `sf_100/customer/`...), but actually with Rusoto, even in that case the `size` field is set to `Some(0)` 🙃.
My argument is that _we can expect from any system that provides the list of filenames to also reliably provide the file sizes_. With this assumption, avoiding an optional field helps simplifying consumers that do use that information. For instance for parquet you will need to read from the end. As the `ObjectReader` works using the (start, length) tuple, we critically need the length to get the end of the file. This means that if the `size` provided from the listing is optional, every implementation of `ObjectReader.length` will need to have a branch with the case where the size was provided, in which case it uses it directly, and another branch where it fetches the size from the store. I find this adds some complexity.
For me the true question is whether the assumption _"we can expect from any system that provides the list of filenames to also reliably provide the file sizes"_ holds. I am pretty sure it does, but if you don't think so we can definitively pay the price of `Option<>` 😉
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] houqp commented on a change in pull request #950: ObjectStore API to read from remote storage systems
Posted by GitBox <gi...@apache.org>.
houqp commented on a change in pull request #950:
URL: https://github.com/apache/arrow-datafusion/pull/950#discussion_r699474709
##########
File path: datafusion/src/datasource/object_store/local.rs
##########
@@ -0,0 +1,173 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+//! Object store that represents the Local File System.
+
+use std::fs::Metadata;
+use std::sync::Arc;
+
+use async_trait::async_trait;
+use futures::{stream, AsyncRead, StreamExt};
+
+use crate::datasource::object_store::{
+ FileMeta, FileMetaStream, ObjectReader, ObjectStore,
+};
+use crate::error::DataFusionError;
+use crate::error::Result;
+
+#[derive(Debug)]
+/// Local File System as Object Store.
+pub struct LocalFileSystem;
+
+#[async_trait]
+impl ObjectStore for LocalFileSystem {
+ async fn list(
+ &self,
+ prefix: &str,
+ _delimiter: Option<String>,
+ ) -> Result<FileMetaStream> {
+ list_all(prefix.to_owned()).await
Review comment:
@alamb that's interesting, do you have any opinion on the design here?
Since list without delimiter always returns object keys, not logical directory keys, I feel like returning `FileMeta` in list call provides a better more statically typed interface for callers.
In IOx, do you remember what was the reason for going back to `ObjectStorePath` as return type for regular list calls?
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] alamb commented on a change in pull request #950: ObjectStore API to read from remote storage systems
Posted by GitBox <gi...@apache.org>.
alamb commented on a change in pull request #950:
URL: https://github.com/apache/arrow-datafusion/pull/950#discussion_r697681701
##########
File path: datafusion/src/datasource/object_store/mod.rs
##########
@@ -0,0 +1,130 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+//! Object Store abstracts access to an underlying file/object storage.
+
+pub mod local;
+
+use std::collections::HashMap;
+use std::fmt::Debug;
+use std::pin::Pin;
+use std::sync::{Arc, RwLock};
+
+use async_trait::async_trait;
+use futures::{AsyncRead, Stream};
+
+use local::LocalFileSystem;
+
+use crate::error::{DataFusionError, Result};
+use crate::logical_plan::Expr;
+use chrono::Utc;
+
+/// Object Reader for one file in a object store
+#[async_trait]
+pub trait ObjectReader {
+ /// Get reader for a part [start, start + length] in the file asynchronously
+ async fn get_reader(&self, start: u64, length: usize) -> Result<Arc<dyn AsyncRead>>;
+
+ /// Get length for the file
+ fn length(&self) -> u64;
+}
+
+/// File meta we got from object store
+pub struct FileMeta {
+ /// Path of the file
+ pub path: String,
+ /// Last time the file was modified in UTC
+ pub last_modified: Option<chrono::DateTime<Utc>>,
+ /// File size in total
+ pub size: u64,
+}
+
+/// Stream of files get listed from object store
+pub type FileMetaStream =
+ Pin<Box<dyn Stream<Item = Result<FileMeta>> + Send + Sync + 'static>>;
+
+/// A ObjectStore abstracts access to an underlying file/object storage.
+/// It maps strings (e.g. URLs, filesystem paths, etc) to sources of bytes
+#[async_trait]
+pub trait ObjectStore: Sync + Send + Debug {
+ /// Returns all the files in path `prefix` asynchronously, implementations could
+ /// choose to use the pushed down filters for list filtering.
+ async fn list(&self, prefix: &str, filters: &[Expr]) -> Result<FileMetaStream>;
Review comment:
```suggestion
/// Returns all the files in path `prefix` asynchronously, with optional filters.
///
/// `filters` are optional expressions that may have been pushed down
/// during scans as an optional optimization that can be used to reduce
/// returned results. Implementations of
/// ObjectStore are *not* required to do anything with optional_filters.
async fn list(&self, prefix: &str, optional_filters: Option<&[Expr]>) -> Result<FileMetaStream>;
```
##########
File path: datafusion/src/datasource/object_store/local.rs
##########
@@ -0,0 +1,127 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+//! Object store that represents the Local File System.
+
+use std::fs::Metadata;
+use std::sync::Arc;
+
+use async_trait::async_trait;
+use futures::{stream, AsyncRead, StreamExt};
+
+use crate::datasource::object_store::{
+ FileMeta, FileMetaStream, ObjectReader, ObjectStore,
+};
+use crate::error::DataFusionError;
+use crate::error::Result;
+use crate::logical_plan::Expr;
+
+#[derive(Debug)]
+/// Local File System as Object Store.
+pub struct LocalFileSystem;
+
+#[async_trait]
+impl ObjectStore for LocalFileSystem {
+ async fn list(&self, prefix: &str, _filters: &[Expr]) -> Result<FileMetaStream> {
+ list_all(prefix.to_owned()).await
+ }
+
+ fn get_reader(&self, file: FileMeta) -> Result<Arc<dyn ObjectReader>> {
+ Ok(Arc::new(LocalFileReader::new(file)?))
+ }
+}
+
+struct LocalFileReader {
+ file: FileMeta,
+}
+
+impl LocalFileReader {
+ fn new(file: FileMeta) -> Result<Self> {
+ Ok(Self { file })
+ }
+}
+
+#[async_trait]
+impl ObjectReader for LocalFileReader {
+ async fn get_reader(
+ &self,
+ _start: u64,
+ _length: usize,
+ ) -> Result<Arc<dyn AsyncRead>> {
+ todo!()
+ }
+
+ fn length(&self) -> u64 {
+ self.file.size
+ }
+}
+
+async fn list_all(prefix: String) -> Result<FileMetaStream> {
+ fn get_meta(path: String, metadata: Metadata) -> FileMeta {
+ FileMeta {
+ path,
+ last_modified: metadata.modified().map(chrono::DateTime::from).ok(),
+ size: metadata.len(),
+ }
+ }
+
+ async fn find_files_in_dir(
+ path: String,
+ to_visit: &mut Vec<String>,
+ ) -> Result<Vec<FileMeta>> {
+ let mut dir = tokio::fs::read_dir(path).await?;
+ let mut files = Vec::new();
+
+ while let Some(child) = dir.next_entry().await? {
+ if let Some(child_path) = child.path().to_str() {
+ let metadata = child.metadata().await?;
+ if metadata.is_dir() {
+ to_visit.push(child_path.to_string());
+ } else {
+ files.push(get_meta(child_path.to_owned(), metadata))
+ }
+ } else {
+ return Err(DataFusionError::Plan("Invalid path".to_string()));
+ }
+ }
+ Ok(files)
+ }
+
+ let prefix_meta = tokio::fs::metadata(&prefix).await?;
+ let prefix = prefix.to_owned();
+ if prefix_meta.is_file() {
+ Ok(Box::pin(stream::once(async move {
+ Ok(get_meta(prefix, prefix_meta))
+ })))
+ } else {
+ let result = stream::unfold(vec![prefix], move |mut to_visit| async move {
+ match to_visit.pop() {
+ None => None,
+ Some(path) => {
+ let file_stream = match find_files_in_dir(path, &mut to_visit).await {
+ Ok(files) => stream::iter(files).map(Ok).left_stream(),
+ Err(e) => stream::once(async { Err(e) }).right_stream(),
+ };
+
+ Some((file_stream, to_visit))
+ }
+ }
+ })
+ .flatten();
+ Ok(Box::pin(result))
+ }
+}
Review comment:
At some point tests for the code in `local.rs` might be cool.
##########
File path: datafusion/src/datasource/object_store/local.rs
##########
@@ -0,0 +1,127 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+//! Object store that represents the Local File System.
+
+use std::fs::Metadata;
+use std::sync::Arc;
+
+use async_trait::async_trait;
+use futures::{stream, AsyncRead, StreamExt};
+
+use crate::datasource::object_store::{
+ FileMeta, FileMetaStream, ObjectReader, ObjectStore,
+};
+use crate::error::DataFusionError;
+use crate::error::Result;
+use crate::logical_plan::Expr;
+
+#[derive(Debug)]
+/// Local File System as Object Store.
+pub struct LocalFileSystem;
+
+#[async_trait]
+impl ObjectStore for LocalFileSystem {
+ async fn list(&self, prefix: &str, _filters: &[Expr]) -> Result<FileMetaStream> {
+ list_all(prefix.to_owned()).await
+ }
+
+ fn get_reader(&self, file: FileMeta) -> Result<Arc<dyn ObjectReader>> {
+ Ok(Arc::new(LocalFileReader::new(file)?))
+ }
+}
+
+struct LocalFileReader {
+ file: FileMeta,
+}
+
+impl LocalFileReader {
+ fn new(file: FileMeta) -> Result<Self> {
+ Ok(Self { file })
+ }
+}
+
+#[async_trait]
+impl ObjectReader for LocalFileReader {
+ async fn get_reader(
Review comment:
I don't understand this comment -- `ObjectStore::get_reader` is already `async` isn't it?
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] houqp commented on a change in pull request #950: ObjectStore API to read from remote storage systems
Posted by GitBox <gi...@apache.org>.
houqp commented on a change in pull request #950:
URL: https://github.com/apache/arrow-datafusion/pull/950#discussion_r697158713
##########
File path: datafusion/src/datasource/object_store/mod.rs
##########
@@ -0,0 +1,127 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+//! Object Store abstracts access to an underlying file/object storage.
+
+pub mod local;
+
+use std::collections::HashMap;
+use std::fmt::Debug;
+use std::pin::Pin;
+use std::sync::{Arc, RwLock};
+
+use async_trait::async_trait;
+use futures::{AsyncRead, Stream};
+
+use local::LocalFileSystem;
+
+use crate::error::{DataFusionError, Result};
+use chrono::Utc;
+
+/// Object Reader for one file in a object store
+#[async_trait]
+pub trait ObjectReader {
+ /// Get reader for a part [start, start + length] in the file asynchronously
+ async fn get_reader(&self, start: u64, length: usize) -> Result<Arc<dyn AsyncRead>>;
+
+ /// Get length for the file asynchronously
+ async fn length(&self) -> Result<u64>;
+}
+
+/// File meta we got from object store
+pub struct FileMeta {
+ /// Path of the file
+ pub path: String,
+ /// Last time the file was modified in UTC
+ pub last_modified: Option<chrono::DateTime<Utc>>,
+ /// File size in total
+ pub size: Option<u64>,
+}
+
+/// Stream of files get listed from object store
+pub type FileMetaStream =
+ Pin<Box<dyn Stream<Item = Result<FileMeta>> + Send + Sync + 'static>>;
+
+/// A ObjectStore abstracts access to an underlying file/object storage.
+/// It maps strings (e.g. URLs, filesystem paths, etc) to sources of bytes
+#[async_trait]
+pub trait ObjectStore: Sync + Send + Debug {
+ /// Returns all the files in path `prefix` asynchronously
+ async fn list(&self, prefix: &str) -> Result<FileMetaStream>;
+
+ /// Get object reader for one file
+ async fn get_reader(&self, file: FileMeta) -> Result<Arc<dyn ObjectReader>>;
+}
+
+static LOCAL_SCHEME: &str = "file";
+
+/// A Registry holds all the object stores at runtime with a scheme for each store.
+/// This allows the user to extend DataFusion with different storage systems such as S3 or HDFS
+/// and query data inside these systems.
+pub struct ObjectStoreRegistry {
+ /// A map from scheme to object store that serve list / read operations for the store
+ pub object_stores: RwLock<HashMap<String, Arc<dyn ObjectStore>>>,
+}
+
+impl ObjectStoreRegistry {
+ /// Create the registry that object stores can registered into.
+ /// ['LocalFileSystem'] store is registered in by default to support read local files natively.
+ pub fn new() -> Self {
+ let mut map: HashMap<String, Arc<dyn ObjectStore>> = HashMap::new();
+ map.insert(LOCAL_SCHEME.to_string(), Arc::new(LocalFileSystem));
+
+ Self {
+ object_stores: RwLock::new(map),
+ }
+ }
+
+ /// Adds a new store to this registry.
+ /// If a store of the same prefix existed before, it is replaced in the registry and returned.
+ pub fn register_store(
+ &self,
+ scheme: String,
+ store: Arc<dyn ObjectStore>,
+ ) -> Option<Arc<dyn ObjectStore>> {
+ let mut stores = self.object_stores.write().unwrap();
+ stores.insert(scheme, store)
+ }
+
+ /// Get the store registered for scheme
+ pub fn get(&self, scheme: &str) -> Option<Arc<dyn ObjectStore>> {
+ let stores = self.object_stores.read().unwrap();
+ stores.get(scheme).cloned()
+ }
+
+ /// Get a suitable store for the URI based on it's scheme. For example:
+ /// URI with scheme file or no schema will return the default LocalFS store,
+ /// URI with scheme s3 will return the S3 store if it's registered.
+ pub fn get_by_uri(&self, uri: &str) -> Result<Arc<dyn ObjectStore>> {
+ if let Some((scheme, _)) = uri.split_once(':') {
+ let stores = self.object_stores.read().unwrap();
+ if let Some(store) = stores.get(&*scheme.to_lowercase()) {
+ Ok(store.clone())
+ } else {
+ Err(DataFusionError::Internal(format!(
+ "No suitable object store found for {}",
+ scheme
+ )))
+ }
Review comment:
You can probably use ok_or_else to simplify the code:
```suggestion
stores.get(&*scheme.to_lowercase()).ok_or_else(|| DataFusionError::Internal(format!(
"No suitable object store found for {}",
scheme
)))
```
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] houqp commented on a change in pull request #950: ObjectStore API to read from remote storage systems
Posted by GitBox <gi...@apache.org>.
houqp commented on a change in pull request #950:
URL: https://github.com/apache/arrow-datafusion/pull/950#discussion_r698021907
##########
File path: datafusion/src/datasource/object_store/mod.rs
##########
@@ -0,0 +1,130 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+//! Object Store abstracts access to an underlying file/object storage.
+
+pub mod local;
+
+use std::collections::HashMap;
+use std::fmt::Debug;
+use std::pin::Pin;
+use std::sync::{Arc, RwLock};
+
+use async_trait::async_trait;
+use futures::{AsyncRead, Stream};
+
+use local::LocalFileSystem;
+
+use crate::error::{DataFusionError, Result};
+use chrono::Utc;
+
+/// Object Reader for one file in a object store
+#[async_trait]
+pub trait ObjectReader {
+ /// Get reader for a part [start, start + length] in the file asynchronously
+ async fn chunk_reader(&self, start: u64, length: usize)
+ -> Result<Arc<dyn AsyncRead>>;
+
+ /// Get length for the file
+ fn length(&self) -> u64;
+}
+
+/// File meta we got from object store
+#[derive(Debug)]
+pub struct FileMeta {
+ /// Path of the file
+ pub path: String,
+ /// Last time the file was modified in UTC
+ pub last_modified: Option<chrono::DateTime<Utc>>,
+ /// File size in total
+ pub size: u64,
+}
+
+/// Stream of files get listed from object store
+pub type FileMetaStream =
+ Pin<Box<dyn Stream<Item = Result<FileMeta>> + Send + Sync + 'static>>;
+
+/// A ObjectStore abstracts access to an underlying file/object storage.
+/// It maps strings (e.g. URLs, filesystem paths, etc) to sources of bytes
+#[async_trait]
+pub trait ObjectStore: Sync + Send + Debug {
+ /// Returns all the files in path `prefix` asynchronously.
+ async fn list(&self, prefix: &str) -> Result<FileMetaStream>;
+
+ /// Get object reader for one file
+ fn file_reader(&self, file: FileMeta) -> Result<Arc<dyn ObjectReader>>;
Review comment:
That's a good point @rdettai , we could keep it as a sync method to force lazy initialization. I also agree that sharing a single file descriptor with subsequent read_chunk calls would limit the concurrency to 1. This goes to my second comment in https://github.com/apache/arrow-datafusion/pull/950#discussion_r697552520: making self mut for `read_chunk` method, so we can implement these caching lazily.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] houqp commented on a change in pull request #950: ObjectStore API to read from remote storage systems
Posted by GitBox <gi...@apache.org>.
houqp commented on a change in pull request #950:
URL: https://github.com/apache/arrow-datafusion/pull/950#discussion_r697156961
##########
File path: datafusion/src/datasource/object_store/mod.rs
##########
@@ -0,0 +1,127 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+//! Object Store abstracts access to an underlying file/object storage.
+
+pub mod local;
+
+use std::collections::HashMap;
+use std::fmt::Debug;
+use std::pin::Pin;
+use std::sync::{Arc, RwLock};
+
+use async_trait::async_trait;
+use futures::{AsyncRead, Stream};
+
+use local::LocalFileSystem;
+
+use crate::error::{DataFusionError, Result};
+use chrono::Utc;
+
+/// Object Reader for one file in a object store
+#[async_trait]
+pub trait ObjectReader {
+ /// Get reader for a part [start, start + length] in the file asynchronously
+ async fn get_reader(&self, start: u64, length: usize) -> Result<Arc<dyn AsyncRead>>;
+
+ /// Get length for the file asynchronously
+ async fn length(&self) -> Result<u64>;
+}
+
+/// File meta we got from object store
+pub struct FileMeta {
+ /// Path of the file
+ pub path: String,
+ /// Last time the file was modified in UTC
+ pub last_modified: Option<chrono::DateTime<Utc>>,
+ /// File size in total
+ pub size: Option<u64>,
Review comment:
I remember running into similar issues (null object size) with Golang SDK in the past, but I couldn't remember exactly what triggered it.
What's interesting is the new official AWS SDK doesn't have size declared as optional anymore:
https://awslabs.github.io/aws-sdk-rust/aws_sdk_s3/model/struct.Object.html.
I think in general, if we expect the consumer of `FileMeta` needing to access `size` for core logic, we should declare it as non-optional and manage error propagation in the s3 object store's implementation. If it's used in non-core logic, then I think declaring it as optional is fine.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] houqp commented on a change in pull request #950: ObjectStore API to read from remote storage systems
Posted by GitBox <gi...@apache.org>.
houqp commented on a change in pull request #950:
URL: https://github.com/apache/arrow-datafusion/pull/950#discussion_r697540216
##########
File path: datafusion/src/datasource/object_store/mod.rs
##########
@@ -0,0 +1,127 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+//! Object Store abstracts access to an underlying file/object storage.
+
+pub mod local;
+
+use std::collections::HashMap;
+use std::fmt::Debug;
+use std::pin::Pin;
+use std::sync::{Arc, RwLock};
+
+use async_trait::async_trait;
+use futures::{AsyncRead, Stream};
+
+use local::LocalFileSystem;
+
+use crate::error::{DataFusionError, Result};
+use chrono::Utc;
+
+/// Object Reader for one file in a object store
+#[async_trait]
+pub trait ObjectReader {
+ /// Get reader for a part [start, start + length] in the file asynchronously
+ async fn get_reader(&self, start: u64, length: usize) -> Result<Arc<dyn AsyncRead>>;
+
+ /// Get length for the file asynchronously
+ async fn length(&self) -> Result<u64>;
+}
+
+/// File meta we got from object store
+pub struct FileMeta {
+ /// Path of the file
+ pub path: String,
+ /// Last time the file was modified in UTC
+ pub last_modified: Option<chrono::DateTime<Utc>>,
+ /// File size in total
+ pub size: Option<u64>,
Review comment:
It looks like all of local, s3, gcs, adls2 and hdfs returns object size on listing, so I think it's fairly safe to set it to non-optional for now. If we run into object stores that do not return size in response in the future, we can change it back to optional and refactor the code accordingly. For now, keeping it as non-optional does simplify the code.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] rdettai edited a comment on pull request #950: ObjectStore API to read from remote storage systems
Posted by GitBox <gi...@apache.org>.
rdettai edited a comment on pull request #950:
URL: https://github.com/apache/arrow-datafusion/pull/950#issuecomment-907769383
> I am under the assumption that TableProvider is responsible for parsing the partition parameters from the URI bucket/prefix?partition=year&partition=month and constructing the list object API calls with appropriate prefixes
In the case of hive partitioning, you need an extra capability of the object store to achieve that: **list the folders**. Otherwise you cannot now what partitions are there and build the appropriate prefixes. Maybe this example helps getting more context: https://docs.google.com/document/d/1nKXPvolft1_DuVjVK7ceOx37arCdQLKc_Im-TfZh9dk/edit?usp=sharing
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] houqp commented on a change in pull request #950: ObjectStore API to read from remote storage systems
Posted by GitBox <gi...@apache.org>.
houqp commented on a change in pull request #950:
URL: https://github.com/apache/arrow-datafusion/pull/950#discussion_r698945223
##########
File path: datafusion/src/datasource/object_store/local.rs
##########
@@ -0,0 +1,173 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+//! Object store that represents the Local File System.
+
+use std::fs::Metadata;
+use std::sync::Arc;
+
+use async_trait::async_trait;
+use futures::{stream, AsyncRead, StreamExt};
+
+use crate::datasource::object_store::{
+ FileMeta, FileMetaStream, ObjectReader, ObjectStore,
+};
+use crate::error::DataFusionError;
+use crate::error::Result;
+
+#[derive(Debug)]
+/// Local File System as Object Store.
+pub struct LocalFileSystem;
+
+#[async_trait]
+impl ObjectStore for LocalFileSystem {
+ async fn list(
+ &self,
+ prefix: &str,
+ _delimiter: Option<String>,
+ ) -> Result<FileMetaStream> {
+ list_all(prefix.to_owned()).await
Review comment:
naming is hard indeed :D
how about `ListEntry` or `ListItem`? we can define it as an enum of `FileMeta` and prefix string to distinguish between regular objects and "folder" prefixes.
References:
S3 has prefixes returned in the ` <CommonPrefixes>` section: https://docs.aws.amazon.com/AmazonS3/latest/API/API_ListObjectsV2.html
GCS has prefixes returned in response object's prefixes property: https://cloud.google.com/storage/docs/json_api/v1/objects/list#response
Azure blob is similar to S3 and has prefix strings returned in its `BlobPrefix` section: https://docs.microsoft.com/en-us/rest/api/storageservices/enumerating-blob-resources#DelimitedBlobList
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] rdettai commented on a change in pull request #950: ObjectStore API to read from remote storage systems
Posted by GitBox <gi...@apache.org>.
rdettai commented on a change in pull request #950:
URL: https://github.com/apache/arrow-datafusion/pull/950#discussion_r698039515
##########
File path: datafusion/src/datasource/object_store/mod.rs
##########
@@ -0,0 +1,130 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+//! Object Store abstracts access to an underlying file/object storage.
+
+pub mod local;
+
+use std::collections::HashMap;
+use std::fmt::Debug;
+use std::pin::Pin;
+use std::sync::{Arc, RwLock};
+
+use async_trait::async_trait;
+use futures::{AsyncRead, Stream};
+
+use local::LocalFileSystem;
+
+use crate::error::{DataFusionError, Result};
+use chrono::Utc;
+
+/// Object Reader for one file in a object store
+#[async_trait]
+pub trait ObjectReader {
+ /// Get reader for a part [start, start + length] in the file asynchronously
+ async fn chunk_reader(&self, start: u64, length: usize)
+ -> Result<Arc<dyn AsyncRead>>;
+
+ /// Get length for the file
+ fn length(&self) -> u64;
+}
+
+/// File meta we got from object store
+#[derive(Debug)]
+pub struct FileMeta {
+ /// Path of the file
+ pub path: String,
+ /// Last time the file was modified in UTC
+ pub last_modified: Option<chrono::DateTime<Utc>>,
+ /// File size in total
+ pub size: u64,
+}
+
+/// Stream of files get listed from object store
+pub type FileMetaStream =
+ Pin<Box<dyn Stream<Item = Result<FileMeta>> + Send + Sync + 'static>>;
+
+/// A ObjectStore abstracts access to an underlying file/object storage.
+/// It maps strings (e.g. URLs, filesystem paths, etc) to sources of bytes
+#[async_trait]
+pub trait ObjectStore: Sync + Send + Debug {
+ /// Returns all the files in path `prefix` asynchronously.
+ async fn list(&self, prefix: &str) -> Result<FileMetaStream>;
Review comment:
maybe simply supporting `/` is enough, that would make it easier to also implement the feature for the regular file system. In this setup, *delimiter mode* means that you only display the first level of the folder hierarchy!
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] yjshen commented on pull request #950: ObjectStore API to read from remote storage systems
Posted by GitBox <gi...@apache.org>.
yjshen commented on pull request #950:
URL: https://github.com/apache/arrow-datafusion/pull/950#issuecomment-911650226
@alamb What's your opinion on the current `list_file` and `list_dir` API?
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] yjshen commented on pull request #950: ObjectStore API to read from remote storage systems
Posted by GitBox <gi...@apache.org>.
yjshen commented on pull request #950:
URL: https://github.com/apache/arrow-datafusion/pull/950#issuecomment-908889969
Python test failure seems unrelated, could you please retrigger this single GitHub action @houqp ? Thanks!
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] houqp commented on a change in pull request #950: ObjectStore API to read from remote storage systems
Posted by GitBox <gi...@apache.org>.
houqp commented on a change in pull request #950:
URL: https://github.com/apache/arrow-datafusion/pull/950#discussion_r698035944
##########
File path: datafusion/src/datasource/object_store/mod.rs
##########
@@ -0,0 +1,130 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+//! Object Store abstracts access to an underlying file/object storage.
+
+pub mod local;
+
+use std::collections::HashMap;
+use std::fmt::Debug;
+use std::pin::Pin;
+use std::sync::{Arc, RwLock};
+
+use async_trait::async_trait;
+use futures::{AsyncRead, Stream};
+
+use local::LocalFileSystem;
+
+use crate::error::{DataFusionError, Result};
+use chrono::Utc;
+
+/// Object Reader for one file in a object store
+#[async_trait]
+pub trait ObjectReader {
+ /// Get reader for a part [start, start + length] in the file asynchronously
+ async fn chunk_reader(&self, start: u64, length: usize)
+ -> Result<Arc<dyn AsyncRead>>;
+
+ /// Get length for the file
+ fn length(&self) -> u64;
+}
+
+/// File meta we got from object store
+#[derive(Debug)]
+pub struct FileMeta {
+ /// Path of the file
+ pub path: String,
+ /// Last time the file was modified in UTC
+ pub last_modified: Option<chrono::DateTime<Utc>>,
+ /// File size in total
+ pub size: u64,
+}
+
+/// Stream of files get listed from object store
+pub type FileMetaStream =
+ Pin<Box<dyn Stream<Item = Result<FileMeta>> + Send + Sync + 'static>>;
+
+/// A ObjectStore abstracts access to an underlying file/object storage.
+/// It maps strings (e.g. URLs, filesystem paths, etc) to sources of bytes
+#[async_trait]
+pub trait ObjectStore: Sync + Send + Debug {
+ /// Returns all the files in path `prefix` asynchronously.
+ async fn list(&self, prefix: &str) -> Result<FileMetaStream>;
Review comment:
as @rdettai mentioned in his comment, I think we should add an optional delimiter argument here, this is a native feature supported by object stores in aws, gcp and azure.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] yjshen commented on a change in pull request #950: ObjectStore API to read from remote storage systems
Posted by GitBox <gi...@apache.org>.
yjshen commented on a change in pull request #950:
URL: https://github.com/apache/arrow-datafusion/pull/950#discussion_r696877841
##########
File path: datafusion/src/datasource/object_store/mod.rs
##########
@@ -0,0 +1,127 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+//! Object Store abstracts access to an underlying file/object storage.
+
+pub mod local;
+
+use std::collections::HashMap;
+use std::fmt::Debug;
+use std::pin::Pin;
+use std::sync::{Arc, RwLock};
+
+use async_trait::async_trait;
+use futures::{AsyncRead, Stream};
+
+use local::LocalFileSystem;
+
+use crate::error::{DataFusionError, Result};
+use chrono::Utc;
+
+/// Object Reader for one file in a object store
+#[async_trait]
+pub trait ObjectReader {
+ /// Get reader for a part [start, start + length] in the file asynchronously
+ async fn get_reader(&self, start: u64, length: usize) -> Result<Arc<dyn AsyncRead>>;
+
+ /// Get length for the file asynchronously
+ async fn length(&self) -> Result<u64>;
+}
+
+/// File meta we got from object store
+pub struct FileMeta {
+ /// Path of the file
+ pub path: String,
+ /// Last time the file was modified in UTC
+ pub last_modified: Option<chrono::DateTime<Utc>>,
+ /// File size in total
+ pub size: Option<u64>,
Review comment:
I checked several definitions in rusoto_s3, and it seems Option is used widely for length, size. for example, `rusoto_s3:Object`, `rusoto_s3:GetObjectOutput`, so it's here. I am neutral on this and please correct me in a more idiomatic way.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] rdettai edited a comment on pull request #950: ObjectStore API to read from remote storage systems
Posted by GitBox <gi...@apache.org>.
rdettai edited a comment on pull request #950:
URL: https://github.com/apache/arrow-datafusion/pull/950#issuecomment-907769383
> I am under the assumption that TableProvider is responsible for parsing the partition parameters from the URI bucket/prefix?partition=year&partition=month and constructing the list object API calls with appropriate prefixes
In the case of hive partitioning, you need an extra capability of the object store to achieve that: **list the folders** (ex for s3 [here](https://docs.aws.amazon.com/AmazonS3/latest/userguide/using-prefixes.html)). Otherwise you cannot now what partitions are there and build the appropriate prefixes. Maybe this example helps getting more context: https://docs.google.com/document/d/1nKXPvolft1_DuVjVK7ceOx37arCdQLKc_Im-TfZh9dk/edit?usp=sharing
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] yjshen edited a comment on pull request #950: ObjectStore API to read from remote storage systems
Posted by GitBox <gi...@apache.org>.
yjshen edited a comment on pull request #950:
URL: https://github.com/apache/arrow-datafusion/pull/950#issuecomment-906652660
@rdettai Thanks for reviewing 👍
> an URI (string just like the prefix currently), which could be a sort of path (bucket+prefix) for a plain object store like S3, but could also be something a bit more evolved:
> - an S3 location with hive partitioning (URI=bucket/prefix?partition=year&partition=month)
> - a delta table (URI=bucket/prefix?versionAsOf=v2)
I think this could be achieved inside the S3 object store implementation with another PR on the `PartitionedFile` abstraction #932 . `list` could return a stream of `PartitionedFile` instead of the current `FileMeta`. (`PartitionedFile` could have a field of `FileMeta`).
> an expression so that we can pushdown the filter to the generation of the file list. This is VERY important for very large datasets with lots of files where listing all files is too long.
I think the current, non-filtering version of the listing is made here for simplicity. check more discussions on this in doc [here](https://docs.google.com/document/d/1ZEZqvdohrot0ewtTNeaBtqczOIJ1Q0OnX9PqMMxpOF8/edit?disco=AAAANwU9MzE#heading=h.358nvuimx7yr)
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] alamb commented on pull request #950: ObjectStore API to read from remote storage systems
Posted by GitBox <gi...@apache.org>.
alamb commented on pull request #950:
URL: https://github.com/apache/arrow-datafusion/pull/950#issuecomment-917062432
> @alamb What's your opinion on the current list_file and list_dir API?
I think it looks good 👍
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] yjshen edited a comment on pull request #950: ObjectStore API to read from remote storage systems
Posted by GitBox <gi...@apache.org>.
yjshen edited a comment on pull request #950:
URL: https://github.com/apache/arrow-datafusion/pull/950#issuecomment-906652660
@rdettai Thanks for reviewing 👍
> an URI (string just like the prefix currently), which could be a sort of path (bucket+prefix) for a plain object store like S3, but could also be something a bit more evolved:
> - an S3 location with hive partitioning (URI=bucket/prefix?partition=year&partition=month)
> - a delta table (URI=bucket/prefix?versionAsOf=v2)
I think this could be achieved inside the S3 object store implementation with another PR on the `PartitionedFile` abstraction #946. `list` could return a stream of `PartitionedFile` instead of the current `FileMeta`. (`PartitionedFile` could have a field of `FileMeta`).
> an expression so that we can pushdown the filter to the generation of the file list. This is VERY important for very large datasets with lots of files where listing all files is too long.
I think the current, non-filtering version of the listing is made here for simplicity. check more discussions on this in doc [here](https://docs.google.com/document/d/1ZEZqvdohrot0ewtTNeaBtqczOIJ1Q0OnX9PqMMxpOF8/edit?disco=AAAANwU9MzE#heading=h.358nvuimx7yr)
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] alamb commented on pull request #950: ObjectStore API to read from remote storage systems
Posted by GitBox <gi...@apache.org>.
alamb commented on pull request #950:
URL: https://github.com/apache/arrow-datafusion/pull/950#issuecomment-916897077
Sorry for the delay @yjshen -- I will review this carefully today.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] rdettai commented on a change in pull request #950: ObjectStore API to read from remote storage systems
Posted by GitBox <gi...@apache.org>.
rdettai commented on a change in pull request #950:
URL: https://github.com/apache/arrow-datafusion/pull/950#discussion_r696770693
##########
File path: datafusion/src/datasource/object_store/mod.rs
##########
@@ -0,0 +1,127 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+//! Object Store abstracts access to an underlying file/object storage.
+
+pub mod local;
+
+use std::collections::HashMap;
+use std::fmt::Debug;
+use std::pin::Pin;
+use std::sync::{Arc, RwLock};
+
+use async_trait::async_trait;
+use futures::{AsyncRead, Stream};
+
+use local::LocalFileSystem;
+
+use crate::error::{DataFusionError, Result};
+use chrono::Utc;
+
+/// Object Reader for one file in a object store
+#[async_trait]
+pub trait ObjectReader {
+ /// Get reader for a part [start, start + length] in the file asynchronously
+ async fn get_reader(&self, start: u64, length: usize) -> Result<Arc<dyn AsyncRead>>;
+
+ /// Get length for the file asynchronously
+ async fn length(&self) -> Result<u64>;
+}
+
+/// File meta we got from object store
+pub struct FileMeta {
+ /// Path of the file
+ pub path: String,
+ /// Last time the file was modified in UTC
+ pub last_modified: Option<chrono::DateTime<Utc>>,
+ /// File size in total
+ pub size: Option<u64>,
Review comment:
Why is the size left optional? I think that it will always be very easy to get it for the ObjectStore implementation and Option<> here adds some annoying branching from the user side (I wanted to submit this on the design document already but I think my post didn't went through 😃).
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] rdettai edited a comment on pull request #950: ObjectStore API to read from remote storage systems
Posted by GitBox <gi...@apache.org>.
rdettai edited a comment on pull request #950:
URL: https://github.com/apache/arrow-datafusion/pull/950#issuecomment-907014261
> I think this could be achieved inside the S3 object store implementation with another PR
Agreed, my point is mostly about the naming/doc, I find `prefix` pretty restrictive
> I think the current, non-filtering version of the listing is made here for simplicity
Understood! As the goal of this PR is to set the interface, I thought we might as well include everything that will be required, and I am pretty sure that filter pushdown to the file listing is a must have. Note that it can be included into the interface and not used in the implementations at first, just like the filter pushdown in `TableProvider` .
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] houqp commented on pull request #950: ObjectStore API to read from remote storage systems
Posted by GitBox <gi...@apache.org>.
houqp commented on pull request #950:
URL: https://github.com/apache/arrow-datafusion/pull/950#issuecomment-909331804
looks like python test is broken on master
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] houqp commented on a change in pull request #950: ObjectStore API to read from remote storage systems
Posted by GitBox <gi...@apache.org>.
houqp commented on a change in pull request #950:
URL: https://github.com/apache/arrow-datafusion/pull/950#discussion_r698139232
##########
File path: datafusion/src/datasource/object_store/mod.rs
##########
@@ -0,0 +1,130 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+//! Object Store abstracts access to an underlying file/object storage.
+
+pub mod local;
+
+use std::collections::HashMap;
+use std::fmt::Debug;
+use std::pin::Pin;
+use std::sync::{Arc, RwLock};
+
+use async_trait::async_trait;
+use futures::{AsyncRead, Stream};
+
+use local::LocalFileSystem;
+
+use crate::error::{DataFusionError, Result};
+use chrono::Utc;
+
+/// Object Reader for one file in a object store
+#[async_trait]
+pub trait ObjectReader {
+ /// Get reader for a part [start, start + length] in the file asynchronously
+ async fn chunk_reader(&self, start: u64, length: usize)
+ -> Result<Arc<dyn AsyncRead>>;
+
+ /// Get length for the file
+ fn length(&self) -> u64;
+}
+
+/// File meta we got from object store
+#[derive(Debug)]
+pub struct FileMeta {
+ /// Path of the file
+ pub path: String,
+ /// Last time the file was modified in UTC
+ pub last_modified: Option<chrono::DateTime<Utc>>,
+ /// File size in total
+ pub size: u64,
+}
+
+/// Stream of files get listed from object store
+pub type FileMetaStream =
+ Pin<Box<dyn Stream<Item = Result<FileMeta>> + Send + Sync + 'static>>;
+
+/// A ObjectStore abstracts access to an underlying file/object storage.
+/// It maps strings (e.g. URLs, filesystem paths, etc) to sources of bytes
+#[async_trait]
+pub trait ObjectStore: Sync + Send + Debug {
+ /// Returns all the files in path `prefix` asynchronously.
+ async fn list(&self, prefix: &str) -> Result<FileMetaStream>;
+
+ /// Get object reader for one file
+ fn file_reader(&self, file: FileMeta) -> Result<Arc<dyn ObjectReader>>;
Review comment:
Sorry for going back and forth on this one. But I think both you and @rdettai were correct that it would be better to keep `file_reader` sync so we can force io into `chunk_reader` lazily. So I think it's best if we could revert the async change for `file_reader`.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] alamb merged pull request #950: ObjectStore API to read from remote storage systems
Posted by GitBox <gi...@apache.org>.
alamb merged pull request #950:
URL: https://github.com/apache/arrow-datafusion/pull/950
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] rdettai edited a comment on pull request #950: ObjectStore API to read from remote storage systems
Posted by GitBox <gi...@apache.org>.
rdettai edited a comment on pull request #950:
URL: https://github.com/apache/arrow-datafusion/pull/950#issuecomment-907820496
@yjshen @houqp you are right, I think that maybe with the delimiter argument added to `ObjectStore.list()`, the TableProvider would be able to get all the information it needs to efficiently prune the partitions in a raw hive partitioning setup! This way we leave the ObjectStore abstraction with only core object store capabilities 😄
I am still wondering how we could replicate the hive partitioning support for `TableProvider`s relative to different file formats (`ParquetTable`, `CsvFile`, `NdJsonFile`). Actually, same would apply for a HiveCatalogTable that could actually reference various file formats. It makes me feel that something is wrong with how the different abstractions are organized. This is being discussed in #133.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] rdettai commented on a change in pull request #950: ObjectStore API to read from remote storage systems
Posted by GitBox <gi...@apache.org>.
rdettai commented on a change in pull request #950:
URL: https://github.com/apache/arrow-datafusion/pull/950#discussion_r697994301
##########
File path: datafusion/src/datasource/object_store/mod.rs
##########
@@ -0,0 +1,130 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+//! Object Store abstracts access to an underlying file/object storage.
+
+pub mod local;
+
+use std::collections::HashMap;
+use std::fmt::Debug;
+use std::pin::Pin;
+use std::sync::{Arc, RwLock};
+
+use async_trait::async_trait;
+use futures::{AsyncRead, Stream};
+
+use local::LocalFileSystem;
+
+use crate::error::{DataFusionError, Result};
+use chrono::Utc;
+
+/// Object Reader for one file in a object store
+#[async_trait]
+pub trait ObjectReader {
+ /// Get reader for a part [start, start + length] in the file asynchronously
+ async fn chunk_reader(&self, start: u64, length: usize)
+ -> Result<Arc<dyn AsyncRead>>;
+
+ /// Get length for the file
+ fn length(&self) -> u64;
+}
+
+/// File meta we got from object store
+#[derive(Debug)]
+pub struct FileMeta {
+ /// Path of the file
+ pub path: String,
+ /// Last time the file was modified in UTC
+ pub last_modified: Option<chrono::DateTime<Utc>>,
+ /// File size in total
+ pub size: u64,
+}
+
+/// Stream of files get listed from object store
+pub type FileMetaStream =
+ Pin<Box<dyn Stream<Item = Result<FileMeta>> + Send + Sync + 'static>>;
+
+/// A ObjectStore abstracts access to an underlying file/object storage.
+/// It maps strings (e.g. URLs, filesystem paths, etc) to sources of bytes
+#[async_trait]
+pub trait ObjectStore: Sync + Send + Debug {
+ /// Returns all the files in path `prefix` asynchronously.
+ async fn list(&self, prefix: &str) -> Result<FileMetaStream>;
+
+ /// Get object reader for one file
+ fn file_reader(&self, file: FileMeta) -> Result<Arc<dyn ObjectReader>>;
Review comment:
👍 for the method renames also 😉
> Maybe we should keep ObjectStore::get_reader as async? Otherwise, it looks like every call to ObjectReader::get_reader would need to open a new file descriptor.
- `ObjectStore` can cache the file descriptor if it wants to, this just forces it to get it lazily
- Re-using the file descriptor means that the various AsyncRead objects generated might interfer with each other if used in parallel, no? (calling `read` on one might move the cursor thus moving the cursor of the other one`). Is this ok?
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] alamb commented on pull request #950: ObjectStore API to read from remote storage systems
Posted by GitBox <gi...@apache.org>.
alamb commented on pull request #950:
URL: https://github.com/apache/arrow-datafusion/pull/950#issuecomment-917065741
We have resolved the python CI failure on master so I am going to merge this PR in as is. Thanks again @yjshen
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] houqp commented on a change in pull request #950: ObjectStore API to read from remote storage systems
Posted by GitBox <gi...@apache.org>.
houqp commented on a change in pull request #950:
URL: https://github.com/apache/arrow-datafusion/pull/950#discussion_r697552520
##########
File path: datafusion/src/datasource/object_store/local.rs
##########
@@ -0,0 +1,127 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+//! Object store that represents the Local File System.
+
+use std::fs::Metadata;
+use std::sync::Arc;
+
+use async_trait::async_trait;
+use futures::{stream, AsyncRead, StreamExt};
+
+use crate::datasource::object_store::{
+ FileMeta, FileMetaStream, ObjectReader, ObjectStore,
+};
+use crate::error::DataFusionError;
+use crate::error::Result;
+use crate::logical_plan::Expr;
+
+#[derive(Debug)]
+/// Local File System as Object Store.
+pub struct LocalFileSystem;
+
+#[async_trait]
+impl ObjectStore for LocalFileSystem {
+ async fn list(&self, prefix: &str, _filters: &[Expr]) -> Result<FileMetaStream> {
+ list_all(prefix.to_owned()).await
+ }
+
+ fn get_reader(&self, file: FileMeta) -> Result<Arc<dyn ObjectReader>> {
+ Ok(Arc::new(LocalFileReader::new(file)?))
+ }
+}
+
+struct LocalFileReader {
+ file: FileMeta,
+}
+
+impl LocalFileReader {
+ fn new(file: FileMeta) -> Result<Self> {
+ Ok(Self { file })
+ }
+}
+
+#[async_trait]
+impl ObjectReader for LocalFileReader {
+ async fn get_reader(
Review comment:
Maybe we should keep `ObjectStore::get_reader` as async? Otherwise, it looks like every call to `ObjectReader::get_reader` would need to open a new file descriptor.
We may want to change `ObjectReader::get_reader` to take `self` as `&mut` so an object reader can implement caching and look ahead read behind the scene.
##########
File path: datafusion/src/datasource/object_store/mod.rs
##########
@@ -0,0 +1,130 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+//! Object Store abstracts access to an underlying file/object storage.
+
+pub mod local;
+
+use std::collections::HashMap;
+use std::fmt::Debug;
+use std::pin::Pin;
+use std::sync::{Arc, RwLock};
+
+use async_trait::async_trait;
+use futures::{AsyncRead, Stream};
+
+use local::LocalFileSystem;
+
+use crate::error::{DataFusionError, Result};
+use crate::logical_plan::Expr;
+use chrono::Utc;
+
+/// Object Reader for one file in a object store
+#[async_trait]
+pub trait ObjectReader {
+ /// Get reader for a part [start, start + length] in the file asynchronously
+ async fn get_reader(&self, start: u64, length: usize) -> Result<Arc<dyn AsyncRead>>;
Review comment:
Mostly a nitpick, for the naming, it might make the code more readable if we use a name that's different from the `get_reader` method we have in `ObjectStore` here. For example, something along the lines of `get_read`, `get_chunk`, `chunk`:
```rust
store.get_reader().get_read().read()
```
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] houqp commented on pull request #950: ObjectStore API to read from remote storage systems
Posted by GitBox <gi...@apache.org>.
houqp commented on pull request #950:
URL: https://github.com/apache/arrow-datafusion/pull/950#issuecomment-909331804
looks like python test is broken on master
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] rdettai commented on pull request #950: ObjectStore API to read from remote storage systems
Posted by GitBox <gi...@apache.org>.
rdettai commented on pull request #950:
URL: https://github.com/apache/arrow-datafusion/pull/950#issuecomment-907014261
> I think this could be achieved inside the S3 object store implementation with another PR
Agreed, my point is mostly about the naming/doc, I find `prefix` pretty restrictive
> I think the current, non-filtering version of the listing is made here for simplicity
Understood! As the goal of this PR is to set the interface, I thought we might as well include everything that will be required, and I am pretty sure that filter pushdown to the file listing is a must have. Note that it can be included into the interface and not used in the implementations at first, just like the `TableProvider` pushdown.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] rdettai commented on a change in pull request #950: ObjectStore API to read from remote storage systems
Posted by GitBox <gi...@apache.org>.
rdettai commented on a change in pull request #950:
URL: https://github.com/apache/arrow-datafusion/pull/950#discussion_r697226770
##########
File path: datafusion/src/datasource/object_store/mod.rs
##########
@@ -0,0 +1,127 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+//! Object Store abstracts access to an underlying file/object storage.
+
+pub mod local;
+
+use std::collections::HashMap;
+use std::fmt::Debug;
+use std::pin::Pin;
+use std::sync::{Arc, RwLock};
+
+use async_trait::async_trait;
+use futures::{AsyncRead, Stream};
+
+use local::LocalFileSystem;
+
+use crate::error::{DataFusionError, Result};
+use chrono::Utc;
+
+/// Object Reader for one file in a object store
+#[async_trait]
+pub trait ObjectReader {
+ /// Get reader for a part [start, start + length] in the file asynchronously
+ async fn get_reader(&self, start: u64, length: usize) -> Result<Arc<dyn AsyncRead>>;
+
+ /// Get length for the file asynchronously
+ async fn length(&self) -> Result<u64>;
+}
+
+/// File meta we got from object store
+pub struct FileMeta {
+ /// Path of the file
+ pub path: String,
+ /// Last time the file was modified in UTC
+ pub last_modified: Option<chrono::DateTime<Utc>>,
+ /// File size in total
+ pub size: Option<u64>,
Review comment:
You are right that technically the S3 API says the `size` field is optional in the list response. I couldn't find why. It might make sense when the `Object` is a prefix and not an actual object (for exemple if you list the memsql-tpch-dataset bucket, you will get entries for `sf_100/`, `sf_100/customer/`...), but actually with Rusoto, even in that case the `size` field is set to `Some(0)` 🙃.
My argument is that _we can expect from any system that provides the list of filenames to also reliably provide the file sizes_. With this assumption, avoiding an optional field helps simplifying consumers that do use that information. For instance for parquet you will need to read from the end. As the `ObjectReader` works using the (start, length) tuple, we critically need the length to get the end of the file. This means that if the `size` provided from the listing is optional, every implementation of `ObjectReader.length` will need to have a branch with the case where the size was provided, in which case it uses it directly, and another branch where it fetches the size from the store. I find this adds some complexity.
For me the true question is whether the assumption _we can expect from any system that provides the list of filenames to also reliably provide the file sizes_ holds. I am pretty sure it does, but if you don't think so we can definitively pay the price of `Option<>` 😉
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] houqp commented on pull request #950: ObjectStore API to read from remote storage systems
Posted by GitBox <gi...@apache.org>.
houqp commented on pull request #950:
URL: https://github.com/apache/arrow-datafusion/pull/950#issuecomment-907815987
Thanks @rdettai for the detailed google doc write up. I left some comments there :) I think the object store interface proposed in this PR does support listing object keys with partition prefixes, for example: `ObjectStore::lsit("bucket/path/date=2018")`. But it's missing an important argument `delimiter`, which is required to make "folder" hierarchy discovery more efficient in object stores.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] houqp commented on a change in pull request #950: ObjectStore API to read from remote storage systems
Posted by GitBox <gi...@apache.org>.
houqp commented on a change in pull request #950:
URL: https://github.com/apache/arrow-datafusion/pull/950#discussion_r697759783
##########
File path: datafusion/src/datasource/object_store/local.rs
##########
@@ -0,0 +1,127 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+//! Object store that represents the Local File System.
+
+use std::fs::Metadata;
+use std::sync::Arc;
+
+use async_trait::async_trait;
+use futures::{stream, AsyncRead, StreamExt};
+
+use crate::datasource::object_store::{
+ FileMeta, FileMetaStream, ObjectReader, ObjectStore,
+};
+use crate::error::DataFusionError;
+use crate::error::Result;
+use crate::logical_plan::Expr;
+
+#[derive(Debug)]
+/// Local File System as Object Store.
+pub struct LocalFileSystem;
+
+#[async_trait]
+impl ObjectStore for LocalFileSystem {
+ async fn list(&self, prefix: &str, _filters: &[Expr]) -> Result<FileMetaStream> {
+ list_all(prefix.to_owned()).await
+ }
+
+ fn get_reader(&self, file: FileMeta) -> Result<Arc<dyn ObjectReader>> {
+ Ok(Arc::new(LocalFileReader::new(file)?))
+ }
+}
+
+struct LocalFileReader {
+ file: FileMeta,
+}
+
+impl LocalFileReader {
+ fn new(file: FileMeta) -> Result<Self> {
+ Ok(Self { file })
+ }
+}
+
+#[async_trait]
+impl ObjectReader for LocalFileReader {
+ async fn get_reader(
Review comment:
`ObjectStore::get_reader` is defined as `fn get_reader(&self, file: FileMeta) -> Result<Arc<dyn ObjectReader>>;`, which is a sync method.
`ObjectReader::get_reader` is defined as async. That's why in my comment https://github.com/apache/arrow-datafusion/pull/950/files/f4ed233397210dd74b84a10721460edd4979987c#r697560511 I recommended a method name change in the `ObjectReader` trait to make the code easier to follow :)
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] yjshen commented on a change in pull request #950: ObjectStore API to read from remote storage systems
Posted by GitBox <gi...@apache.org>.
yjshen commented on a change in pull request #950:
URL: https://github.com/apache/arrow-datafusion/pull/950#discussion_r698612203
##########
File path: datafusion/src/datasource/object_store/local.rs
##########
@@ -0,0 +1,173 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+//! Object store that represents the Local File System.
+
+use std::fs::Metadata;
+use std::sync::Arc;
+
+use async_trait::async_trait;
+use futures::{stream, AsyncRead, StreamExt};
+
+use crate::datasource::object_store::{
+ FileMeta, FileMetaStream, ObjectReader, ObjectStore,
+};
+use crate::error::DataFusionError;
+use crate::error::Result;
+
+#[derive(Debug)]
+/// Local File System as Object Store.
+pub struct LocalFileSystem;
+
+#[async_trait]
+impl ObjectStore for LocalFileSystem {
+ async fn list(
+ &self,
+ prefix: &str,
+ _delimiter: Option<String>,
+ ) -> Result<FileMetaStream> {
+ list_all(prefix.to_owned()).await
Review comment:
`PathMeta`? `FilePathMeta`? 😂 I'm run out of naming
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] yjshen edited a comment on pull request #950: ObjectStore API to read from remote storage systems
Posted by GitBox <gi...@apache.org>.
yjshen edited a comment on pull request #950:
URL: https://github.com/apache/arrow-datafusion/pull/950#issuecomment-906652660
@rdettai Thanks for reviewing 👍
> an URI (string just like the prefix currently), which could be a sort of path (bucket+prefix) for a plain object store like S3, but could also be something a bit more evolved:
> - an S3 location with hive partitioning (URI=bucket/prefix?partition=year&partition=month)
> - a delta table (URI=bucket/prefix?versionAsOf=v2)
I think this could be achieved inside the S3 object store implementation with another PR on the `PartitionedFile` abstraction #946 #932 . `list` could return a stream of `PartitionedFile` instead of the current `FileMeta`. (`PartitionedFile` could have a field of `FileMeta`).
> an expression so that we can pushdown the filter to the generation of the file list. This is VERY important for very large datasets with lots of files where listing all files is too long.
I think the current, non-filtering version of the listing is made here for simplicity. check more discussions on this in doc [here](https://docs.google.com/document/d/1ZEZqvdohrot0ewtTNeaBtqczOIJ1Q0OnX9PqMMxpOF8/edit?disco=AAAANwU9MzE#heading=h.358nvuimx7yr)
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] rdettai commented on a change in pull request #950: ObjectStore API to read from remote storage systems
Posted by GitBox <gi...@apache.org>.
rdettai commented on a change in pull request #950:
URL: https://github.com/apache/arrow-datafusion/pull/950#discussion_r698582872
##########
File path: datafusion/src/datasource/object_store/local.rs
##########
@@ -0,0 +1,173 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+//! Object store that represents the Local File System.
+
+use std::fs::Metadata;
+use std::sync::Arc;
+
+use async_trait::async_trait;
+use futures::{stream, AsyncRead, StreamExt};
+
+use crate::datasource::object_store::{
+ FileMeta, FileMetaStream, ObjectReader, ObjectStore,
+};
+use crate::error::DataFusionError;
+use crate::error::Result;
+
+#[derive(Debug)]
+/// Local File System as Object Store.
+pub struct LocalFileSystem;
+
+#[async_trait]
+impl ObjectStore for LocalFileSystem {
+ async fn list(
+ &self,
+ prefix: &str,
+ _delimiter: Option<String>,
+ ) -> Result<FileMetaStream> {
+ list_all(prefix.to_owned()).await
Review comment:
it seems to me that if you don't comply to the API, a TODO is appropriate even if nobody is using the feature yet 😃
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] yjshen commented on pull request #950: ObjectStore API to read from remote storage systems
Posted by GitBox <gi...@apache.org>.
yjshen commented on pull request #950:
URL: https://github.com/apache/arrow-datafusion/pull/950#issuecomment-906652660
> an URI (string just like the prefix currently), which could be a sort of path (bucket+prefix) for a plain object store like S3, but could also be something a bit more evolved:
> - an S3 location with hive partitioning (URI=bucket/prefix?partition=year&partition=month)
> - a delta table (URI=bucket/prefix?versionAsOf=v2)
I think this could be achieved inside the S3 object store implementation with another PR on the `PartitionedFile` abstraction #946. `list` could return a stream of `PartitionedFile` instead of the current `FileMeta`. (`PartitionedFile` could have a field of `FileMeta`).
> an expression so that we can pushdown the filter to the generation of the file list. This is VERY important for very large datasets with lots of files where listing all files is too long.
I think the current, non-filtering version of the listing is made here for simplicity. check more discussions on this in doc [here](https://docs.google.com/document/d/1ZEZqvdohrot0ewtTNeaBtqczOIJ1Q0OnX9PqMMxpOF8/edit?disco=AAAANwU9MzE#heading=h.358nvuimx7yr)
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] houqp commented on a change in pull request #950: ObjectStore API to read from remote storage systems
Posted by GitBox <gi...@apache.org>.
houqp commented on a change in pull request #950:
URL: https://github.com/apache/arrow-datafusion/pull/950#discussion_r697962711
##########
File path: datafusion/src/datasource/object_store/mod.rs
##########
@@ -0,0 +1,130 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+//! Object Store abstracts access to an underlying file/object storage.
+
+pub mod local;
+
+use std::collections::HashMap;
+use std::fmt::Debug;
+use std::pin::Pin;
+use std::sync::{Arc, RwLock};
+
+use async_trait::async_trait;
+use futures::{AsyncRead, Stream};
+
+use local::LocalFileSystem;
+
+use crate::error::{DataFusionError, Result};
+use chrono::Utc;
+
+/// Object Reader for one file in a object store
+#[async_trait]
+pub trait ObjectReader {
+ /// Get reader for a part [start, start + length] in the file asynchronously
+ async fn chunk_reader(&self, start: u64, length: usize)
+ -> Result<Arc<dyn AsyncRead>>;
+
+ /// Get length for the file
+ fn length(&self) -> u64;
+}
+
+/// File meta we got from object store
+#[derive(Debug)]
+pub struct FileMeta {
+ /// Path of the file
+ pub path: String,
+ /// Last time the file was modified in UTC
+ pub last_modified: Option<chrono::DateTime<Utc>>,
+ /// File size in total
+ pub size: u64,
+}
+
+/// Stream of files get listed from object store
+pub type FileMetaStream =
+ Pin<Box<dyn Stream<Item = Result<FileMeta>> + Send + Sync + 'static>>;
+
+/// A ObjectStore abstracts access to an underlying file/object storage.
+/// It maps strings (e.g. URLs, filesystem paths, etc) to sources of bytes
+#[async_trait]
+pub trait ObjectStore: Sync + Send + Debug {
+ /// Returns all the files in path `prefix` asynchronously.
+ async fn list(&self, prefix: &str) -> Result<FileMetaStream>;
+
+ /// Get object reader for one file
+ fn file_reader(&self, file: FileMeta) -> Result<Arc<dyn ObjectReader>>;
Review comment:
The new `file_reader` and `chunk_reader` method renames makes the code much easier to follow :+1:
I noticed `file_reader` is still defined as sync method here, curious what's your thoughts on my comment at https://github.com/apache/arrow-datafusion/pull/950#discussion_r697552520?
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] houqp commented on a change in pull request #950: ObjectStore API to read from remote storage systems
Posted by GitBox <gi...@apache.org>.
houqp commented on a change in pull request #950:
URL: https://github.com/apache/arrow-datafusion/pull/950#discussion_r697759783
##########
File path: datafusion/src/datasource/object_store/local.rs
##########
@@ -0,0 +1,127 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+//! Object store that represents the Local File System.
+
+use std::fs::Metadata;
+use std::sync::Arc;
+
+use async_trait::async_trait;
+use futures::{stream, AsyncRead, StreamExt};
+
+use crate::datasource::object_store::{
+ FileMeta, FileMetaStream, ObjectReader, ObjectStore,
+};
+use crate::error::DataFusionError;
+use crate::error::Result;
+use crate::logical_plan::Expr;
+
+#[derive(Debug)]
+/// Local File System as Object Store.
+pub struct LocalFileSystem;
+
+#[async_trait]
+impl ObjectStore for LocalFileSystem {
+ async fn list(&self, prefix: &str, _filters: &[Expr]) -> Result<FileMetaStream> {
+ list_all(prefix.to_owned()).await
+ }
+
+ fn get_reader(&self, file: FileMeta) -> Result<Arc<dyn ObjectReader>> {
+ Ok(Arc::new(LocalFileReader::new(file)?))
+ }
+}
+
+struct LocalFileReader {
+ file: FileMeta,
+}
+
+impl LocalFileReader {
+ fn new(file: FileMeta) -> Result<Self> {
+ Ok(Self { file })
+ }
+}
+
+#[async_trait]
+impl ObjectReader for LocalFileReader {
+ async fn get_reader(
Review comment:
`ObjectStore::get_reader` is defined as `fn get_reader(&self, file: FileMeta) -> Result<Arc<dyn ObjectReader>>;`, which is a sync method.
`ObjectReader::get_reader` is defined as async. That's why in my comment https://github.com/apache/arrow-datafusion/pull/950/files/f4ed233397210dd74b84a10721460edd4979987c#r697560511 I recommended a method name change here to make the code easier to follow :)
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] houqp commented on a change in pull request #950: ObjectStore API to read from remote storage systems
Posted by GitBox <gi...@apache.org>.
houqp commented on a change in pull request #950:
URL: https://github.com/apache/arrow-datafusion/pull/950#discussion_r699474709
##########
File path: datafusion/src/datasource/object_store/local.rs
##########
@@ -0,0 +1,173 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+//! Object store that represents the Local File System.
+
+use std::fs::Metadata;
+use std::sync::Arc;
+
+use async_trait::async_trait;
+use futures::{stream, AsyncRead, StreamExt};
+
+use crate::datasource::object_store::{
+ FileMeta, FileMetaStream, ObjectReader, ObjectStore,
+};
+use crate::error::DataFusionError;
+use crate::error::Result;
+
+#[derive(Debug)]
+/// Local File System as Object Store.
+pub struct LocalFileSystem;
+
+#[async_trait]
+impl ObjectStore for LocalFileSystem {
+ async fn list(
+ &self,
+ prefix: &str,
+ _delimiter: Option<String>,
+ ) -> Result<FileMetaStream> {
+ list_all(prefix.to_owned()).await
Review comment:
@alamb that's interesting, do you have any opinion on the design here?
Since list without delimiter always returns object keys, not logical directory keys, I feel like returning `FileMeta` in list call provides a better more statically typed interface for callers.
In IOx, do you remember what was the reason for going back to `ObjectStorePath` as return type for regular list calls?
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] yjshen commented on a change in pull request #950: ObjectStore API to read from remote storage systems
Posted by GitBox <gi...@apache.org>.
yjshen commented on a change in pull request #950:
URL: https://github.com/apache/arrow-datafusion/pull/950#discussion_r698522038
##########
File path: datafusion/src/datasource/object_store/local.rs
##########
@@ -0,0 +1,173 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+//! Object store that represents the Local File System.
+
+use std::fs::Metadata;
+use std::sync::Arc;
+
+use async_trait::async_trait;
+use futures::{stream, AsyncRead, StreamExt};
+
+use crate::datasource::object_store::{
+ FileMeta, FileMetaStream, ObjectReader, ObjectStore,
+};
+use crate::error::DataFusionError;
+use crate::error::Result;
+
+#[derive(Debug)]
+/// Local File System as Object Store.
+pub struct LocalFileSystem;
+
+#[async_trait]
+impl ObjectStore for LocalFileSystem {
+ async fn list(
+ &self,
+ prefix: &str,
+ _delimiter: Option<String>,
+ ) -> Result<FileMetaStream> {
+ list_all(prefix.to_owned()).await
Review comment:
I think it's fine to leave out the TODO since the partitioned file read in not supported?
##########
File path: datafusion/src/datasource/object_store/mod.rs
##########
@@ -0,0 +1,130 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+//! Object Store abstracts access to an underlying file/object storage.
+
+pub mod local;
+
+use std::collections::HashMap;
+use std::fmt::Debug;
+use std::pin::Pin;
+use std::sync::{Arc, RwLock};
+
+use async_trait::async_trait;
+use futures::{AsyncRead, Stream};
+
+use local::LocalFileSystem;
+
+use crate::error::{DataFusionError, Result};
+use chrono::Utc;
+
+/// Object Reader for one file in a object store
+#[async_trait]
+pub trait ObjectReader {
+ /// Get reader for a part [start, start + length] in the file asynchronously
+ async fn chunk_reader(&self, start: u64, length: usize)
+ -> Result<Arc<dyn AsyncRead>>;
+
+ /// Get length for the file
+ fn length(&self) -> u64;
+}
+
+/// File meta we got from object store
+#[derive(Debug)]
+pub struct FileMeta {
+ /// Path of the file
+ pub path: String,
+ /// Last time the file was modified in UTC
+ pub last_modified: Option<chrono::DateTime<Utc>>,
+ /// File size in total
+ pub size: u64,
+}
+
+/// Stream of files get listed from object store
+pub type FileMetaStream =
+ Pin<Box<dyn Stream<Item = Result<FileMeta>> + Send + Sync + 'static>>;
+
+/// A ObjectStore abstracts access to an underlying file/object storage.
+/// It maps strings (e.g. URLs, filesystem paths, etc) to sources of bytes
+#[async_trait]
+pub trait ObjectStore: Sync + Send + Debug {
+ /// Returns all the files in path `prefix` asynchronously.
+ async fn list(&self, prefix: &str) -> Result<FileMetaStream>;
+
+ /// Get object reader for one file
+ fn file_reader(&self, file: FileMeta) -> Result<Arc<dyn ObjectReader>>;
Review comment:
Reverted to sync method now.
##########
File path: datafusion/src/datasource/object_store/local.rs
##########
@@ -0,0 +1,173 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+//! Object store that represents the Local File System.
+
+use std::fs::Metadata;
+use std::sync::Arc;
+
+use async_trait::async_trait;
+use futures::{stream, AsyncRead, StreamExt};
+
+use crate::datasource::object_store::{
+ FileMeta, FileMetaStream, ObjectReader, ObjectStore,
+};
+use crate::error::DataFusionError;
+use crate::error::Result;
+
+#[derive(Debug)]
+/// Local File System as Object Store.
+pub struct LocalFileSystem;
+
+#[async_trait]
+impl ObjectStore for LocalFileSystem {
+ async fn list(
+ &self,
+ prefix: &str,
+ _delimiter: Option<String>,
+ ) -> Result<FileMetaStream> {
+ list_all(prefix.to_owned()).await
Review comment:
`PathMeta`? `FilePathMeta`? 😂 I'm run out of naming
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] rdettai commented on a change in pull request #950: ObjectStore API to read from remote storage systems
Posted by GitBox <gi...@apache.org>.
rdettai commented on a change in pull request #950:
URL: https://github.com/apache/arrow-datafusion/pull/950#discussion_r698584977
##########
File path: datafusion/src/datasource/object_store/local.rs
##########
@@ -0,0 +1,173 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+//! Object store that represents the Local File System.
+
+use std::fs::Metadata;
+use std::sync::Arc;
+
+use async_trait::async_trait;
+use futures::{stream, AsyncRead, StreamExt};
+
+use crate::datasource::object_store::{
+ FileMeta, FileMetaStream, ObjectReader, ObjectStore,
+};
+use crate::error::DataFusionError;
+use crate::error::Result;
+
+#[derive(Debug)]
+/// Local File System as Object Store.
+pub struct LocalFileSystem;
+
+#[async_trait]
+impl ObjectStore for LocalFileSystem {
+ async fn list(
+ &self,
+ prefix: &str,
+ _delimiter: Option<String>,
+ ) -> Result<FileMetaStream> {
+ list_all(prefix.to_owned()).await
Review comment:
also, if the API can return folders, the name of the structs returned (`FileMeta`) is kind of misleading.
##########
File path: datafusion/src/datasource/object_store/local.rs
##########
@@ -0,0 +1,173 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+//! Object store that represents the Local File System.
+
+use std::fs::Metadata;
+use std::sync::Arc;
+
+use async_trait::async_trait;
+use futures::{stream, AsyncRead, StreamExt};
+
+use crate::datasource::object_store::{
+ FileMeta, FileMetaStream, ObjectReader, ObjectStore,
+};
+use crate::error::DataFusionError;
+use crate::error::Result;
+
+#[derive(Debug)]
+/// Local File System as Object Store.
+pub struct LocalFileSystem;
+
+#[async_trait]
+impl ObjectStore for LocalFileSystem {
+ async fn list(
+ &self,
+ prefix: &str,
+ _delimiter: Option<String>,
+ ) -> Result<FileMetaStream> {
+ list_all(prefix.to_owned()).await
Review comment:
also, if the method can return folders, the name of the structs returned (`FileMeta`) is kind of misleading.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] houqp commented on pull request #950: ObjectStore API to read from remote storage systems
Posted by GitBox <gi...@apache.org>.
houqp commented on pull request #950:
URL: https://github.com/apache/arrow-datafusion/pull/950#issuecomment-907525589
@rdettai on the topics of prefix semantic and filter pushdown, in my mind, these are higher level logic/abstractions that are not natively supported by the object store. For example, I am under the assumption that `TableProvider` is responsible for parsing the partition parameters from the URI `bucket/prefix?partition=year&partition=month` and constructing the list object API calls with appropriate prefixes. Similar for URIs like `bucket/prefix?versionAsOf=v2`, the delta table `TableProvider` should be responsible to parse out `versionAsOf=v2` and issue corresponding object list calls to gather the right list of commit/snapshot objects to fetch from the object store.
I am also having a hard time imaging how will filter push down expressions be used to help the object store build a better or more efficient API call. It seems like these kind of higher level logic should be handled at layers above ObjectStore? They seem to be object store agnostics. As a result, it would be better to just implement them once in abstraction layers like `TableProvider` instead of adding separate implementation in each type of object store.
Does this make sense to you? Or am I missing specific features from some object stores that can support native filter push down optimizations at the API level?
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] rdettai commented on a change in pull request #950: ObjectStore API to read from remote storage systems
Posted by GitBox <gi...@apache.org>.
rdettai commented on a change in pull request #950:
URL: https://github.com/apache/arrow-datafusion/pull/950#discussion_r697233530
##########
File path: datafusion/src/datasource/object_store/mod.rs
##########
@@ -0,0 +1,127 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+//! Object Store abstracts access to an underlying file/object storage.
+
+pub mod local;
+
+use std::collections::HashMap;
+use std::fmt::Debug;
+use std::pin::Pin;
+use std::sync::{Arc, RwLock};
+
+use async_trait::async_trait;
+use futures::{AsyncRead, Stream};
+
+use local::LocalFileSystem;
+
+use crate::error::{DataFusionError, Result};
+use chrono::Utc;
+
+/// Object Reader for one file in a object store
+#[async_trait]
+pub trait ObjectReader {
+ /// Get reader for a part [start, start + length] in the file asynchronously
+ async fn get_reader(&self, start: u64, length: usize) -> Result<Arc<dyn AsyncRead>>;
+
+ /// Get length for the file asynchronously
+ async fn length(&self) -> Result<u64>;
+}
+
+/// File meta we got from object store
+pub struct FileMeta {
+ /// Path of the file
+ pub path: String,
+ /// Last time the file was modified in UTC
+ pub last_modified: Option<chrono::DateTime<Utc>>,
+ /// File size in total
+ pub size: Option<u64>,
+}
+
+/// Stream of files get listed from object store
+pub type FileMetaStream =
+ Pin<Box<dyn Stream<Item = Result<FileMeta>> + Send + Sync + 'static>>;
+
+/// A ObjectStore abstracts access to an underlying file/object storage.
+/// It maps strings (e.g. URLs, filesystem paths, etc) to sources of bytes
+#[async_trait]
+pub trait ObjectStore: Sync + Send + Debug {
+ /// Returns all the files in path `prefix` asynchronously
+ async fn list(&self, prefix: &str) -> Result<FileMetaStream>;
+
+ /// Get object reader for one file
+ async fn get_reader(&self, file: FileMeta) -> Result<Arc<dyn ObjectReader>>;
Review comment:
I also find that it makes sense to pass the whole information package we got from the listing to `get_reader`.
nit: do we really need to be `async` here? as both methods on the `ObjectReader` trait are async, we could postpone any blocking operation to the invokation of the trait methods
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] houqp commented on pull request #950: ObjectStore API to read from remote storage systems
Posted by GitBox <gi...@apache.org>.
houqp commented on pull request #950:
URL: https://github.com/apache/arrow-datafusion/pull/950#issuecomment-912006894
@alamb is on vacation this week :P
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] yjshen commented on a change in pull request #950: ObjectStore API to read from remote storage systems
Posted by GitBox <gi...@apache.org>.
yjshen commented on a change in pull request #950:
URL: https://github.com/apache/arrow-datafusion/pull/950#discussion_r697992693
##########
File path: datafusion/src/datasource/object_store/mod.rs
##########
@@ -0,0 +1,130 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+//! Object Store abstracts access to an underlying file/object storage.
+
+pub mod local;
+
+use std::collections::HashMap;
+use std::fmt::Debug;
+use std::pin::Pin;
+use std::sync::{Arc, RwLock};
+
+use async_trait::async_trait;
+use futures::{AsyncRead, Stream};
+
+use local::LocalFileSystem;
+
+use crate::error::{DataFusionError, Result};
+use chrono::Utc;
+
+/// Object Reader for one file in a object store
+#[async_trait]
+pub trait ObjectReader {
+ /// Get reader for a part [start, start + length] in the file asynchronously
+ async fn chunk_reader(&self, start: u64, length: usize)
+ -> Result<Arc<dyn AsyncRead>>;
+
+ /// Get length for the file
+ fn length(&self) -> u64;
+}
+
+/// File meta we got from object store
+#[derive(Debug)]
+pub struct FileMeta {
+ /// Path of the file
+ pub path: String,
+ /// Last time the file was modified in UTC
+ pub last_modified: Option<chrono::DateTime<Utc>>,
+ /// File size in total
+ pub size: u64,
+}
+
+/// Stream of files get listed from object store
+pub type FileMetaStream =
+ Pin<Box<dyn Stream<Item = Result<FileMeta>> + Send + Sync + 'static>>;
+
+/// A ObjectStore abstracts access to an underlying file/object storage.
+/// It maps strings (e.g. URLs, filesystem paths, etc) to sources of bytes
+#[async_trait]
+pub trait ObjectStore: Sync + Send + Debug {
+ /// Returns all the files in path `prefix` asynchronously.
+ async fn list(&self, prefix: &str) -> Result<FileMetaStream>;
+
+ /// Get object reader for one file
+ fn file_reader(&self, file: FileMeta) -> Result<Arc<dyn ObjectReader>>;
Review comment:
Sorry I didn't fully understand your concern at the first time, fixed now.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] yjshen commented on a change in pull request #950: ObjectStore API to read from remote storage systems
Posted by GitBox <gi...@apache.org>.
yjshen commented on a change in pull request #950:
URL: https://github.com/apache/arrow-datafusion/pull/950#discussion_r698522038
##########
File path: datafusion/src/datasource/object_store/local.rs
##########
@@ -0,0 +1,173 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+//! Object store that represents the Local File System.
+
+use std::fs::Metadata;
+use std::sync::Arc;
+
+use async_trait::async_trait;
+use futures::{stream, AsyncRead, StreamExt};
+
+use crate::datasource::object_store::{
+ FileMeta, FileMetaStream, ObjectReader, ObjectStore,
+};
+use crate::error::DataFusionError;
+use crate::error::Result;
+
+#[derive(Debug)]
+/// Local File System as Object Store.
+pub struct LocalFileSystem;
+
+#[async_trait]
+impl ObjectStore for LocalFileSystem {
+ async fn list(
+ &self,
+ prefix: &str,
+ _delimiter: Option<String>,
+ ) -> Result<FileMetaStream> {
+ list_all(prefix.to_owned()).await
Review comment:
I think it's fine to leave out the TODO since the partitioned file read in not supported?
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] yjshen commented on pull request #950: ObjectStore API to read from remote storage systems
Posted by GitBox <gi...@apache.org>.
yjshen commented on pull request #950:
URL: https://github.com/apache/arrow-datafusion/pull/950#issuecomment-907616198
@rdettai @houqp After checking DataFusion and delta-rs code, I think it is more natural to have the TableProvider dealing with filters; it's **the** abstraction over a table, therefore also a suitable entity for table partitions (inferred or user-provided). The laziness of file listing could be achieved by TableProvider and rely on ObjectStore to only listing related files.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] alamb commented on a change in pull request #950: ObjectStore API to read from remote storage systems
Posted by GitBox <gi...@apache.org>.
alamb commented on a change in pull request #950:
URL: https://github.com/apache/arrow-datafusion/pull/950#discussion_r699396716
##########
File path: datafusion/src/datasource/object_store/local.rs
##########
@@ -0,0 +1,173 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+//! Object store that represents the Local File System.
+
+use std::fs::Metadata;
+use std::sync::Arc;
+
+use async_trait::async_trait;
+use futures::{stream, AsyncRead, StreamExt};
+
+use crate::datasource::object_store::{
+ FileMeta, FileMetaStream, ObjectReader, ObjectStore,
+};
+use crate::error::DataFusionError;
+use crate::error::Result;
+
+#[derive(Debug)]
+/// Local File System as Object Store.
+pub struct LocalFileSystem;
+
+#[async_trait]
+impl ObjectStore for LocalFileSystem {
+ async fn list(
+ &self,
+ prefix: &str,
+ _delimiter: Option<String>,
+ ) -> Result<FileMetaStream> {
+ list_all(prefix.to_owned()).await
Review comment:
FWIW in IOx we went back and forth on this as well. There we have one LIST API https://github.com/influxdata/influxdb_iox/blob/main/object_store/src/lib.rs#L95-L98 that takes an`ObjectStorePath` and the path itself can distinguish between "is a (logical) directory" or "is a filename" : https://github.com/influxdata/influxdb_iox/blob/main/object_store/src/path.rs#L30-L42
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] yjshen commented on a change in pull request #950: ObjectStore API to read from remote storage systems
Posted by GitBox <gi...@apache.org>.
yjshen commented on a change in pull request #950:
URL: https://github.com/apache/arrow-datafusion/pull/950#discussion_r697161330
##########
File path: datafusion/src/datasource/object_store/mod.rs
##########
@@ -0,0 +1,127 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+//! Object Store abstracts access to an underlying file/object storage.
+
+pub mod local;
+
+use std::collections::HashMap;
+use std::fmt::Debug;
+use std::pin::Pin;
+use std::sync::{Arc, RwLock};
+
+use async_trait::async_trait;
+use futures::{AsyncRead, Stream};
+
+use local::LocalFileSystem;
+
+use crate::error::{DataFusionError, Result};
+use chrono::Utc;
+
+/// Object Reader for one file in a object store
+#[async_trait]
+pub trait ObjectReader {
+ /// Get reader for a part [start, start + length] in the file asynchronously
+ async fn get_reader(&self, start: u64, length: usize) -> Result<Arc<dyn AsyncRead>>;
+
+ /// Get length for the file asynchronously
+ async fn length(&self) -> Result<u64>;
+}
+
+/// File meta we got from object store
+pub struct FileMeta {
+ /// Path of the file
+ pub path: String,
+ /// Last time the file was modified in UTC
+ pub last_modified: Option<chrono::DateTime<Utc>>,
+ /// File size in total
+ pub size: Option<u64>,
+}
+
+/// Stream of files get listed from object store
+pub type FileMetaStream =
+ Pin<Box<dyn Stream<Item = Result<FileMeta>> + Send + Sync + 'static>>;
+
+/// A ObjectStore abstracts access to an underlying file/object storage.
+/// It maps strings (e.g. URLs, filesystem paths, etc) to sources of bytes
+#[async_trait]
+pub trait ObjectStore: Sync + Send + Debug {
+ /// Returns all the files in path `prefix` asynchronously
+ async fn list(&self, prefix: &str) -> Result<FileMetaStream>;
+
+ /// Get object reader for one file
+ async fn get_reader(&self, file: FileMeta) -> Result<Arc<dyn ObjectReader>>;
Review comment:
The created reader might need to get file length I think? The current `ChunkReader` sub-trait `Length`, I'm not sure if we still need it when @alamb changes Parquet reader to async.
```rust
/// Get length for the file asynchronously
async fn length(&self) -> Result<u64>;
```
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] alamb commented on a change in pull request #950: ObjectStore API to read from remote storage systems
Posted by GitBox <gi...@apache.org>.
alamb commented on a change in pull request #950:
URL: https://github.com/apache/arrow-datafusion/pull/950#discussion_r699396716
##########
File path: datafusion/src/datasource/object_store/local.rs
##########
@@ -0,0 +1,173 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+//! Object store that represents the Local File System.
+
+use std::fs::Metadata;
+use std::sync::Arc;
+
+use async_trait::async_trait;
+use futures::{stream, AsyncRead, StreamExt};
+
+use crate::datasource::object_store::{
+ FileMeta, FileMetaStream, ObjectReader, ObjectStore,
+};
+use crate::error::DataFusionError;
+use crate::error::Result;
+
+#[derive(Debug)]
+/// Local File System as Object Store.
+pub struct LocalFileSystem;
+
+#[async_trait]
+impl ObjectStore for LocalFileSystem {
+ async fn list(
+ &self,
+ prefix: &str,
+ _delimiter: Option<String>,
+ ) -> Result<FileMetaStream> {
+ list_all(prefix.to_owned()).await
Review comment:
FWIW in IOx we went back and forth on this as well. There we have one LIST API https://github.com/influxdata/influxdb_iox/blob/main/object_store/src/lib.rs#L95-L98 that takes an`ObjectStorePath` and the path itself can distinguish between "is a (logical) directory" or "is a filename" : https://github.com/influxdata/influxdb_iox/blob/main/object_store/src/path.rs#L30-L42
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] yjshen commented on a change in pull request #950: ObjectStore API to read from remote storage systems
Posted by GitBox <gi...@apache.org>.
yjshen commented on a change in pull request #950:
URL: https://github.com/apache/arrow-datafusion/pull/950#discussion_r698524616
##########
File path: datafusion/src/datasource/object_store/mod.rs
##########
@@ -0,0 +1,130 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+//! Object Store abstracts access to an underlying file/object storage.
+
+pub mod local;
+
+use std::collections::HashMap;
+use std::fmt::Debug;
+use std::pin::Pin;
+use std::sync::{Arc, RwLock};
+
+use async_trait::async_trait;
+use futures::{AsyncRead, Stream};
+
+use local::LocalFileSystem;
+
+use crate::error::{DataFusionError, Result};
+use chrono::Utc;
+
+/// Object Reader for one file in a object store
+#[async_trait]
+pub trait ObjectReader {
+ /// Get reader for a part [start, start + length] in the file asynchronously
+ async fn chunk_reader(&self, start: u64, length: usize)
+ -> Result<Arc<dyn AsyncRead>>;
+
+ /// Get length for the file
+ fn length(&self) -> u64;
+}
+
+/// File meta we got from object store
+#[derive(Debug)]
+pub struct FileMeta {
+ /// Path of the file
+ pub path: String,
+ /// Last time the file was modified in UTC
+ pub last_modified: Option<chrono::DateTime<Utc>>,
+ /// File size in total
+ pub size: u64,
+}
+
+/// Stream of files get listed from object store
+pub type FileMetaStream =
+ Pin<Box<dyn Stream<Item = Result<FileMeta>> + Send + Sync + 'static>>;
+
+/// A ObjectStore abstracts access to an underlying file/object storage.
+/// It maps strings (e.g. URLs, filesystem paths, etc) to sources of bytes
+#[async_trait]
+pub trait ObjectStore: Sync + Send + Debug {
+ /// Returns all the files in path `prefix` asynchronously.
+ async fn list(&self, prefix: &str) -> Result<FileMetaStream>;
+
+ /// Get object reader for one file
+ fn file_reader(&self, file: FileMeta) -> Result<Arc<dyn ObjectReader>>;
Review comment:
Reverted to sync method now.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] rdettai edited a comment on pull request #950: ObjectStore API to read from remote storage systems
Posted by GitBox <gi...@apache.org>.
rdettai edited a comment on pull request #950:
URL: https://github.com/apache/arrow-datafusion/pull/950#issuecomment-907769383
> I am under the assumption that TableProvider is responsible for parsing the partition parameters from the URI bucket/prefix?partition=year&partition=month and constructing the list object API calls with appropriate prefixes
In the case of hive partitioning, you need an extra capability of the object store to do that: **list the folders**. Otherwise you cannot now what partitions are there and build the appropriate prefixes. Maybe this example helps getting more context: https://docs.google.com/document/d/1nKXPvolft1_DuVjVK7ceOx37arCdQLKc_Im-TfZh9dk/edit?usp=sharing
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] houqp commented on a change in pull request #950: ObjectStore API to read from remote storage systems
Posted by GitBox <gi...@apache.org>.
houqp commented on a change in pull request #950:
URL: https://github.com/apache/arrow-datafusion/pull/950#discussion_r697159560
##########
File path: datafusion/src/datasource/object_store/mod.rs
##########
@@ -0,0 +1,127 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+//! Object Store abstracts access to an underlying file/object storage.
+
+pub mod local;
+
+use std::collections::HashMap;
+use std::fmt::Debug;
+use std::pin::Pin;
+use std::sync::{Arc, RwLock};
+
+use async_trait::async_trait;
+use futures::{AsyncRead, Stream};
+
+use local::LocalFileSystem;
+
+use crate::error::{DataFusionError, Result};
+use chrono::Utc;
+
+/// Object Reader for one file in a object store
+#[async_trait]
+pub trait ObjectReader {
+ /// Get reader for a part [start, start + length] in the file asynchronously
+ async fn get_reader(&self, start: u64, length: usize) -> Result<Arc<dyn AsyncRead>>;
+
+ /// Get length for the file asynchronously
+ async fn length(&self) -> Result<u64>;
+}
+
+/// File meta we got from object store
+pub struct FileMeta {
+ /// Path of the file
+ pub path: String,
+ /// Last time the file was modified in UTC
+ pub last_modified: Option<chrono::DateTime<Utc>>,
+ /// File size in total
+ pub size: Option<u64>,
+}
+
+/// Stream of files get listed from object store
+pub type FileMetaStream =
+ Pin<Box<dyn Stream<Item = Result<FileMeta>> + Send + Sync + 'static>>;
+
+/// A ObjectStore abstracts access to an underlying file/object storage.
+/// It maps strings (e.g. URLs, filesystem paths, etc) to sources of bytes
+#[async_trait]
+pub trait ObjectStore: Sync + Send + Debug {
+ /// Returns all the files in path `prefix` asynchronously
+ async fn list(&self, prefix: &str) -> Result<FileMetaStream>;
+
+ /// Get object reader for one file
+ async fn get_reader(&self, file: FileMeta) -> Result<Arc<dyn ObjectReader>>;
Review comment:
Do we need to provide file size and last_modified field for object content get? If not, I think it would be better to accept file path as a simple `&str` here.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] rdettai commented on pull request #950: ObjectStore API to read from remote storage systems
Posted by GitBox <gi...@apache.org>.
rdettai commented on pull request #950:
URL: https://github.com/apache/arrow-datafusion/pull/950#issuecomment-907769383
> I am under the assumption that TableProvider is responsible for parsing the partition parameters from the URI bucket/prefix?partition=year&partition=month and constructing the list object API calls with appropriate prefixes
You need an extra capability of the object store to do that: **list the folders**. Otherwise you cannot now what partitions are there and build the appropriate prefixes. Maybe this example helps getting more context: https://docs.google.com/document/d/1nKXPvolft1_DuVjVK7ceOx37arCdQLKc_Im-TfZh9dk/edit?usp=sharing
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] yjshen commented on pull request #950: ObjectStore API to read from remote storage systems
Posted by GitBox <gi...@apache.org>.
yjshen commented on pull request #950:
URL: https://github.com/apache/arrow-datafusion/pull/950#issuecomment-917119367
Thanks again for all the guidance and help @houqp @rdettai @alamb 🎉 .
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] houqp commented on a change in pull request #950: ObjectStore API to read from remote storage systems
Posted by GitBox <gi...@apache.org>.
houqp commented on a change in pull request #950:
URL: https://github.com/apache/arrow-datafusion/pull/950#discussion_r698945223
##########
File path: datafusion/src/datasource/object_store/local.rs
##########
@@ -0,0 +1,173 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+//! Object store that represents the Local File System.
+
+use std::fs::Metadata;
+use std::sync::Arc;
+
+use async_trait::async_trait;
+use futures::{stream, AsyncRead, StreamExt};
+
+use crate::datasource::object_store::{
+ FileMeta, FileMetaStream, ObjectReader, ObjectStore,
+};
+use crate::error::DataFusionError;
+use crate::error::Result;
+
+#[derive(Debug)]
+/// Local File System as Object Store.
+pub struct LocalFileSystem;
+
+#[async_trait]
+impl ObjectStore for LocalFileSystem {
+ async fn list(
+ &self,
+ prefix: &str,
+ _delimiter: Option<String>,
+ ) -> Result<FileMetaStream> {
+ list_all(prefix.to_owned()).await
Review comment:
naming is hard indeed :D
how about `ListEntry` or `ListItem`? we can define it as an enum of `FileMeta` and prefix string to distinguish between regular objects and "folder" prefixes.
References:
S3 has prefixes returned in the ` <CommonPrefixes>` section: https://docs.aws.amazon.com/AmazonS3/latest/API/API_ListObjectsV2.html
GCS has prefixes returned in response object's prefixes property: https://cloud.google.com/storage/docs/json_api/v1/objects/list#response
Azure blob is similar to S3 and has prefix strings returned in its `BlobPrefix` section: https://docs.microsoft.com/en-us/rest/api/storageservices/enumerating-blob-resources#DelimitedBlobList
##########
File path: datafusion/src/datasource/object_store/local.rs
##########
@@ -0,0 +1,173 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+//! Object store that represents the Local File System.
+
+use std::fs::Metadata;
+use std::sync::Arc;
+
+use async_trait::async_trait;
+use futures::{stream, AsyncRead, StreamExt};
+
+use crate::datasource::object_store::{
+ FileMeta, FileMetaStream, ObjectReader, ObjectStore,
+};
+use crate::error::DataFusionError;
+use crate::error::Result;
+
+#[derive(Debug)]
+/// Local File System as Object Store.
+pub struct LocalFileSystem;
+
+#[async_trait]
+impl ObjectStore for LocalFileSystem {
+ async fn list(
+ &self,
+ prefix: &str,
+ _delimiter: Option<String>,
+ ) -> Result<FileMetaStream> {
+ list_all(prefix.to_owned()).await
Review comment:
There might be value in having two list apis as well, one for listing objects without delimiters and returns stream of `FileMeta`, the other for listing "directories" with delimiters that could return either `FileMeta` or prefix strings.
I am thinking there are use-case where we only need to perform listing without delimiters. For example after we have partition "folders" fully discovered, we will just use list api call to get the final list of objects using partition prefixes. Returning a stream of `FileMeta` directly in this case could avoid enum matching overhead on the call site.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] houqp commented on pull request #950: ObjectStore API to read from remote storage systems
Posted by GitBox <gi...@apache.org>.
houqp commented on pull request #950:
URL: https://github.com/apache/arrow-datafusion/pull/950#issuecomment-917154017
Amazing work on laying out a solid foundation for IO abstraction in datafusion @yjshen !
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] rdettai commented on a change in pull request #950: ObjectStore API to read from remote storage systems
Posted by GitBox <gi...@apache.org>.
rdettai commented on a change in pull request #950:
URL: https://github.com/apache/arrow-datafusion/pull/950#discussion_r698515026
##########
File path: datafusion/src/datasource/object_store/local.rs
##########
@@ -0,0 +1,173 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+//! Object store that represents the Local File System.
+
+use std::fs::Metadata;
+use std::sync::Arc;
+
+use async_trait::async_trait;
+use futures::{stream, AsyncRead, StreamExt};
+
+use crate::datasource::object_store::{
+ FileMeta, FileMetaStream, ObjectReader, ObjectStore,
+};
+use crate::error::DataFusionError;
+use crate::error::Result;
+
+#[derive(Debug)]
+/// Local File System as Object Store.
+pub struct LocalFileSystem;
+
+#[async_trait]
+impl ObjectStore for LocalFileSystem {
+ async fn list(
+ &self,
+ prefix: &str,
+ _delimiter: Option<String>,
+ ) -> Result<FileMetaStream> {
+ list_all(prefix.to_owned()).await
Review comment:
we can maybe mark this as `// TODO` as the delimiter is ignored 😉
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] rdettai commented on pull request #950: ObjectStore API to read from remote storage systems
Posted by GitBox <gi...@apache.org>.
rdettai commented on pull request #950:
URL: https://github.com/apache/arrow-datafusion/pull/950#issuecomment-907820496
@yjshen @houqp you are right, I think that maybe with the delimiter argument added to `ObjectStore.list()`, the TableProvider would be able to get all the information it needs to efficiently prune the partitions in a raw hive partitioning setup! This way we leave the ObjectStore abstraction with only core object store capabilities 😄
I am still wondering how we could replicate the hive partitioning support for `TableProvider`s relative to different file formats (`ParquetTable`, `CsvFile`, `NdJsonFile`). Actually, same would apply for a HiveCatalogTable that could actually store any file format. It makes me feel that something is wrong with how the different abstractions are organized. This is being discussed in #133.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] houqp commented on pull request #950: ObjectStore API to read from remote storage systems
Posted by GitBox <gi...@apache.org>.
houqp commented on pull request #950:
URL: https://github.com/apache/arrow-datafusion/pull/950#issuecomment-907984810
> I am still wondering how we could replicate the hive partitioning support for TableProviders relative to different file formats (ParquetTable, CsvFile, NdJsonFile).
For raw file tables like a folder of parquet, csv and ndjson files, I think the partition discovery and pruning logic should be agnostic to both file formats and object storages. We could implement this logic as a shared module, which gets invoked in ParquetTable, CsvFile and NdJsonFile table providers to resolve the filtered down object list. Then these table providers will pass down the object paths to corresponding format specific physical execution plans.
> Actually, same would apply for a HiveCatalogTable that could actually reference various file formats. It makes me feel that something is wrong with how the different abstractions are organized. This is being discussed in #133.
Given that hive catalog manages a collection of tables with their corresponding metadata, I think CatalogProvider is probably the right abstraction for it. The Hive catalog provider could query hive metastore for a given schema and table name. It can then construct a format specific table provider based on hive metastore response.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] rdettai commented on a change in pull request #950: ObjectStore API to read from remote storage systems
Posted by GitBox <gi...@apache.org>.
rdettai commented on a change in pull request #950:
URL: https://github.com/apache/arrow-datafusion/pull/950#discussion_r698515026
##########
File path: datafusion/src/datasource/object_store/local.rs
##########
@@ -0,0 +1,173 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+//! Object store that represents the Local File System.
+
+use std::fs::Metadata;
+use std::sync::Arc;
+
+use async_trait::async_trait;
+use futures::{stream, AsyncRead, StreamExt};
+
+use crate::datasource::object_store::{
+ FileMeta, FileMetaStream, ObjectReader, ObjectStore,
+};
+use crate::error::DataFusionError;
+use crate::error::Result;
+
+#[derive(Debug)]
+/// Local File System as Object Store.
+pub struct LocalFileSystem;
+
+#[async_trait]
+impl ObjectStore for LocalFileSystem {
+ async fn list(
+ &self,
+ prefix: &str,
+ _delimiter: Option<String>,
+ ) -> Result<FileMetaStream> {
+ list_all(prefix.to_owned()).await
Review comment:
we can maybe mark this as `// TODO` as the delimiter is ignored 😉
##########
File path: datafusion/src/datasource/object_store/local.rs
##########
@@ -0,0 +1,173 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+//! Object store that represents the Local File System.
+
+use std::fs::Metadata;
+use std::sync::Arc;
+
+use async_trait::async_trait;
+use futures::{stream, AsyncRead, StreamExt};
+
+use crate::datasource::object_store::{
+ FileMeta, FileMetaStream, ObjectReader, ObjectStore,
+};
+use crate::error::DataFusionError;
+use crate::error::Result;
+
+#[derive(Debug)]
+/// Local File System as Object Store.
+pub struct LocalFileSystem;
+
+#[async_trait]
+impl ObjectStore for LocalFileSystem {
+ async fn list(
+ &self,
+ prefix: &str,
+ _delimiter: Option<String>,
+ ) -> Result<FileMetaStream> {
+ list_all(prefix.to_owned()).await
Review comment:
it seems to me that if you don't comply to the API, a TODO is appropriate even if nobody is using the feature yet 😃
##########
File path: datafusion/src/datasource/object_store/local.rs
##########
@@ -0,0 +1,173 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+//! Object store that represents the Local File System.
+
+use std::fs::Metadata;
+use std::sync::Arc;
+
+use async_trait::async_trait;
+use futures::{stream, AsyncRead, StreamExt};
+
+use crate::datasource::object_store::{
+ FileMeta, FileMetaStream, ObjectReader, ObjectStore,
+};
+use crate::error::DataFusionError;
+use crate::error::Result;
+
+#[derive(Debug)]
+/// Local File System as Object Store.
+pub struct LocalFileSystem;
+
+#[async_trait]
+impl ObjectStore for LocalFileSystem {
+ async fn list(
+ &self,
+ prefix: &str,
+ _delimiter: Option<String>,
+ ) -> Result<FileMetaStream> {
+ list_all(prefix.to_owned()).await
Review comment:
also, if the API can return folders, the name of the structs returned (`FileMeta`) is kind of misleading.
##########
File path: datafusion/src/datasource/object_store/local.rs
##########
@@ -0,0 +1,173 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+//! Object store that represents the Local File System.
+
+use std::fs::Metadata;
+use std::sync::Arc;
+
+use async_trait::async_trait;
+use futures::{stream, AsyncRead, StreamExt};
+
+use crate::datasource::object_store::{
+ FileMeta, FileMetaStream, ObjectReader, ObjectStore,
+};
+use crate::error::DataFusionError;
+use crate::error::Result;
+
+#[derive(Debug)]
+/// Local File System as Object Store.
+pub struct LocalFileSystem;
+
+#[async_trait]
+impl ObjectStore for LocalFileSystem {
+ async fn list(
+ &self,
+ prefix: &str,
+ _delimiter: Option<String>,
+ ) -> Result<FileMetaStream> {
+ list_all(prefix.to_owned()).await
Review comment:
also, if the method can return folders, the name of the structs returned (`FileMeta`) is kind of misleading.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] yjshen commented on pull request #950: ObjectStore API to read from remote storage systems
Posted by GitBox <gi...@apache.org>.
yjshen commented on pull request #950:
URL: https://github.com/apache/arrow-datafusion/pull/950#issuecomment-907836854
Thanks @rdettai @houqp for the optional delimiter.
I was thinking `list` API could deal with multiple partition columns by listing one directory at a time for filesystems (instead of listing leaf files recursively for non-partitioned tables), therefore could achieve a lazy listing when implementing ObjectStore.
After taking both partitioned and non-partitioned tables in one remote storage into consideration, this original `list` signature would lead to two `ObjectStore` implementations for one storage system, one for partitioned and one for non-partitioned. It's wired. I like the idea of an optional delimiter because it brings a lot of flexibility.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] yjshen commented on pull request #950: ObjectStore API to read from remote storage systems
Posted by GitBox <gi...@apache.org>.
yjshen commented on pull request #950:
URL: https://github.com/apache/arrow-datafusion/pull/950#issuecomment-908889969
Python test failure seems unrelated, could you please retrigger this single GitHub action @houqp ? Thanks!
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org