feat: Conditionally allow to keep partition_by columns when using PARTITIONED BY enhancement #11107

hveiga · 2024-06-24T17:50:47Z

Which issue does this PR close?

Closes #10971

What changes are included in this PR?

Added a flag to FileSinkConfig to conditionally enable this feature. Disabled by default.

Are these changes tested?

Test is included as part of copy.slt.

Are there any user-facing changes?

No breaking changes.
Added some documentation for this new option.

datafusion/core/src/physical_planner.rs

alamb

Thank you for this contribution @hveiga -- I think this PR looks quite close.

I think we need should make sure this configuration setting matches the pattern of the other settings (it is somewhat special as it isn't a format specific option, and it doesn't have special DML syntax either...)

datafusion/core/src/physical_planner.rs

datafusion/sqllogictest/test_files/copy.slt

docs/source/user-guide/sql/dml.md

devinjdangelo

Thank you @hveiga, this looks great! I think we should clean up the fact that "format." is being added as a prefix to this option and create a different name space for it instead. This could help avoid the TableOptions raising an error for an unexpected format option as well.

datafusion/core/src/datasource/listing/table.rs

datafusion/core/src/physical_planner.rs

docs/source/user-guide/sql/dml.md

devinjdangelo · 2024-06-25T01:44:29Z

One last thing we could add in this PR (or a ticket for a follow on) would be to add a session level configuration setting for this, so users could control the default behavior.

hveiga · 2024-06-25T16:45:39Z

I think I have addressed all the comments in the PR, the only lingering one is about flowing the new option from SQL when using CREATE EXTERNAL TABLE. Thank you for the quick turnaround on the reviews and comments. I believe the new option keep_partition_by_columns would need to also be added to ListingOptions and parsed accordingly. Is that the way to go?

devinjdangelo

Thank you for the updates @hveiga ! The new session parameter is a great addition. I do think we should avoid changing the CopyTo struct.

Updates to create external table can wait for a future PR in my view.

datafusion/expr/src/logical_plan/dml.rs

- separate options by prefix 'hive.' - add hive_options to CopyTo struct - add more documentation - add session execution flag to enable feature, false by default

hveiga · 2024-06-25T22:41:33Z

Thank you for the updates @hveiga ! The new session parameter is a great addition. I do think we should avoid changing the CopyTo struct.

Updates to create external table can wait for a future PR in my view.

Thanks for the review. I reverted the hive_options changes and the PR is ready for (yet another) review :)

alamb · 2024-06-25T23:23:41Z

I took the liberty of pushing some commits to this PR to fix some CI errors

alamb

Thank you @hveiga and @devinjdangelo and @berkaysynnada for the reviews

It would be great if @devinjdangelo and @berkaysynnada could give this another review, but I think it is good enough to merge now.

I agree we should file a follow on ticket to figure out how to support this as part of the CREATE EXTERNAL TABLE syntax

alamb · 2024-06-25T23:24:57Z

datafusion/sql/src/statement.rs

@@ -888,7 +888,15 @@ impl<'a, S: ContextProvider> SqlToRel<'a, S> {
                }
                Some(v) => v,
            };
