Skip to content

Commit

Permalink
Avoid unnecessary move when setting SessionConfig (#12260)
Browse files Browse the repository at this point in the history
  • Loading branch information
findepi committed Sep 2, 2024
1 parent 780cccb commit d86b7f9
Show file tree
Hide file tree
Showing 7 changed files with 17 additions and 15 deletions.
2 changes: 1 addition & 1 deletion datafusion/common/src/config.rs
Original file line number Diff line number Diff line change
Expand Up @@ -781,7 +781,7 @@ impl ConfigOptions {
///
/// Only the built-in configurations will be extracted from the hash map
/// and other key value pairs will be ignored.
pub fn from_string_hash_map(settings: HashMap<String, String>) -> Result<Self> {
pub fn from_string_hash_map(settings: &HashMap<String, String>) -> Result<Self> {
struct Visitor(Vec<String>);

impl Visit for Visitor {
Expand Down
8 changes: 5 additions & 3 deletions datafusion/core/src/dataframe/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3277,7 +3277,7 @@ mod tests {
#[tokio::test]
async fn with_column_renamed_case_sensitive() -> Result<()> {
let config =
SessionConfig::from_string_hash_map(std::collections::HashMap::from([(
SessionConfig::from_string_hash_map(&std::collections::HashMap::from([(
"datafusion.sql_parser.enable_ident_normalization".to_owned(),
"false".to_owned(),
)]))?;
Expand Down Expand Up @@ -3713,8 +3713,10 @@ mod tests {
// Test issue: https://github.com/apache/datafusion/issues/12065
#[tokio::test]
async fn filtered_aggr_with_param_values() -> Result<()> {
let cfg = SessionConfig::new()
.set("datafusion.sql_parser.dialect", "PostgreSQL".into());
let cfg = SessionConfig::new().set(
"datafusion.sql_parser.dialect",
&ScalarValue::from("PostgreSQL"),
);
let ctx = SessionContext::new_with_config(cfg);
register_aggregate_csv(&ctx, "table1").await?;

Expand Down
2 changes: 1 addition & 1 deletion datafusion/core/src/dataframe/parquet.rs
Original file line number Diff line number Diff line change
Expand Up @@ -190,7 +190,7 @@ mod tests {
// This test verifies writing a parquet file with small rg size
// relative to datafusion.execution.batch_size does not panic
let ctx = SessionContext::new_with_config(SessionConfig::from_string_hash_map(
HashMap::from_iter(
&HashMap::from_iter(
[("datafusion.execution.batch_size", "10")]
.iter()
.map(|(s1, s2)| (s1.to_string(), s2.to_string())),
Expand Down
4 changes: 2 additions & 2 deletions datafusion/core/src/datasource/listing/table.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1781,7 +1781,7 @@ mod tests {
// Create the initial context, schema, and batch.
let session_ctx = match session_config_map {
Some(cfg) => {
let config = SessionConfig::from_string_hash_map(cfg)?;
let config = SessionConfig::from_string_hash_map(&cfg)?;
SessionContext::new_with_config(config)
}
None => SessionContext::new(),
Expand Down Expand Up @@ -1979,7 +1979,7 @@ mod tests {
// Create the initial context
let session_ctx = match session_config_map {
Some(cfg) => {
let config = SessionConfig::from_string_hash_map(cfg)?;
let config = SessionConfig::from_string_hash_map(&cfg)?;
SessionContext::new_with_config(config)
}
None => SessionContext::new(),
Expand Down
2 changes: 1 addition & 1 deletion datafusion/core/src/execution/context/parquet.rs
Original file line number Diff line number Diff line change
Expand Up @@ -106,7 +106,7 @@ mod tests {
#[tokio::test]
async fn read_with_glob_path_issue_2465() -> Result<()> {
let config =
SessionConfig::from_string_hash_map(std::collections::HashMap::from([(
SessionConfig::from_string_hash_map(&std::collections::HashMap::from([(
"datafusion.execution.listing_table_ignore_subdirectory".to_owned(),
"false".to_owned(),
)]))?;
Expand Down
6 changes: 3 additions & 3 deletions datafusion/execution/src/config.rs
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,7 @@ use datafusion_common::{
/// use datafusion_common::ScalarValue;
///
/// let config = SessionConfig::new()
/// .set("datafusion.execution.batch_size", ScalarValue::UInt64(Some(1234)))
/// .set("datafusion.execution.batch_size", &ScalarValue::UInt64(Some(1234)))
/// .set_bool("datafusion.execution.parquet.pushdown_filters", true);
///
/// assert_eq!(config.batch_size(), 1234);
Expand Down Expand Up @@ -123,7 +123,7 @@ impl SessionConfig {
}

/// Create new ConfigOptions struct, taking values from a string hash map.
pub fn from_string_hash_map(settings: HashMap<String, String>) -> Result<Self> {
pub fn from_string_hash_map(settings: &HashMap<String, String>) -> Result<Self> {
Ok(ConfigOptions::from_string_hash_map(settings)?.into())
}

Expand Down Expand Up @@ -157,7 +157,7 @@ impl SessionConfig {
}

/// Set a configuration option
pub fn set(self, key: &str, value: ScalarValue) -> Self {
pub fn set(self, key: &str, value: &ScalarValue) -> Self {
self.set_str(key, &value.to_string())
}

Expand Down
8 changes: 4 additions & 4 deletions datafusion/physical-plan/src/aggregates/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2373,11 +2373,11 @@ mod tests {
let mut session_config = SessionConfig::default();
session_config = session_config.set(
"datafusion.execution.skip_partial_aggregation_probe_rows_threshold",
ScalarValue::Int64(Some(2)),
&ScalarValue::Int64(Some(2)),
);
session_config = session_config.set(
"datafusion.execution.skip_partial_aggregation_probe_ratio_threshold",
ScalarValue::Float64(Some(0.1)),
&ScalarValue::Float64(Some(0.1)),
);

let ctx = TaskContext::default().with_session_config(session_config);
Expand Down Expand Up @@ -2462,11 +2462,11 @@ mod tests {
let mut session_config = SessionConfig::default();
session_config = session_config.set(
"datafusion.execution.skip_partial_aggregation_probe_rows_threshold",
ScalarValue::Int64(Some(5)),
&ScalarValue::Int64(Some(5)),
);
session_config = session_config.set(
"datafusion.execution.skip_partial_aggregation_probe_ratio_threshold",
ScalarValue::Float64(Some(0.1)),
&ScalarValue::Float64(Some(0.1)),
);

let ctx = TaskContext::default().with_session_config(session_config);
Expand Down

0 comments on commit d86b7f9

Please sign in to comment.