support unprefixed config format options #9594

tinfoil-knight · 2024-03-13T10:44:29Z

Which issue does this PR close?

Rationale for this change

#9382 standardised write configuration but broke backwards compatibility for the format specific options of the COPY command.

Earlier users could have a query like this without prefixing all options:

COPY source_table TO 'test_files/scratch/copy/format_table/' (
    format parquet,
    compression snappy,
    'compression::col1' 'zstd(5)'
);

But after #9382, they'd need to prefix every format-specific options in this verbose way:

COPY source_table TO 'test_files/scratch/copy/format_table/' (
    format parquet,
    'parquet.compression' snappy,
    'parquet.compression::col1' 'zstd(5)'
);

This PR adds back the support for format-specific options that worked without providing the format prefix.

What changes are included in this PR?

This PR renames keys without the file_type/format prefix for file types supported by the COPY command for backwards compatibility.

Are these changes tested?

Existing tests in copy.slt confirm that the prefixed format options (eg. (format json, 'json.compression' gzip)) added in Systematic Configuration in 'Create External Table' and 'Copy To' Options #9382 work & aren't broken.
Tests have been added for the legacy format (eg. (format json, compression gzip)).

Are there any user-facing changes?

Yes. But the current documentation (https://arrow.apache.org/datafusion/user-guide/sql/write_options.html#available-options) lists the legacy format so this PR doesn't require a doc update.

If there are any breaking changes to public APIs, please add the api change label.

This PR restores breaking changes so I'm not adding the api change label. Please let me know if you think we should add the label nonetheless.

devinjdangelo

Thank you for working on this @tinfoil-knight! The overall approach looks good and is well tested. I have proposed a few changes. Let me know what you think.

devinjdangelo · 2024-03-13T12:56:24Z

datafusion/sql/src/statement.rs

+        match &file_type {
+            // Renames un-prefixed keys to support legacy format specific options
+            FileType::CSV | FileType::JSON | FileType::PARQUET => {
+                let prefix = format!("{}", file_type);
+                let keys_to_rename: Vec<_> = options
+                    .keys()
+                    .filter(|key| !key.starts_with(&prefix))
+                    .cloned()
+                    .collect();
+
+                for key in keys_to_rename {
+                    if let Some(value) = options.remove(&key) {
+                        let new_key = format!("{}.{}", prefix, key);
+                        options.insert(new_key, value);
+                    }
+                }
+            }
+            _ => {}
+        }
+


Suggested change

match &file_type {

// Renames un-prefixed keys to support legacy format specific options

FileType::CSV | FileType::JSON | FileType::PARQUET => {

let prefix = format!("{}", file_type);

let keys_to_rename: Vec<_> = options

.keys()

.filter(|key| !key.starts_with(&prefix))

.cloned()

.collect();

for key in keys_to_rename {

if let Some(value) = options.remove(&key) {

let new_key = format!("{}.{}", prefix, key);

options.insert(new_key, value);

}

}

}

_ => {}

}

let options = match &file_type {

// Renames un-prefixed keys to support legacy format specific options

FileType::CSV | FileType::JSON | FileType::PARQUET => {

options

.into_iter()

.map(|(k, v)| {

// If config does not belong to any namespace, assume it is

// a legacy option and apply file_type namespace for backwards

// compatibility.

if !k.contains('.') {

let new_key = format!("{}.{}", file_type, k);

(new_key, v)

} else {

(k, v)

}

})

.collect()

}

_ => options,

};

The overall strategy here looks good, but I propose two changes:

We can make the logic more concise by modifying options in a single pass with into_iter, map, collect. This also avoids clones.

I think we should be a bit more conservative about the keys we modify. If the key has a namespace at all (rather than specifically the file format namespace), we can leave it as is. Downstream users may for example add a custom namespace and we wouldn't wan't this code to modify it unexpectedly.

Implemented both changes.

Converted the PR to draft for the while being due to #9594 (comment).

alamb

Thank you @tinfoil-knight and @devinjdangelo

I think once @devinjdangelo 's comments are addressed this PR will be ready to go

cc @metesynnada @ozankabak and @mustafasrepo as you commented on the original PR

alamb · 2024-03-13T15:28:35Z

datafusion/sqllogictest/test_files/copy.slt

@@ -484,3 +484,35 @@ COPY (select col2, sum(col1) from source_table
 # Copy from table with non literal
 query error DataFusion error: SQL error: ParserError\("Expected ',' or '\)' after option definition, found: \+"\)
 COPY source_table  to '/tmp/table.parquet' (row_group_size 55 + 102);
+
+
+# Legacy Format Options Support


ozankabak · 2024-03-13T18:08:10Z

Thank you @tinfoil-knight for this. We will do a quick follow-up finalizing the design in #9382. Let's get the follow-up in and then modify this/get this in afterwards for backwards compatibility.

tinfoil-knight · 2024-03-13T18:16:34Z

We will do a quick follow-up finalizing the design in #9382. Let's get the follow-up in and then modify this/get this in afterwards for backwards compatibility.

Okay. Sounds good.

datafusion/sql/src/statement.rs

devinjdangelo · 2024-03-19T13:14:00Z

@tinfoil-knight I think this should be unblocked now that #9382 is closed. If you have a moment to rebase/resolve any conflicts and CI failures, I think we can get this merged after a final review.

alamb · 2024-03-19T17:08:25Z

I think we should also consider holding the release for this PR (see #9682 ) - so if you need help finishing it up @tinfoil-knight please just let us know

tinfoil-knight · 2024-03-19T17:50:02Z

I think we should also consider holding the release for this PR (see #9682 ) - so if you need help finishing it up @tinfoil-knight please just let us know

I'll fix the tests by tomorrow & will let you know if anything comes up.

tinfoil-knight · 2024-03-20T15:14:09Z

@devinjdangelo I'm not sure if the original issue is valid anymore. After #9604, you can use formatting options without prefixing them with the format name. See this for an example:
https://github.com/apache/arrow-datafusion/blob/89efc4a7e06bd0295ca72dd6ec5fe987d1ac246b/datafusion/sqllogictest/test_files/copy.slt#L22-L32

I think this PR & the linked issue should be closed.

@alamb noted an issue here: #9604 (review) with the format prefix but IMO it's separate from the original issue.

I also found a CLI bug while working on this issue & I've filed: #9707 & pushed a PR for it separately.

tinfoil-knight · 2024-03-20T19:20:23Z

Update:

#9716 is opened now for the format prefix issue.

@alamb I'm closing this PR since it doesn't seem relevant anymore. See the previous comment (#9594 (comment)) for more details.

Please close the issue: #9575 now as it's stale.

support unprefixed config format options

7af17ba

github-actions bot added sql sqllogictest labels Mar 13, 2024

devinjdangelo suggested changes Mar 13, 2024

View reviewed changes

alamb reviewed Mar 13, 2024

View reviewed changes

This was referenced Mar 13, 2024

DataFusion weekly project plan (Andrew Lamb) - March 11, 2024 #9555

Closed

COPY TO allign with CREATE EXTERNAL TABLE synnada-ai/datafusion-upstream#10

Closed

avoid clones, stop modifying any namespaced key

2e9f919

tinfoil-knight marked this pull request as draft March 13, 2024 18:16

tinfoil-knight commented Mar 13, 2024

View reviewed changes

datafusion/sql/src/statement.rs Show resolved Hide resolved

tinfoil-knight closed this Mar 20, 2024

alamb mentioned this pull request Mar 20, 2024

Support backwards compatible / un prefixed config options (compression rather than parquet.compression) #9575

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

support unprefixed config format options #9594

support unprefixed config format options #9594

tinfoil-knight commented Mar 13, 2024

devinjdangelo left a comment

devinjdangelo Mar 13, 2024

devinjdangelo Mar 13, 2024

tinfoil-knight Mar 13, 2024

alamb left a comment

alamb Mar 13, 2024

ozankabak commented Mar 13, 2024

tinfoil-knight commented Mar 13, 2024

devinjdangelo commented Mar 19, 2024

alamb commented Mar 19, 2024 •

edited

Loading

tinfoil-knight commented Mar 19, 2024

tinfoil-knight commented Mar 20, 2024 •

edited

Loading

tinfoil-knight commented Mar 20, 2024

support unprefixed config format options #9594

support unprefixed config format options #9594

Conversation

tinfoil-knight commented Mar 13, 2024

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?

devinjdangelo left a comment

Choose a reason for hiding this comment

devinjdangelo Mar 13, 2024

Choose a reason for hiding this comment

devinjdangelo Mar 13, 2024

Choose a reason for hiding this comment

tinfoil-knight Mar 13, 2024

Choose a reason for hiding this comment

alamb left a comment

Choose a reason for hiding this comment

alamb Mar 13, 2024

Choose a reason for hiding this comment

ozankabak commented Mar 13, 2024

tinfoil-knight commented Mar 13, 2024

devinjdangelo commented Mar 19, 2024

alamb commented Mar 19, 2024 • edited Loading

tinfoil-knight commented Mar 19, 2024

tinfoil-knight commented Mar 20, 2024 • edited Loading

tinfoil-knight commented Mar 20, 2024

alamb commented Mar 19, 2024 •

edited

Loading

tinfoil-knight commented Mar 20, 2024 •

edited

Loading