yjshen commented on a change in pull request #1562:
URL: https://github.com/apache/arrow-datafusion/pull/1562#discussion_r784709208



##########
File path: ballista/rust/core/src/serde/logical_plan/from_proto.rs
##########
@@ -246,8 +246,8 @@ impl TryInto<LogicalPlan> for &protobuf::LogicalPlanNode {
                             .collect::<Result<Vec<_>, _>>()?,
                         partition_count as usize,
                     ),
-                    PartitionMethod::RoundRobin(batch_size) => {
-                        Partitioning::RoundRobinBatch(batch_size as usize)
+                    PartitionMethod::RoundRobin(partition_count) => {

Review comment:
       `batch_size` here is wrong, it's partition count 

##########
File path: ballista/rust/core/src/serde/logical_plan/mod.rs
##########
@@ -92,8 +92,8 @@ mod roundtrip_tests {
             .map_err(BallistaError::DataFusionError)?,
         );
 
-        for batch_size in test_batch_sizes.iter() {
-            let rr_repartition = Partitioning::RoundRobinBatch(*batch_size);
+        for partition_count in test_partition_counts.iter() {

Review comment:
       same here

##########
File path: datafusion/src/datasource/file_format/csv.rs
##########
@@ -178,7 +178,7 @@ mod tests {
     async fn read_limit() -> Result<()> {
         let runtime = Arc::new(RuntimeEnv::default());
         let projection = Some(vec![0, 1, 2, 3]);
-        let exec = get_exec("aggregate_test_100.csv", &projection, 1024, 
Some(1)).await?;
+        let exec = get_exec("aggregate_test_100.csv", &projection, 
Some(1)).await?;

Review comment:
       After this PR, most of the magic number `1024` is removed from the 
project.

##########
File path: datafusion/src/execution/context.rs
##########
@@ -901,14 +898,13 @@ pub struct ExecutionConfig {
     /// Should Datafusion parquet reader using the predicate to prune data
     parquet_pruning: bool,
     /// Runtime configurations such as memory threshold and local disk for 
spill
-    pub runtime_config: RuntimeConfig,
+    pub runtime: RuntimeConfig,

Review comment:
       Renamed to `runtime` since I think the word config is useless. Consider 
the usage `exec_config.runtime_config.batch_size` for example. Everything in 
`exec_config` is config.




-- 
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]


Reply via email to