alamb commented on code in PR #4492:
URL: https://github.com/apache/arrow-datafusion/pull/4492#discussion_r1038770105
##########
benchmarks/src/bin/tpch.rs:
##########
@@ -410,7 +410,7 @@ async fn get_table(
let options = ListingOptions::new(format)
.with_file_extension(extension)
.with_target_partitions(target_partitions)
- .with_collect_stat(ctx.config.collect_statistics);
+ .with_collect_stat(ctx.config.collect_statistics());
Review Comment:
this illustrates the major API change for DataFusion users
##########
datafusion/core/src/execution/context.rs:
##########
@@ -1328,27 +1381,27 @@ impl SessionConfig {
}
map.insert(
TARGET_PARTITIONS.to_owned(),
- format!("{}", self.target_partitions),
+ format!("{}", self.target_partitions()),
Review Comment:
this whole function should likely be deprecated and instead we should use
ConfigOptions directly. However, for this PR I opted to leave it alone (and
thus backwards compatible for Ballista) cc @mingmwang
##########
datafusion/core/src/execution/context.rs:
##########
@@ -1140,28 +1144,11 @@ impl Hasher for IdHasher {
/// Configuration options for session context
#[derive(Clone)]
pub struct SessionConfig {
- /// Number of partitions for query execution. Increasing partitions can
increase concurrency.
- pub target_partitions: usize,
/// Default catalog name for table resolution
default_catalog: String,
- /// Default schema name for table resolution
+ /// Default schema name for table resolution (not in ConfigOptions
+ /// due to `resolve_table_ref` which passes back references)
default_schema: String,
- /// Whether the default catalog and schema should be created automatically
Review Comment:
the change in this PR is to remove these fields and move them to
`ConfigOptions`
--
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]
