Re: [PR] Split out avro, parquet, json and csv into individual crates [datafusion]

["logan-keede (via GitHub)" <gi...@apache.org>]

logan-keede commented on code in PR #14951:
URL: https://github.com/apache/datafusion/pull/14951#discussion_r1976405859


##########
datafusion/core/src/execution/session_state.rs:
##########
@@ -832,17 +836,16 @@ impl SessionState {
         self.config.options()
     }
 
-    /// return the TableOptions options with its extensions
-    pub fn default_table_options(&self) -> TableOptions {
-        self.table_options
-            .combine_with_session_config(self.config_options())
-    }
-
     /// Return the table options
     pub fn table_options(&self) -> &TableOptions {
         &self.table_options
     }
 
+    /// return the TableOptions options with its extensions
+    pub fn default_table_options(&self) -> TableOptions {
+        Session::default_table_options(self)
+    }
+

Review Comment:
   I think `default_table_options` and `table_options` only need to be part of `impl Session` rather than 
   `SessionState`



##########
datafusion/datasource/src/file_scan_config.rs:
##########
@@ -501,7 +501,7 @@ impl FileScanConfig {
         (schema, constraints, stats, output_ordering)
     }
 
-    #[cfg_attr(not(feature = "avro"), allow(unused))] // Only used by avro

Review Comment:
   It is my understanding that this is being removed to avoid dependency on avro.
   I am not sure what is better between this and empty feature. 



##########
datafusion/core/src/execution/session_state.rs:
##########
@@ -832,17 +836,16 @@ impl SessionState {
         self.config.options()
     }
 
-    /// return the TableOptions options with its extensions
-    pub fn default_table_options(&self) -> TableOptions {
-        self.table_options
-            .combine_with_session_config(self.config_options())
-    }
-
     /// Return the table options
     pub fn table_options(&self) -> &TableOptions {
         &self.table_options
     }
 
+    /// return the TableOptions options with its extensions
+    pub fn default_table_options(&self) -> TableOptions {
+        Session::default_table_options(self)
+    }
+
     /// Return mutable table options
     pub fn table_options_mut(&mut self) -> &mut TableOptions {
         &mut self.table_options

Review Comment:
   perhaps this should also be a part of Session



##########
datafusion/core/Cargo.toml:
##########
@@ -54,14 +60,15 @@ default = [
     "string_expressions",
     "unicode_expressions",
     "compression",
+    "avro",

Review Comment:
   should avro be a default feature?



-- 
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@datafusion.apache.org

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


---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org