Remove single_file_output option from FileSinkConfig and Copy statement #9041
Conversation
github-actions
bot
added
sql
There was a problem hiding this comment.
Thank you @yyy1000 -- I think this looks great. Thank you for the help and contribution 🙏
FYI @devinjdangelo in case you have some time to review or have additional comments or changes to suggest
|
Haha, I'm happy that I could help it. :) |
| @@ -718,19 +718,13 @@ impl<'a, S: ContextProvider> SqlToRel<'a, S> { | |||
|
|
|||
| let mut statement_options = StatementOptions::new(options); | |||
| let file_format = statement_options.try_infer_file_type(&statement.target)?; | |||
| let single_file_output = | |||
| statement_options.take_bool_option("single_file_output")?; | |||
There was a problem hiding this comment.
It may be worth keeping the call to take_bool_option and discarding the result. That will allow queries that specify single file output to continue working as expected in most cases.
There was a problem hiding this comment.
Seems a good idea.
But I worry that some users didn't specify the output folder path using /, but using a
single_output_option = false. Keeping this option may let the query running but has different result. 🤔
There was a problem hiding this comment.
I agree that it seems cleaner to just remove the option rather than ignore it
Which issue does this PR close?
Closes #8621
Rationale for this change
See #8621 for details
What changes are included in this PR?
single_file_outputinFileSinkConfigandLogicalPlan::Copy, but using/in path to see whether it's a single file or not.single_file_optionas a param. (2). Test cases which used explicitsingle_file_output = falseand didn't specify the path ended in/, (in modified code, the path will be treated as a single file which caused error)Are these changes tested?
Yes,
cargo testin local envAre there any user-facing changes?
I think yes?