Skip to content

Commit ff694ec

Browse files
peterxcliclaude
andcommitted
fix: harden CometObjectStoreRegistry per review feedback
- install the ABFS-aware registry on the initRecordBatchReader (native_iceberg_compat) path so per-container isolation does not depend on that context staying private to a single reader - document the registry as a temporary workaround for apache/datafusion#23935 so it can be removed at the version bump - implement deregister_store, the one registry path that bypassed the Azure store map (previously returned not-implemented) - use std::sync::RwLock for consistency with the rest of the file - rename test_s3_store_cache_keys_by_host to test_non_azure_schemes_ignore_userinfo and note the synthetic URL shape Co-Authored-By: Claude Fable 5 <[email protected]>
1 parent a740998 commit ff694ec

2 files changed

Lines changed: 59 additions & 11 deletions

File tree

‎native/core/src/parquet/mod.rs‎

Lines changed: 17 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -49,9 +49,12 @@ use crate::execution::utils::SparkArrowConvert;
4949
use crate::jvm_bridge::JVMClasses;
5050
use crate::parquet::encryption_support::{CometEncryptionFactory, ENCRYPTION_FACTORY_ID};
5151
use crate::parquet::parquet_exec::init_datasource_exec;
52-
use crate::parquet::parquet_support::prepare_object_store_with_configs;
52+
use crate::parquet::parquet_support::{
53+
prepare_object_store_with_configs, CometObjectStoreRegistry,
54+
};
5355
use arrow::array::{Array, RecordBatch};
5456
use datafusion::datasource::listing::PartitionedFile;
57+
use datafusion::execution::runtime_env::RuntimeEnvBuilder;
5558
use datafusion::execution::SendableRecordBatchStream;
5659
use datafusion::physical_plan::ExecutionPlan;
5760
use datafusion::prelude::{SessionConfig, SessionContext};
@@ -153,8 +156,19 @@ pub unsafe extern "system" fn Java_org_apache_comet_parquet_Native_initRecordBat
153156
try_unwrap_or_throw(&e, |env| unsafe {
154157
JVMClasses::init(env);
155158
let session_config = SessionConfig::new().with_batch_size(batch_size as usize);
156-
let planner =
157-
PhysicalPlanner::new(Arc::new(SessionContext::new_with_config(session_config)), 0);
159+
// This context is private to one reader, so its registry only ever sees a single
160+
// store; the ABFS-aware registry is still installed so per-container isolation does
161+
// not silently depend on that stays-private invariant.
162+
let runtime_env = RuntimeEnvBuilder::new()
163+
.with_object_store_registry(Arc::new(CometObjectStoreRegistry::default()))
164+
.build_arc()?;
165+
let planner = PhysicalPlanner::new(
166+
Arc::new(SessionContext::new_with_config_rt(
167+
session_config,
168+
runtime_env,
169+
)),
170+
0,
171+
);
158172
let session_ctx = planner.session_ctx();
159173

160174
let path: String = file_path.try_to_string(env).unwrap();

‎native/core/src/parquet/parquet_support.rs‎

Lines changed: 42 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -44,7 +44,11 @@ use parquet::arrow::PARQUET_FIELD_ID_META_KEY;
4444
use std::collections::HashMap;
4545
use std::sync::OnceLock;
4646
use std::time::Duration;
47-
use std::{collections::hash_map::DefaultHasher, hash::Hasher, sync::RwLock};
47+
use std::{
48+
collections::hash_map::DefaultHasher,
49+
hash::Hasher,
50+
sync::{PoisonError, RwLock},
51+
};
4852
use std::{fmt::Debug, hash::Hash, sync::Arc};
4953
use url::Url;
5054

@@ -430,10 +434,17 @@ fn object_store_url_key(url: &Url) -> String {
430434
)
431435
}
432436

437+
/// An [`ObjectStoreRegistry`] that keys ABFS stores by the full authority
438+
/// (`scheme://container@host:port`) instead of DataFusion's `scheme://host:port`.
439+
///
440+
/// This is a workaround for `DefaultObjectStoreRegistry`, whose key drops the userinfo
441+
/// component, collapsing every container in an Azure storage account onto a single store.
442+
/// Fixed upstream in <https://github.com/apache/datafusion/pull/23935>; this registry can be
443+
/// removed once Comet is on a DataFusion release containing that fix.
433444
#[derive(Debug, Default)]
434445
pub(crate) struct CometObjectStoreRegistry {
435446
default: DefaultObjectStoreRegistry,
436-
azure_stores: parking_lot::RwLock<HashMap<String, Arc<dyn ObjectStore>>>,
447+
azure_stores: RwLock<HashMap<String, Arc<dyn ObjectStore>>>,
437448
}
438449

439450
impl ObjectStoreRegistry for CometObjectStoreRegistry {
@@ -445,15 +456,35 @@ impl ObjectStoreRegistry for CometObjectStoreRegistry {
445456
if is_azure_scheme(url.scheme()) {
446457
self.azure_stores
447458
.write()
459+
.unwrap_or_else(PoisonError::into_inner)
448460
.insert(object_store_url_key(url), store)
449461
} else {
450462
self.default.register_store(url, store)
451463
}
452464
}
453465

466+
fn deregister_store(&self, url: &Url) -> DataFusionResult<Arc<dyn ObjectStore>> {
467+
if is_azure_scheme(url.scheme()) {
468+
self.azure_stores
469+
.write()
470+
.unwrap_or_else(PoisonError::into_inner)
471+
.remove(&object_store_url_key(url))
472+
.ok_or_else(|| {
473+
DataFusionError::Internal(format!("No suitable object store found for {url}"))
474+
})
475+
} else {
476+
self.default.deregister_store(url)
477+
}
478+
}
479+
454480
fn get_store(&self, url: &Url) -> DataFusionResult<Arc<dyn ObjectStore>> {
455481
if is_azure_scheme(url.scheme()) {
456-
if let Some(store) = self.azure_stores.read().get(&object_store_url_key(url)) {
482+
if let Some(store) = self
483+
.azure_stores
484+
.read()
485+
.unwrap_or_else(PoisonError::into_inner)
486+
.get(&object_store_url_key(url))
487+
{
457488
return Ok(Arc::clone(store));
458489
}
459490
}
@@ -747,28 +778,31 @@ mod tests {
747778
}
748779

749780
#[test]
750-
fn test_s3_store_cache_keys_by_host() {
781+
fn test_non_azure_schemes_ignore_userinfo() {
782+
// The URL shape here is synthetic: real S3 URLs carry the bucket in the host
783+
// position, not in userinfo. Placing distinct userinfo in front of a shared host
784+
// pins down that only Azure schemes include userinfo in the cache key.
751785
let configs = HashMap::from([
752786
(
753787
"fs.s3a.aws.credentials.provider".into(),
754788
"org.apache.hadoop.fs.s3a.AnonymousAWSCredentialsProvider".into(),
755789
),
756790
("fs.s3a.endpoint.region".into(), "us-east-1".into()),
757791
]);
758-
let object_store = |bucket| {
792+
let object_store = |userinfo| {
759793
let runtime_env = Arc::new(RuntimeEnv::default());
760794
let (object_store_url, _) = super::prepare_object_store_with_configs(
761795
Arc::clone(&runtime_env),
762-
format!("s3://{bucket}@shared-host/path/file.parquet"),
796+
format!("s3://{userinfo}@shared-host/path/file.parquet"),
763797
&configs,
764798
)
765799
.unwrap();
766800
runtime_env.object_store(&object_store_url).unwrap()
767801
};
768802

769803
assert!(Arc::ptr_eq(
770-
&object_store("bucket-a"),
771-
&object_store("bucket-b")
804+
&object_store("userinfo-a"),
805+
&object_store("userinfo-b")
772806
));
773807
}
774808
}

0 commit comments

Comments
 (0)