You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "yyy1000 (via GitHub)" <gi...@apache.org> on 2024/01/29 06:59:59 UTC
[PR] Remove single_file_output option from FileSinkConfig and Copy statement [arrow-datafusion]
yyy1000 opened a new pull request, #9041:
URL: https://github.com/apache/arrow-datafusion/pull/9041
## Which issue does this PR close?
Closes #8621
## Rationale for this change
See #8621 for details
## What changes are included in this PR?
1. Remove `single_file_output` in `FileSinkConfig` and `LogicalPlan::Copy`, but using `/` in path to see whether it's a single file or not.
2. Remove the field in proto for serde
3. Change test cases, this includes two aspects, (1). Test cases will not need `single_file_option` as a param. (2). Test cases which used explicit `single_file_output = false` and didn't specify the path ended in `/ `, (in modified code, the path will be treated as a single file which caused error)
4. Document update, though I think it needs to be polished.
## Are these changes tested?
Yes, `cargo test` in local env
## Are there any user-facing changes?
I think yes?
--
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@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Remove single_file_output option from FileSinkConfig and Copy statement [arrow-datafusion]
Posted by "devinjdangelo (via GitHub)" <gi...@apache.org>.
devinjdangelo commented on code in PR #9041:
URL: https://github.com/apache/arrow-datafusion/pull/9041#discussion_r1471190059
##########
datafusion/sql/src/statement.rs:
##########
@@ -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")?;
Review 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.
--
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@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Remove single_file_output option from FileSinkConfig and Copy statement [arrow-datafusion]
Posted by "yyy1000 (via GitHub)" <gi...@apache.org>.
yyy1000 commented on PR #9041:
URL: https://github.com/apache/arrow-datafusion/pull/9041#issuecomment-1915652448
Haha, I'm happy that I could help it. :)
--
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@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Remove single_file_output option from FileSinkConfig and Copy statement [arrow-datafusion]
Posted by "yyy1000 (via GitHub)" <gi...@apache.org>.
yyy1000 commented on code in PR #9041:
URL: https://github.com/apache/arrow-datafusion/pull/9041#discussion_r1471465691
##########
datafusion/sql/src/statement.rs:
##########
@@ -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")?;
Review 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. 🤔
--
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@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org