-
Notifications
You must be signed in to change notification settings - Fork 1.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Introduce ObjectStoreProvider to create an object store based on the url #2906
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -162,7 +162,7 @@ pub fn split_files( | |
pub async fn pruned_partition_list<'a>( | ||
store: &'a dyn ObjectStore, | ||
table_path: &'a ListingTableUrl, | ||
filters: &[Expr], | ||
filters: &'a [Expr], | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why this change? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. When compiling, if without this, it throws lifetime exception. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think it is a rust complier bug, the original code should not compile. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I also see this error occasionally (but not always) locally There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It seems it's the Rust compiler bug. From my understanding for the lifetime, it's necessary for us to add 'a |
||
file_extension: &'a str, | ||
table_partition_cols: &'a [String], | ||
) -> Result<BoxStream<'a, Result<PartitionedFile>>> { | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -81,10 +81,19 @@ impl std::fmt::Display for ObjectStoreUrl { | |
} | ||
} | ||
|
||
/// Object store provider can detector an object store based on the url | ||
pub trait ObjectStoreProvider: Send + Sync + 'static { | ||
/// Detector a suitable object store based on its url if possible | ||
/// Return the key and object store | ||
fn get_by_url(&self, url: &Url) -> Option<Arc<dyn ObjectStore>>; | ||
} | ||
|
||
/// Object store registry | ||
#[derive(Clone)] | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why is this needed, afaict all locations use |
||
pub struct ObjectStoreRegistry { | ||
/// A map from scheme to object store that serve list / read operations for the store | ||
object_stores: RwLock<HashMap<String, Arc<dyn ObjectStore>>>, | ||
object_stores: Arc<RwLock<HashMap<String, Arc<dyn ObjectStore>>>>, | ||
provider: Option<Arc<dyn ObjectStoreProvider>>, | ||
} | ||
|
||
impl std::fmt::Debug for ObjectStoreRegistry { | ||
|
@@ -105,13 +114,19 @@ impl Default for ObjectStoreRegistry { | |
} | ||
|
||
impl ObjectStoreRegistry { | ||
/// By default the self detector is None | ||
pub fn new() -> Self { | ||
ObjectStoreRegistry::new_with_provider(None) | ||
} | ||
|
||
/// 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 { | ||
pub fn new_with_provider(provider: Option<Arc<dyn ObjectStoreProvider>>) -> Self { | ||
let mut map: HashMap<String, Arc<dyn ObjectStore>> = HashMap::new(); | ||
map.insert("file://".to_string(), Arc::new(LocalFileSystem::new())); | ||
Self { | ||
object_stores: RwLock::new(map), | ||
object_stores: Arc::new(RwLock::new(map)), | ||
provider, | ||
} | ||
} | ||
|
||
|
@@ -132,19 +147,43 @@ impl ObjectStoreRegistry { | |
/// | ||
/// - URL with scheme `file:///` or no schema will return the default LocalFS store | ||
/// - URL with scheme `s3://bucket/` will return the S3 store if it's registered | ||
/// - URL with scheme `hdfs://hostname:port/` will return the hdfs store if it's registered | ||
/// | ||
pub fn get_by_url(&self, url: impl AsRef<Url>) -> Result<Arc<dyn ObjectStore>> { | ||
let url = url.as_ref(); | ||
let s = &url[url::Position::BeforeScheme..url::Position::AfterHost]; | ||
let stores = self.object_stores.read(); | ||
let store = stores.get(s).ok_or_else(|| { | ||
DataFusionError::Internal(format!( | ||
"No suitable object store found for {}", | ||
url | ||
)) | ||
})?; | ||
|
||
Ok(store.clone()) | ||
// First check whether can get object store from registry | ||
let store = { | ||
let stores = self.object_stores.read(); | ||
let s = &url[url::Position::BeforeScheme..url::Position::BeforePath]; | ||
stores.get(s).cloned() | ||
}; | ||
|
||
// If not, then try to detector based on its url. | ||
let store = store | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This currently has a thread race, it needs to:
To be honest, this could probably switch to just using a There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't think so. The RWLock is better than just using Mutex. Here, the read case will happen frequently. While the write case only happens a few times. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In the uncontended case, which is extremely likely given how short the critical section is, they will perform exactly the same - if anything the Mutex might be marginally faster. It was more an observation that the complexity of using a RWLock is probably not actually yielding any return. A simple Mutex would allow you to use |
||
.or_else(|| { | ||
if let Some(provider) = &self.provider { | ||
// If detected, register it | ||
if let Some(store) = provider.get_by_url(url) { | ||
let mut stores = self.object_stores.write(); | ||
let key = | ||
&url[url::Position::BeforeScheme..url::Position::BeforePath]; | ||
stores.insert(key.to_owned(), store.clone()); | ||
Some(store) | ||
} else { | ||
None | ||
} | ||
} else { | ||
None | ||
} | ||
}) | ||
.ok_or_else(|| { | ||
DataFusionError::Internal(format!( | ||
"No suitable object store found for {}", | ||
url | ||
)) | ||
})?; | ||
|
||
Ok(store) | ||
} | ||
} | ||
|
||
|
@@ -190,6 +229,14 @@ mod tests { | |
assert_eq!(err.to_string(), "Execution error: ObjectStoreUrl must only contain scheme and authority, got: /foo"); | ||
} | ||
|
||
#[test] | ||
fn test_get_by_url_hdfs() { | ||
let sut = ObjectStoreRegistry::default(); | ||
sut.register_store("hdfs", "localhost:8020", Arc::new(LocalFileSystem::new())); | ||
let url = ListingTableUrl::parse("hdfs://localhost:8020/key").unwrap(); | ||
sut.get_by_url(&url).unwrap(); | ||
} | ||
|
||
#[test] | ||
fn test_get_by_url_s3() { | ||
let sut = ObjectStoreRegistry::default(); | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -57,12 +57,13 @@ impl RuntimeEnv { | |
let RuntimeConfig { | ||
memory_manager, | ||
disk_manager, | ||
object_store_registry, | ||
} = config; | ||
|
||
Ok(Self { | ||
memory_manager: MemoryManager::new(memory_manager), | ||
disk_manager: DiskManager::try_new(disk_manager)?, | ||
object_store_registry: Arc::new(ObjectStoreRegistry::new()), | ||
object_store_registry: Arc::new(object_store_registry), | ||
}) | ||
} | ||
|
||
|
@@ -121,6 +122,8 @@ pub struct RuntimeConfig { | |
pub disk_manager: DiskManagerConfig, | ||
/// MemoryManager to limit access to memory | ||
pub memory_manager: MemoryManagerConfig, | ||
/// ObjectStoreRegistry to get object store based on url | ||
pub object_store_registry: ObjectStoreRegistry, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This should possibly be There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why? Other properties are not wrapped with Arc. And for one env, there should be only one runtime_env and its related properties. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Two reasons:
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I see. Agree with you that here better to use Arc There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
} | ||
|
||
impl RuntimeConfig { | ||
|
@@ -141,6 +144,15 @@ impl RuntimeConfig { | |
self | ||
} | ||
|
||
/// Customize object store registry | ||
pub fn with_object_store_registry( | ||
mut self, | ||
object_store_registry: ObjectStoreRegistry, | ||
) -> Self { | ||
self.object_store_registry = object_store_registry; | ||
self | ||
} | ||
|
||
/// Specify the total memory to use while running the DataFusion | ||
/// plan to `max_memory * memory_fraction` in bytes. | ||
/// | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if this logic would be better handled in get_data_dir (to remove any trailing /)? As it stands I'm surprised just making this change here is sufficient?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For different OS, the detailed parsing logic is different. It seems the logic here is too trivial and should be refined.