Skip to content
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

use Into<String> as argument type wherever applicable #615

Merged
merged 3 commits into from
Jun 28, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 4 additions & 3 deletions datafusion/src/datasource/csv.rs
Original file line number Diff line number Diff line change
Expand Up @@ -59,11 +59,12 @@ pub struct CsvFile {

impl CsvFile {
/// Attempt to initialize a new `CsvFile` from a file path
pub fn try_new(path: &str, options: CsvReadOptions) -> Result<Self> {
pub fn try_new(path: impl Into<String>, options: CsvReadOptions) -> Result<Self> {
let path = path.into();
let schema = Arc::new(match options.schema {
Some(s) => s.clone(),
None => {
let filenames = common::build_file_list(path, options.file_extension)?;
let filenames = common::build_file_list(&path, options.file_extension)?;
if filenames.is_empty() {
return Err(DataFusionError::Plan(format!(
"No files found at {path} with file extension {file_extension}",
Expand All @@ -76,7 +77,7 @@ impl CsvFile {
});

Ok(Self {
source: Source::Path(path.to_string()),
source: Source::Path(path),
schema,
has_header: options.has_header,
delimiter: options.delimiter,
Expand Down
7 changes: 4 additions & 3 deletions datafusion/src/datasource/parquet.rs
Original file line number Diff line number Diff line change
Expand Up @@ -42,11 +42,12 @@ pub struct ParquetTable {

impl ParquetTable {
/// Attempt to initialize a new `ParquetTable` from a file path.
pub fn try_new(path: &str, max_concurrency: usize) -> Result<Self> {
let parquet_exec = ParquetExec::try_from_path(path, None, None, 0, 1, None)?;
pub fn try_new(path: impl Into<String>, max_concurrency: usize) -> Result<Self> {
let path = path.into();
let parquet_exec = ParquetExec::try_from_path(&path, None, None, 0, 1, None)?;
let schema = parquet_exec.schema();
Ok(Self {
path: path.to_string(),
path,
schema,
statistics: parquet_exec.statistics().to_owned(),
max_concurrency,
Expand Down
17 changes: 11 additions & 6 deletions datafusion/src/execution/context.rs
Original file line number Diff line number Diff line change
Expand Up @@ -270,7 +270,7 @@ impl ExecutionContext {
/// Creates a DataFrame for reading a CSV data source.
pub fn read_csv(
&mut self,
filename: &str,
filename: impl Into<String>,
options: CsvReadOptions,
) -> Result<Arc<dyn DataFrame>> {
Ok(Arc::new(DataFrameImpl::new(
Expand All @@ -280,7 +280,10 @@ impl ExecutionContext {
}

/// Creates a DataFrame for reading a Parquet data source.
pub fn read_parquet(&mut self, filename: &str) -> Result<Arc<dyn DataFrame>> {
pub fn read_parquet(
&mut self,
filename: impl Into<String>,
) -> Result<Arc<dyn DataFrame>> {
Ok(Arc::new(DataFrameImpl::new(
self.state.clone(),
&LogicalPlanBuilder::scan_parquet(
Expand Down Expand Up @@ -474,10 +477,11 @@ impl ExecutionContext {
pub async fn write_csv(
&self,
plan: Arc<dyn ExecutionPlan>,
path: String,
path: impl AsRef<str>,
) -> Result<()> {
let path = path.as_ref();
// create directory to contain the CSV files (one per partition)
let fs_path = Path::new(&path);
let fs_path = Path::new(path);
match fs::create_dir(fs_path) {
Ok(()) => {
let mut tasks = vec![];
Expand Down Expand Up @@ -511,11 +515,12 @@ impl ExecutionContext {
pub async fn write_parquet(
&self,
plan: Arc<dyn ExecutionPlan>,
path: String,
path: impl AsRef<str>,
writer_properties: Option<WriterProperties>,
) -> Result<()> {
let path = path.as_ref();
// create directory to contain the Parquet files (one per partition)
let fs_path = Path::new(&path);
let fs_path = Path::new(path);
match fs::create_dir(fs_path) {
Ok(()) => {
let mut tasks = vec![];
Expand Down
28 changes: 16 additions & 12 deletions datafusion/src/logical_plan/builder.rs
Original file line number Diff line number Diff line change
Expand Up @@ -115,39 +115,41 @@ impl LogicalPlanBuilder {

/// Scan a CSV data source
pub fn scan_csv(
path: &str,
path: impl Into<String>,
options: CsvReadOptions,
projection: Option<Vec<usize>>,
) -> Result<Self> {
Self::scan_csv_with_name(path, options, projection, path)
let path = path.into();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As above, this pattern just doesn't make a lot of sense to me -- if someone passed in a &str then this code will now create a copy (a String) and then simply use a reference to that copy (rather than the path itself)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the owned value is passed along to Self::scan at the end of the function.

Self::scan_csv_with_name(path.clone(), options, projection, path)
}

/// Scan a CSV data source and register it with a given table name
pub fn scan_csv_with_name(
path: &str,
path: impl Into<String>,
options: CsvReadOptions,
projection: Option<Vec<usize>>,
table_name: &str,
table_name: impl Into<String>,
) -> Result<Self> {
let provider = Arc::new(CsvFile::try_new(path, options)?);
Self::scan(table_name, provider, projection)
}

/// Scan a Parquet data source
pub fn scan_parquet(
path: &str,
path: impl Into<String>,
projection: Option<Vec<usize>>,
max_concurrency: usize,
) -> Result<Self> {
Self::scan_parquet_with_name(path, projection, max_concurrency, path)
let path = path.into();
Self::scan_parquet_with_name(path.clone(), projection, max_concurrency, path)
}

/// Scan a Parquet data source and register it with a given table name
pub fn scan_parquet_with_name(
path: &str,
path: impl Into<String>,
projection: Option<Vec<usize>>,
max_concurrency: usize,
table_name: &str,
table_name: impl Into<String>,
) -> Result<Self> {
let provider = Arc::new(ParquetTable::try_new(path, max_concurrency)?);
Self::scan(table_name, provider, projection)
Expand All @@ -166,10 +168,12 @@ impl LogicalPlanBuilder {

/// Convert a table provider into a builder with a TableScan
pub fn scan(
table_name: &str,
table_name: impl Into<String>,
provider: Arc<dyn TableProvider>,
projection: Option<Vec<usize>>,
) -> Result<Self> {
let table_name = table_name.into();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change makes sense to me as this function previously was making an explicit copy and no can reuse the copy the caller passed in, if any 👍

            table_name: table_name.to_string(),


if table_name.is_empty() {
return Err(DataFusionError::Plan(
"table_name cannot be empty".to_string(),
Expand All @@ -184,17 +188,17 @@ impl LogicalPlanBuilder {
DFSchema::new(
p.iter()
.map(|i| {
DFField::from_qualified(table_name, schema.field(*i).clone())
DFField::from_qualified(&table_name, schema.field(*i).clone())
})
.collect(),
)
})
.unwrap_or_else(|| {
DFSchema::try_from_qualified_schema(table_name, &schema)
DFSchema::try_from_qualified_schema(&table_name, &schema)
})?;

let table_scan = LogicalPlan::TableScan {
table_name: table_name.to_string(),
table_name,
source: provider,
projected_schema: Arc::new(projected_schema),
projection,
Expand Down
6 changes: 3 additions & 3 deletions datafusion/src/logical_plan/expr.rs
Original file line number Diff line number Diff line change
Expand Up @@ -44,10 +44,10 @@ pub struct Column {

impl Column {
/// Create Column from unqualified name.
pub fn from_name(name: String) -> Self {
pub fn from_name(name: impl Into<String>) -> Self {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

Self {
relation: None,
name,
name: name.into(),
}
}

Expand Down Expand Up @@ -131,7 +131,7 @@ impl fmt::Display for Column {
/// ```
/// # use datafusion::logical_plan::*;
/// let expr = col("c1");
/// assert_eq!(expr, Expr::Column(Column::from_name("c1".to_string())));
/// assert_eq!(expr, Expr::Column(Column::from_name("c1")));
/// ```
///
/// ## Create the expression `c1 + c2` to add columns "c1" and "c2" together
Expand Down
12 changes: 6 additions & 6 deletions datafusion/src/optimizer/filter_push_down.rs
Original file line number Diff line number Diff line change
Expand Up @@ -890,8 +890,8 @@ mod tests {
.join(
&right,
JoinType::Inner,
vec![Column::from_name("a".to_string())],
vec![Column::from_name("a".to_string())],
vec![Column::from_name("a")],
vec![Column::from_name("a")],
)?
.filter(col("a").lt_eq(lit(1i64)))?
.build()?;
Expand Down Expand Up @@ -933,8 +933,8 @@ mod tests {
.join(
&right,
JoinType::Inner,
vec![Column::from_name("a".to_string())],
vec![Column::from_name("a".to_string())],
vec![Column::from_name("a")],
vec![Column::from_name("a")],
)?
// "b" and "c" are not shared by either side: they are only available together after the join
.filter(col("c").lt_eq(col("b")))?
Expand Down Expand Up @@ -972,8 +972,8 @@ mod tests {
.join(
&right,
JoinType::Inner,
vec![Column::from_name("a".to_string())],
vec![Column::from_name("a".to_string())],
vec![Column::from_name("a")],
vec![Column::from_name("a")],
)?
.filter(col("b").lt_eq(lit(1i64)))?
.build()?;
Expand Down
4 changes: 2 additions & 2 deletions datafusion/src/optimizer/projection_push_down.rs
Original file line number Diff line number Diff line change
Expand Up @@ -241,7 +241,7 @@ fn optimize_plan(
{
window_expr.iter().try_for_each(|expr| {
let name = &expr.name(schema)?;
let column = Column::from_name(name.to_string());
let column = Column::from_name(name);
if required_columns.contains(&column) {
new_window_expr.push(expr.clone());
new_required_columns.insert(column);
Expand Down Expand Up @@ -286,7 +286,7 @@ fn optimize_plan(
let mut new_aggr_expr = Vec::new();
aggr_expr.iter().try_for_each(|expr| {
let name = &expr.name(schema)?;
let column = Column::from_name(name.to_string());
let column = Column::from_name(name);

if required_columns.contains(&column) {
new_aggr_expr.push(expr.clone());
Expand Down
2 changes: 1 addition & 1 deletion datafusion/src/optimizer/utils.rs
Original file line number Diff line number Diff line change
Expand Up @@ -516,7 +516,7 @@ mod tests {
&mut accum,
)?;
assert_eq!(1, accum.len());
assert!(accum.contains(&Column::from_name("a".to_string())));
assert!(accum.contains(&Column::from_name("a")));
Ok(())
}

Expand Down
4 changes: 2 additions & 2 deletions datafusion/src/physical_plan/aggregates.rs
Original file line number Diff line number Diff line change
Expand Up @@ -110,9 +110,9 @@ pub fn create_aggregate_expr(
distinct: bool,
args: &[Arc<dyn PhysicalExpr>],
input_schema: &Schema,
name: String,
name: impl Into<String>,
) -> Result<Arc<dyn AggregateExpr>> {
// coerce
let name = name.into();
let arg = coerce(args, input_schema, &signature(fun))?;
if arg.is_empty() {
return Err(DataFusionError::Plan(format!(
Expand Down
8 changes: 6 additions & 2 deletions datafusion/src/physical_plan/expressions/average.rs
Original file line number Diff line number Diff line change
Expand Up @@ -64,9 +64,13 @@ pub fn avg_return_type(arg_type: &DataType) -> Result<DataType> {

impl Avg {
/// Create a new AVG aggregate function
pub fn new(expr: Arc<dyn PhysicalExpr>, name: String, data_type: DataType) -> Self {
pub fn new(
expr: Arc<dyn PhysicalExpr>,
name: impl Into<String>,
data_type: DataType,
) -> Self {
Self {
name,
name: name.into(),
expr,
data_type,
nullable: true,
Expand Down
8 changes: 6 additions & 2 deletions datafusion/src/physical_plan/expressions/count.rs
Original file line number Diff line number Diff line change
Expand Up @@ -44,9 +44,13 @@ pub struct Count {

impl Count {
/// Create a new COUNT aggregate function.
pub fn new(expr: Arc<dyn PhysicalExpr>, name: String, data_type: DataType) -> Self {
pub fn new(
expr: Arc<dyn PhysicalExpr>,
name: impl Into<String>,
data_type: DataType,
) -> Self {
Self {
name,
name: name.into(),
expr,
data_type,
nullable: true,
Expand Down
16 changes: 12 additions & 4 deletions datafusion/src/physical_plan/expressions/min_max.rs
Original file line number Diff line number Diff line change
Expand Up @@ -49,9 +49,13 @@ pub struct Max {

impl Max {
/// Create a new MAX aggregate function
pub fn new(expr: Arc<dyn PhysicalExpr>, name: String, data_type: DataType) -> Self {
pub fn new(
expr: Arc<dyn PhysicalExpr>,
name: impl Into<String>,
data_type: DataType,
) -> Self {
Self {
name,
name: name.into(),
expr,
data_type,
nullable: true,
Expand Down Expand Up @@ -352,9 +356,13 @@ pub struct Min {

impl Min {
/// Create a new MIN aggregate function
pub fn new(expr: Arc<dyn PhysicalExpr>, name: String, data_type: DataType) -> Self {
pub fn new(
expr: Arc<dyn PhysicalExpr>,
name: impl Into<String>,
data_type: DataType,
) -> Self {
Self {
name,
name: name.into(),
expr,
data_type,
nullable: true,
Expand Down
12 changes: 6 additions & 6 deletions datafusion/src/physical_plan/expressions/nth_value.rs
Original file line number Diff line number Diff line change
Expand Up @@ -45,12 +45,12 @@ pub struct NthValue {
impl NthValue {
/// Create a new FIRST_VALUE window aggregate function
pub fn first_value(
name: String,
name: impl Into<String>,
expr: Arc<dyn PhysicalExpr>,
data_type: DataType,
) -> Self {
Self {
name,
name: name.into(),
expr,
data_type,
kind: NthValueKind::First,
Expand All @@ -59,12 +59,12 @@ impl NthValue {

/// Create a new LAST_VALUE window aggregate function
pub fn last_value(
name: String,
name: impl Into<String>,
expr: Arc<dyn PhysicalExpr>,
data_type: DataType,
) -> Self {
Self {
name,
name: name.into(),
expr,
data_type,
kind: NthValueKind::Last,
Expand All @@ -73,7 +73,7 @@ impl NthValue {

/// Create a new NTH_VALUE window aggregate function
pub fn nth_value(
name: String,
name: impl Into<String>,
expr: Arc<dyn PhysicalExpr>,
data_type: DataType,
n: u32,
Expand All @@ -83,7 +83,7 @@ impl NthValue {
"nth_value expect n to be > 0".to_owned(),
)),
_ => Ok(Self {
name,
name: name.into(),
expr,
data_type,
kind: NthValueKind::Nth(n),
Expand Down
4 changes: 2 additions & 2 deletions datafusion/src/physical_plan/expressions/row_number.rs
Original file line number Diff line number Diff line change
Expand Up @@ -32,8 +32,8 @@ pub struct RowNumber {

impl RowNumber {
/// Create a new ROW_NUMBER function
pub fn new(name: String) -> Self {
Self { name }
pub fn new(name: impl Into<String>) -> Self {
Self { name: name.into() }
}
}

Expand Down
Loading