Set Formatted TableOptions Enum #16166

mertak-synnada · 2025-05-23T12:26:00Z

Which issue does this PR close?

In the current implementation, the TableOptions struct stores information for CSV, JSON, and Parquet file types, and its behavior changes depending on whether the file format is set before or after. This PR introduces a new struct for file-type-specific cases, while keeping TableOptions as the default state, since it is also used in the session before a file type is specified.

Rationale for this change

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

separate FileSpecificTableOptions as enum

…hore/table-options-enum

# Conflicts: # datafusion/core/src/datasource/file_format/arrow.rs # datafusion/datasource/src/file_format.rs

berkaysynnada

I've worked with @mertak-synnada closely here, and the final state makes sense to me. If there are any points that we miss why TableOptions is used such, please let me know. Otherwise, I'll wait for a few days and merge this PR.

alamb

Thank you for this contribution @berkaysynnada and @mertak-synnada

I am a little confused about the new structure and exactly what problem is being solved with this change.

The PR description says

In the current implementation, the TableOptions struct stores information for CSV, JSON, and Parquet file types, and its behavior changes depending on whether the file format is set before or after.

However, I didn't see any tests or examples that show a different behavior after this PR.

In my initial quick read through it I would say it has made the APIs harder to use (as now there are format specific options on TableOptions AND a new TableFormatOptions). Is there any way to consolidate the number of structures? If not, I think we should clearly document the difference between the two structures

alamb · 2025-05-28T18:04:56Z

datafusion/common/src/config.rs

-    /// Initializes a new `TableOptions` from a hash map of string settings.
+#[derive(Debug, Clone)]
+#[allow(clippy::large_enum_variant)]
+pub enum TableFormatOptions {


Since this is a new pub struct, can we please add documentation explaining

what it is used for

How it is different than TableOptions?

I will resolve these if I can relieve your concerns :)

alamb · 2025-05-28T18:06:10Z

datafusion/datasource/src/file_format.rs

+        Ok(self.default_from_output_format(options))
+    }
+
+    fn default_from_output_format(&self, options: OutputFormat) -> Arc<dyn FileFormat>;


Can we please document these new public APIs?

alamb · 2025-05-28T18:07:58Z

datafusion/datasource/src/file_format.rs

@@ -120,7 +121,26 @@ pub trait FileFormatFactory: Sync + Send + GetExt + fmt::Debug {
        &self,
        state: &dyn Session,
        format_options: &HashMap<String, String>,
-    ) -> Result<Arc<dyn FileFormat>>;
+    ) -> Result<Arc<dyn FileFormat>> {
+        let options = match self.options() {


since this method is part of a trait, I think it can be overridden by other implementations. Is that the intention?

In other words, do you expect anyone implementing FileFormatFactory to implement create() or should they now implement default_from_output_format and options?

They should implement default_from_output_format and options. This change is not directly related to the actual changes of the PR, but all create() methods show a similar pattern, so I remember we thought like why not avoid duplication

alamb · 2025-05-28T18:08:41Z

datafusion/datasource/src/file_format.rs

+
+    fn default_from_output_format(&self, options: OutputFormat) -> Arc<dyn FileFormat>;
+
+    fn options(&self) -> (Option<OutputFormat>, ConfigFileType);


I find it strange that a FileFormatFactory returns an Option<OutputFormat> and then another method takes the output format as input

Yeah, this confuses me now too. Having ConfigFileType and OutputFormat at the same time is also a bit weird. I'll take a second look at this part

alamb · 2025-05-28T18:16:07Z

datafusion/common/src/config.rs

+        }
+    }
+
+    /// Sets a configuration value for a specific key within `TableOptions`.


this is in TableFormatOptions I think

alamb · 2025-05-28T18:17:15Z

datafusion/common/src/config.rs

