[GitHub] [arrow-datafusion] alamb commented on a diff in pull request #4243: Use defaults for ListingOptions builder

[GitBox <gi...@apache.org>]

alamb commented on code in PR #4243:
URL: https://github.com/apache/arrow-datafusion/pull/4243#discussion_r1024264617


##########
datafusion/core/tests/sql/parquet.rs:
##########
@@ -72,10 +72,10 @@ async fn parquet_with_sort_order_specified() {
         })
         .collect::<Vec<_>>();
 
-    let options_sort = ListingOptions {
-        file_sort_order: Some(file_sort_order),
-        ..parquet_read_options.to_listing_options(target_partitions)
-    };
+    let options_sort = ListingOptions::from_options(

Review Comment:
   same comment as above



##########
datafusion/core/src/datasource/listing/table.rs:
##########
@@ -773,10 +788,9 @@ mod tests {
         ];
 
         for (file_sort_order, expected_result) in cases {
-            let options = ListingOptions {
-                file_sort_order,
-                ..options.clone()
-            };
+            let options = ListingOptions::from_options(options.clone())
+                .with_file_sort_order(file_sort_order);

Review Comment:
   Would this work? I think we can avoid adding `from_options`
   
   ```suggestion
               let options = options.clone()
                   .with_file_sort_order(file_sort_order);
   ```



##########
datafusion/core/tests/sql/parquet.rs:
##########
@@ -57,10 +57,10 @@ async fn parquet_with_sort_order_specified() {
     let target_partitions = 2;
 
     // The sort order is not specified
-    let options_no_sort = ListingOptions {
-        file_sort_order: None,
-        ..parquet_read_options.to_listing_options(target_partitions)
-    };
+    let options_no_sort = ListingOptions::from_options(
+        parquet_read_options.to_listing_options(target_partitions),
+    )
+    .with_file_sort_order(None);

Review Comment:
   ```suggestion
       let options_no_sort = parquet_read_options
         .to_listing_options(target_partitions)
         .with_file_sort_order(None);
   ```
   
   



##########
datafusion/core/src/datasource/listing/table.rs:
##########
@@ -254,6 +249,26 @@ impl ListingOptions {
             file_sort_order: None,
         }
     }
+
+    /// Build ListingOptions from an existing set of ListingOptions

Review Comment:
   I think typically in rust one uses `options.clone()` for this operation
   
   I will leave a comment at the usage site



-- 
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: github-unsubscribe@arrow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org