alamb commented on code in PR #12573:
URL: https://github.com/apache/datafusion/pull/12573#discussion_r1769545347
##########
datafusion/core/src/execution/context/mod.rs:
##########
@@ -375,21 +382,60 @@ impl SessionContext {
/// # Ok(())
/// # }
/// ```
- pub fn enable_url_table(&self) -> Self {
- let state_ref = self.state();
+ pub fn enable_url_table(self) -> Self {
+ let current_catalog_list =
Arc::clone(self.state.read().catalog_list());
Review Comment:
this avoids copying the entire state just to copy the catalog list
##########
datafusion-examples/examples/external_dependency/query-aws-s3.rs:
##########
@@ -64,7 +64,7 @@ async fn main() -> Result<()> {
df.show().await?;
// dynamic query by the file path
- ctx.enable_url_table();
+ let ctx = ctx.enable_url_table();
Review Comment:
this example actually has the exact same bug I hit in
https://github.com/apache/datafusion/issues/12551 -- namely the *returned* ctx
has the table enabled, not the current one.
##########
datafusion/core/src/execution/context/mod.rs:
##########
@@ -375,21 +382,60 @@ impl SessionContext {
/// # Ok(())
/// # }
/// ```
- pub fn enable_url_table(&self) -> Self {
- let state_ref = self.state();
+ pub fn enable_url_table(self) -> Self {
+ let current_catalog_list =
Arc::clone(self.state.read().catalog_list());
let factory =
Arc::new(DynamicListTableFactory::new(SessionStore::new()));
let catalog_list = Arc::new(DynamicFileCatalog::new(
- Arc::clone(state_ref.catalog_list()),
+ current_catalog_list,
Arc::clone(&factory) as Arc<dyn UrlTableFactory>,
));
- let new_state = SessionStateBuilder::new_from_existing(self.state())
+ let ctx: SessionContext = self
Review Comment:
This is likely just my obsession with trying to have clear Builder style
APIs, but I think it reads more clearly here to to back and forth between the
types
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