-            if !(&key.contains('.')) {
+
+            if key.to_lowercase().contains("keep_partition_by_columns") {


this special case is unfortunate, but I don't have a great idea of how to make it better

After further consideration, IMO we should restrict this handling to format options. For other configurations, users must specify the prefix. Otherwise, this list will continue to grow longer, and using those prefixes would lose their meaning (which is why all these refactors were done to have a structured configuration).

Additionally, instead of using "hive," we need a more general term. Perhaps the "execution" prefix would be a better alternative.

Could you elaborate on what you mean by "we should restrict this handling to format options"? I am unsure if you are suggesting using a different struct for these non-format. options or continue using the HashMap with a mix of options and then extract out the logic to filter out the non-format. options and do not pass those down through table_options.alter_with_string_hash_map.

I agree on changing the prefix to execution. which also aligns with the ExecutionOptions.

I gave a try to this in e91397a . Let me know if that's what you had in mind. Thanks for feedback.

That's exactly what I had in my mind. Thank you for the collaboration. I just would like to mention two more points:

I guess you missed to add execution prefix to the example in datafusion/sql/src/parser.rs.

let keep_partition_by_columns = source_option_tuples .get("execution.keep_partition_by_columns") .map(|v| v.trim() == "true") .unwrap_or(...

If the user provides anything other than "true," it is interpreted as "false." It might be wiser to give an error if the value is neither "true" nor "false."

Good catch! I just fixed that test case and also slightly modified the handling of the value to account for what we discussed:

Give preference to what is explicitly provided.

If what is provided is invalid, through a config error. Added a test in copy.slt for this.

If not provided, fallback to ExecutionOptions value, false by default.

Hopefully e352203 is finally ready. Thanks.

This PR is a good first step towards adding settings other than format. Thanks for your effort, @hveiga. I think the PR is ready to be merged once conflicts are resolved.

berkaysynnada

I have one minor issue left. Once that is resolved, we can merge the PR. Thanks, @hveiga.

datafusion/core/src/physical_planner.rs

alamb · 2024-06-28T14:43:10Z

I merged up from main to resolve some conflicts (and also now so that CI will run automatically). Once CI passes I plan to merge this PR

alamb · 2024-06-28T14:43:15Z

Thanks again @hveiga and @berkaysynnada

hveiga · 2024-06-28T15:50:36Z

Thanks everyone for the help to get this over the finish line! Excited to see this in the next release of Datafusion 🎉

alamb · 2024-06-28T20:13:45Z

🚀

…TITIONED BY enhancement (apache#11107) * feat: conditionally allow to keep partition_by columns * feat: add flag to file sink config, add tests * this commit contains: - separate options by prefix 'hive.' - add hive_options to CopyTo struct - add more documentation - add session execution flag to enable feature, false by default * do not add hive_options to CopyTo * npx prettier * fmt * change prefix to execution. , update override order for condition. * improve handling of flag, added test for config error * trying to make CI happier * prettier * Update test * update doc --------- Co-authored-by: Héctor Veiga Ortiz <hveigaortiz@tesla.com> Co-authored-by: Andrew Lamb <andrew@nerdnetworks.org>

github-actions bot added sql SQL Planner core Core DataFusion crate sqllogictest SQL Logic Tests (.slt) labels Jun 24, 2024

hveiga commented Jun 24, 2024

View reviewed changes

datafusion/core/src/physical_planner.rs Show resolved Hide resolved

alamb reviewed Jun 24, 2024

View reviewed changes

datafusion/core/src/physical_planner.rs Show resolved Hide resolved

datafusion/sqllogictest/test_files/copy.slt Show resolved Hide resolved

docs/source/user-guide/sql/dml.md Show resolved Hide resolved

devinjdangelo reviewed Jun 25, 2024

View reviewed changes

datafusion/core/src/datasource/listing/table.rs Outdated Show resolved Hide resolved

datafusion/core/src/physical_planner.rs Show resolved Hide resolved

datafusion/core/src/physical_planner.rs Outdated Show resolved Hide resolved

docs/source/user-guide/sql/dml.md Show resolved Hide resolved

github-actions bot added the logical-expr Logical plan and expressions label Jun 25, 2024

devinjdangelo reviewed Jun 25, 2024

View reviewed changes

datafusion/expr/src/logical_plan/dml.rs Outdated Show resolved Hide resolved

Héctor Veiga Ortiz added 3 commits June 25, 2024 15:37

feat: conditionally allow to keep partition_by columns

a038d6f

feat: add flag to file sink config, add tests

44be074

this commit contains:

43b931c

- separate options by prefix 'hive.' - add hive_options to CopyTo struct - add more documentation - add session execution flag to enable feature, false by default

hveiga force-pushed the issue-10971 branch from 3ec3ecc to 24e4d25 Compare June 25, 2024 22:37

do not add hive_options to CopyTo

1578273

hveiga force-pushed the issue-10971 branch from 24e4d25 to 1578273 Compare June 25, 2024 22:39

alamb added 3 commits June 25, 2024 19:23

npx prettier

20197a1

fmt

206b274

Merge remote-tracking branch 'apache/main' into issue-10971

89adc88

alamb approved these changes Jun 25, 2024

View reviewed changes

berkaysynnada reviewed Jun 26, 2024

View reviewed changes

datafusion/core/src/physical_planner.rs Outdated Show resolved Hide resolved

Héctor Veiga Ortiz and others added 4 commits June 26, 2024 11:40

change prefix to execution. , update override order for condition.

e91397a

improve handling of flag, added test for config error

e352203

trying to make CI happier

4132827

Merge remote-tracking branch 'apache/main' into issue-10971

23d95c0

prettier

f4e2158

Update test

5f02278

alamb added 2 commits June 28, 2024 14:04

Merge remote-tracking branch 'apache/main' into issue-10971

e76d811

update doc

fdbca78

alamb merged commit 330ece8 into apache:main Jun 28, 2024
24 checks passed

alamb mentioned this pull request Jul 1, 2024

DataFusion weekly project plan (Andrew Lamb) - July 1, 2024 #11190

Closed

10 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Conditionally allow to keep partition_by columns when using PARTITIONED BY enhancement #11107

feat: Conditionally allow to keep partition_by columns when using PARTITIONED BY enhancement #11107

hveiga commented Jun 24, 2024

alamb left a comment

devinjdangelo left a comment

devinjdangelo commented Jun 25, 2024

hveiga commented Jun 25, 2024 •

edited

Loading

devinjdangelo left a comment

hveiga commented Jun 25, 2024

alamb commented Jun 25, 2024

alamb left a comment

alamb Jun 25, 2024

berkaysynnada Jun 26, 2024

hveiga Jun 26, 2024 •

edited

Loading

hveiga Jun 26, 2024

berkaysynnada Jun 27, 2024

hveiga Jun 27, 2024 •

edited

Loading

berkaysynnada Jun 28, 2024 •

edited

Loading

berkaysynnada left a comment

alamb commented Jun 28, 2024

alamb commented Jun 28, 2024

hveiga commented Jun 28, 2024

alamb commented Jun 28, 2024

feat: Conditionally allow to keep partition_by columns when using PARTITIONED BY enhancement #11107

feat: Conditionally allow to keep partition_by columns when using PARTITIONED BY enhancement #11107

Conversation

hveiga commented Jun 24, 2024

Which issue does this PR close?

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

alamb left a comment

Choose a reason for hiding this comment

devinjdangelo left a comment

Choose a reason for hiding this comment

devinjdangelo commented Jun 25, 2024

hveiga commented Jun 25, 2024 • edited Loading

devinjdangelo left a comment

Choose a reason for hiding this comment

hveiga commented Jun 25, 2024

alamb commented Jun 25, 2024

alamb left a comment

Choose a reason for hiding this comment

alamb Jun 25, 2024

Choose a reason for hiding this comment

berkaysynnada Jun 26, 2024

Choose a reason for hiding this comment

hveiga Jun 26, 2024 • edited Loading

Choose a reason for hiding this comment

hveiga Jun 26, 2024

Choose a reason for hiding this comment

berkaysynnada Jun 27, 2024

Choose a reason for hiding this comment

hveiga Jun 27, 2024 • edited Loading

Choose a reason for hiding this comment

berkaysynnada Jun 28, 2024 • edited Loading

Choose a reason for hiding this comment

berkaysynnada left a comment

Choose a reason for hiding this comment

alamb commented Jun 28, 2024

alamb commented Jun 28, 2024

hveiga commented Jun 28, 2024

alamb commented Jun 28, 2024

hveiga commented Jun 25, 2024 •

edited

Loading

hveiga Jun 26, 2024 •

edited

Loading

hveiga Jun 27, 2024 •

edited

Loading

berkaysynnada Jun 28, 2024 •

edited

Loading