2010YOUY01 commented on code in PR #18682:
URL: https://github.com/apache/datafusion/pull/18682#discussion_r2525593393
##########
datafusion/datasource/src/memory.rs:
##########
@@ -283,8 +283,8 @@ impl MemorySourceConfig {
/// Create a new execution plan from a list of constant values
(`ValuesExec`)
pub fn try_new_as_values(
Review Comment:
ditto
##########
datafusion/datasource/src/file_scan_config.rs:
##########
@@ -1147,7 +1148,7 @@ impl PartitionColumnProjector {
// - `partition_values`: the list of partition values, one for each
partition column
pub fn project(
&mut self,
- file_batch: RecordBatch,
+ file_batch: &RecordBatch,
Review Comment:
I suggest to revert the change and suppress it with
`#[expect(clippy::needless_pass_by_value)]`, due to
1. This is a public API, it's better to avoid changing it so that the
downstream datafusion dependents don't have to update it during ugprades
2. Cloning `RecordBatch` is a shallow clone for inner heavy payloads.
##########
datafusion/datasource/src/statistics.rs:
##########
@@ -152,33 +152,29 @@ impl MinMaxStatistics {
.into_iter()
.unzip();
- Self::new(
- &min_max_sort_order,
- &min_max_schema,
- RecordBatch::try_new(Arc::clone(&min_max_schema),
min_values).map_err(
- |e| {
- DataFusionError::ArrowError(
- Box::new(e),
- Some("\ncreate min batch".to_string()),
- )
- },
- )?,
- RecordBatch::try_new(Arc::clone(&min_max_schema),
max_values).map_err(
- |e| {
- DataFusionError::ArrowError(
- Box::new(e),
- Some("\ncreate max batch".to_string()),
- )
- },
- )?,
- )
+ let min_batch = RecordBatch::try_new(Arc::clone(&min_max_schema),
min_values)
+ .map_err(|e| {
+ DataFusionError::ArrowError(
+ Box::new(e),
+ Some("\ncreate min batch".to_string()),
+ )
+ })?;
+ let max_batch = RecordBatch::try_new(Arc::clone(&min_max_schema),
max_values)
+ .map_err(|e| {
+ DataFusionError::ArrowError(
+ Box::new(e),
+ Some("\ncreate max batch".to_string()),
+ )
+ })?;
+
+ Self::new(&min_max_sort_order, &min_max_schema, &min_batch, &max_batch)
}
pub fn new(
Review Comment:
ditto
##########
datafusion/datasource/src/statistics.rs:
##########
@@ -458,18 +454,14 @@ pub fn compute_file_group_statistics(
/// * The summary statistics across all file groups, aka all files summary
statistics
pub fn compute_all_files_statistics(
Review Comment:
ditto
##########
datafusion/datasource/src/memory.rs:
##########
@@ -326,21 +326,21 @@ impl MemorySourceConfig {
.collect::<Result<Vec<_>>>()?;
let batch = RecordBatch::try_new_with_options(
- Arc::clone(&schema),
+ Arc::clone(schema),
arrays,
&RecordBatchOptions::new().with_row_count(Some(n_row)),
)?;
let partitions = vec![batch];
- Self::try_new_from_batches(Arc::clone(&schema), partitions)
+ Self::try_new_from_batches(schema, partitions)
}
/// Create a new plan using the provided schema and batches.
///
/// Errors if any of the batches don't match the provided schema, or if no
/// batches are provided.
pub fn try_new_from_batches(
- schema: SchemaRef,
+ schema: &SchemaRef,
Review Comment:
ditto
--
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]