+        }
+    }
+
+    pub fn csv_options_or_default(&self) -> CsvOptions {


Likewise, these new public methods should have documentation

After reading this some more, I don't undertand the usecase for csv_options_or_default (and the other similar methods)

Since this is now an enum, the calling code should directly handle the format in the TableFormatOptions -- this code will silently drop any config options if it isn't CSV which seems like it would potentially mask a bug in the caller

I would expect calling code to do something like

match TableFormatOptions { Self::Csv => {} Self::Json => {} Self::Parquet => {{ }

berkaysynnada · 2025-05-29T06:15:29Z

Thank you for this contribution @berkaysynnada and @mertak-synnada

I am a little confused about the new structure and exactly what problem is being solved with this change.

The PR description says

In the current implementation, the TableOptions struct stores information for CSV, JSON, and Parquet file types, and its behavior changes depending on whether the file format is set before or after.

However, I didn't see any tests or examples that show a different behavior after this PR.

In my initial quick read through it I would say it has made the APIs harder to use (as now there are format specific options on TableOptions AND a new TableFormatOptions). Is there any way to consolidate the number of structures? If not, I think we should clearly document the difference between the two structures

The confusion we are trying to address is that the TableOptions struct is currently being used in two different ways:

As a sub-configuration field in SessionState, similar to other configuration fields.
For directly configuring the specific file currently being worked on, typically by cloning it from the SessionState and overriding fields as needed.,

In other words, it seems like we actually need two distinct objects for these two different use cases:

One for storing global TableOptions configuration across the session, applicable to all file types.
Another for holding format-specific options derived from the global configuration, intended for the currently active file.
Another key point is that once you set a specific file format, you don’t need the configuration fields for the others in the existing TableOptions. So for the second use case, the appropriate design might be an enum rather than a struct.

In summary, this PR is more of a code cleanup than a functional change. It aims to clarify the separation of concerns and improve maintainability, in my opinion.

I'd like to go over this PR one more time and dropping the approve to not cause someone merge this prematurely

alamb · 2025-05-30T09:48:00Z

datafusion/common/src/config.rs

+    /// # Returns
+    ///
+    /// A result indicating success or failure in applying the settings.
+    pub fn alter_with_string_hash_map(


maybe update would be clearer as alter might imply some change other than updating the map

alamb · 2025-05-30T09:51:42Z

datafusion/common/src/config.rs

+        }
+    }
+
+    pub fn csv_options_or_default(&self) -> CsvOptions {


After reading this some more, I don't undertand the usecase for csv_options_or_default (and the other similar methods)

Since this is now an enum, the calling code should directly handle the format in the TableFormatOptions -- this code will silently drop any config options if it isn't CSV which seems like it would potentially mask a bug in the caller

I would expect calling code to do something like

match TableFormatOptions { Self::Csv => {} Self::Json => {} Self::Parquet => {{ }

alamb · 2025-05-30T09:52:59Z

However, I didn't see any tests or examples that show a different behavior after this PR.
In my initial quick read through it I would say it has made the APIs harder to use (as now there are format specific options on TableOptions AND a new TableFormatOptions). Is there any way to consolidate the number of structures? If not, I think we should clearly document the difference between the two structures

The confusion we are trying to address is that the TableOptions struct is currently being used in two different ways:

As a sub-configuration field in SessionState, similar to other configuration fields.

For directly configuring the specific file currently being worked on, typically by cloning it from the SessionState and overriding fields as needed.,

Yes I agree this is confusing

In other words, it seems like we actually need two distinct objects for these two different use cases:

One for storing global TableOptions configuration across the session, applicable to all file types.

Another for holding format-specific options derived from the global configuration, intended for the currently active file.
Another key point is that once you set a specific file format, you don’t need the configuration fields for the others in the existing TableOptions. So for the second use case, the appropriate design might be an enum rather than a struct.

In summary, this PR is more of a code cleanup than a functional change. It aims to clarify the separation of concerns and improve maintainability, in my opinion.

I see. Thank you.

I think we could largely resolve the confusion with some better documentation (perhaps documentation that explained the differences clearly so that when someone stumbled on the very similarly named TableOptions and TableFormatOptions there would be some context to help them understand

Here are some other ideas:

Different Names

Use names that make the difference between the structs clearer.

Perhaps something like the following:

DefaultTableOptions for the structure on ConfigOptions (currently called TableOptions)
ResolvedTableOptions or SpecificTableOptions for configuring a file specific option (called TableFormatOptions in this PR)

Use the inner structures directly

Once we know the format, could we potentially then change the code to use the ParquetOptions or CsvOptions directly?

mertak-synnada and others added 15 commits April 28, 2025 15:04

initialize TableOptions as enum with different file types

20912ba

format code

fffa42b

re define TableOptions as struct

9116d46

separate FileSpecificTableOptions as enum

reduce diff

42b51af

centralize create functions for CsvFormat, ParquetFormat and JsonFormat

afbf3db

fix no-default-features error

c2aedbd

minor

48d621c

add comments

8b0d06d

Merge remote-tracking branch 'origin/chore/table-options-enum' into c…

86db07c

…hore/table-options-enum

Merge branch 'refs/heads/apache_main' into chore/table-options-enum

12a9c1c

# Conflicts: # datafusion/core/src/datasource/file_format/arrow.rs # datafusion/datasource/src/file_format.rs

change FormatOptions to OutputFormat

6d12edc

remove parquet configuration check from session.rs

a3fe26f

format code

b4a5143

fix parquet feature check on session.rs

53488e2

format toml file

5220e91

github-actions bot added core Core DataFusion crate common Related to common crate datasource Changes to the datasource crate labels May 23, 2025

Merge branch 'main' into chore/table-options-enum

58ba31c

berkaysynnada previously approved these changes May 27, 2025

View reviewed changes

alamb reviewed May 28, 2025

View reviewed changes

alamb reviewed May 30, 2025

View reviewed changes


		fn default_from_output_format(&self, options: OutputFormat) -> Arc<dyn FileFormat>;

		fn options(&self) -> (Option<OutputFormat>, ConfigFileType);

Set Formatted TableOptions Enum #16166

Are you sure you want to change the base?

Set Formatted TableOptions Enum #16166

Uh oh!

Conversation

mertak-synnada commented May 23, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Which issue does this PR close?

Rationale for this change

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

Uh oh!

berkaysynnada left a comment

Choose a reason for hiding this comment

Uh oh!

alamb left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

berkaysynnada commented May 29, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alamb commented May 30, 2025

Different Names

Use the inner structures directly

Uh oh!

Uh oh!

mertak-synnada commented May 23, 2025 •

edited

Loading