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


##########
.github/workflows/rust.yml:
##########
@@ -98,7 +98,7 @@ jobs:
         with:
           rust-version: stable
       - name: Run tests (excluding doctests)
-        run: cargo test --lib --tests --bins --features avro,json,backtrace
+        run: RUST_MIN_STACK=504857600 cargo test --lib --tests --bins 
--features avro,json,backtrace

Review Comment:
   Why is the new stack size needed?



##########
datafusion/common/src/file_options/file_type.rs:
##########
@@ -41,7 +43,8 @@ pub trait GetExt {
 }
 
 /// Readable file type
-#[derive(Debug, Clone, PartialEq, Eq, Hash)]
+#[allow(clippy::derived_hash_with_manual_eq)]

Review Comment:
   You could probably also manually implement `Hash` pretty easily as well, but 
I agree it isn't a bad thing



##########
datafusion/proto/src/physical_plan/to_proto.rs:
##########
@@ -891,6 +891,9 @@ impl TryFrom<&FileTypeWriterOptions> for 
protobuf::FileTypeWriterOptions {
             FileTypeWriterOptions::Arrow(ArrowWriterOptions {}) => {
                 return not_impl_err!("Arrow file sink protobuf serialization")
             }
+            FileTypeWriterOptions::Extension(_) => {

Review Comment:
   We could also punt it into the `FileType::Extension` (e.g. add a 
`serialize(&self) -> Result<Vec<u8>>` type method)



##########
datafusion/core/src/datasource/listing/table.rs:
##########
@@ -1716,6 +1719,9 @@ mod tests {
                     )
                     .await?;
             }
+            FileType::Extension(_) => {
+                panic!("Extension file type not implemented in write path.")

Review Comment:
   Can we instead return an NotImplement exception?



##########
datafusion/core/src/physical_planner.rs:
##########
@@ -605,6 +605,7 @@ impl DefaultPhysicalPlanner {
                         FileType::JSON => Arc::new(JsonFormat::default()),
                         FileType::AVRO => Arc::new(AvroFormat {} ),
                         FileType::ARROW => Arc::new(ArrowFormat {}),
+                        FileType::Extension(_ext) => return 
not_impl_err!("Extension FileTypes not supported in Copy To statements.")

Review Comment:
   Maybe we could have the `FileTypeExtension` have a method that returned 
`Arc<dyn FileFormat>` ?



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