You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "alamb (via GitHub)" <gi...@apache.org> on 2023/08/08 13:22:03 UTC
[GitHub] [arrow-datafusion] alamb opened a new pull request, #7229: Minor: remove duplication in `create_writer`
alamb opened a new pull request, #7229:
URL: https://github.com/apache/arrow-datafusion/pull/7229
## Which issue does this PR close?
Closes #.
## Rationale for this change
I noticed some code duplication while reviewing https://github.com/apache/arrow-datafusion/pull/7212 from @devinjdangelo and I think this code will be used more when adding write support for avro, parquet and arrow files. Thus it makes sense to put it in its own function
## What changes are included in this PR?
Refactor `CsvSink::create_writer` and `JsonSink::create_writer`
## Are these changes tested?
covered by existing tests
<!--
We typically require tests for all PRs in order to:
1. Prevent the code from being accidentally broken by subsequent changes
2. Serve as another way to document the expected behavior of the code
If tests are not included in your PR, please explain why (for example, are they covered by existing tests)?
-->
## Are there any user-facing changes?
no
<!--
If there are user-facing changes then we may require documentation to be updated before approving the PR.
-->
<!--
If there are any breaking changes to public APIs, please add the `api change` label.
-->
--
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
[GitHub] [arrow-datafusion] devinjdangelo commented on a diff in pull request #7229: Minor: remove duplication in `create_writer`
Posted by "devinjdangelo (via GitHub)" <gi...@apache.org>.
devinjdangelo commented on code in PR #7229:
URL: https://github.com/apache/arrow-datafusion/pull/7229#discussion_r1287674162
##########
datafusion/core/src/datasource/file_format/mod.rs:
##########
@@ -235,7 +239,7 @@ pub(crate) enum AbortMode {
}
/// A wrapper struct with abort method and writer
-struct AbortableWrite<W: AsyncWrite + Unpin + Send> {
+pub(crate) struct AbortableWrite<W: AsyncWrite + Unpin + Send> {
Review Comment:
That is a good idea. The code is likely to become more complex / long with additional helpers as we get into parallelization and dealing with parquet. I'm working on a PR to add support for inserting parquet now, and I can consolidate the shared write code into a new module as part of that.
##########
datafusion/core/src/datasource/file_format/csv.rs:
##########
@@ -494,56 +490,6 @@ impl CsvSink {
file_compression_type,
}
}
-
- // Create a write for Csv files
- async fn create_writer(
Review Comment:
Nice catch!
##########
datafusion/core/src/datasource/file_format/file_type.rs:
##########
@@ -62,7 +62,7 @@ pub trait GetExt {
}
/// Readable file compression type
-#[derive(Debug, Clone, PartialEq, Eq)]
+#[derive(Debug, Clone, Copy, PartialEq, Eq)]
Review Comment:
❤️
##########
datafusion/core/src/datasource/file_format/file_type.rs:
##########
@@ -62,7 +62,7 @@ pub trait GetExt {
}
/// Readable file compression type
-#[derive(Debug, Clone, PartialEq, Eq)]
+#[derive(Debug, Clone, Copy, PartialEq, Eq)]
Review Comment:
❤️
--
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
[GitHub] [arrow-datafusion] devinjdangelo commented on a diff in pull request #7229: Minor: remove duplication in `create_writer`
Posted by "devinjdangelo (via GitHub)" <gi...@apache.org>.
devinjdangelo commented on code in PR #7229:
URL: https://github.com/apache/arrow-datafusion/pull/7229#discussion_r1288423268
##########
datafusion/core/src/datasource/file_format/mod.rs:
##########
@@ -235,7 +239,7 @@ pub(crate) enum AbortMode {
}
/// A wrapper struct with abort method and writer
-struct AbortableWrite<W: AsyncWrite + Unpin + Send> {
+pub(crate) struct AbortableWrite<W: AsyncWrite + Unpin + Send> {
Review Comment:
Opened a draft here #7244
--
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
[GitHub] [arrow-datafusion] metesynnada commented on a diff in pull request #7229: Minor: remove duplication in `create_writer`
Posted by "metesynnada (via GitHub)" <gi...@apache.org>.
metesynnada commented on code in PR #7229:
URL: https://github.com/apache/arrow-datafusion/pull/7229#discussion_r1288375362
##########
datafusion/core/src/datasource/file_format/mod.rs:
##########
@@ -306,6 +310,59 @@ pub enum FileWriterMode {
/// Data is written to a new file in multiple parts.
PutMultipart,
}
+
+/// return an [`AbortableWrite`] that writes to the specified object
+/// store location and compression
Review Comment:
```suggestion
/// Returns an [`AbortableWrite`] which writes to the given object store location
/// with the specified compression
```
--
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
[GitHub] [arrow-datafusion] alamb commented on a diff in pull request #7229: Minor: remove duplication in `create_writer`
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #7229:
URL: https://github.com/apache/arrow-datafusion/pull/7229#discussion_r1287113760
##########
datafusion/core/src/datasource/file_format/csv.rs:
##########
@@ -494,56 +490,6 @@ impl CsvSink {
file_compression_type,
}
}
-
- // Create a write for Csv files
- async fn create_writer(
Review Comment:
the whole point of this PR is to move this into a free function that is shared rather than have two copies
##########
datafusion/core/src/datasource/file_format/mod.rs:
##########
@@ -235,7 +239,7 @@ pub(crate) enum AbortMode {
}
/// A wrapper struct with abort method and writer
-struct AbortableWrite<W: AsyncWrite + Unpin + Send> {
+pub(crate) struct AbortableWrite<W: AsyncWrite + Unpin + Send> {
Review Comment:
this module is getting pretty large -- I think it would be easier to navigate it if were split into some smaller modules (e.g. put the write related code into `datafusion/core/src/datasource/file_format/write.rs`, for example). Any interest in doing so @devinjdangelo or @metesynnada ? If not, I can give it a try
--
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
[GitHub] [arrow-datafusion] alamb merged pull request #7229: Minor: remove duplication in `create_writer`
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb merged PR #7229:
URL: https://github.com/apache/arrow-datafusion/pull/7229
--
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