You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by GitBox <gi...@apache.org> on 2022/02/19 19:19:48 UTC
[GitHub] [arrow-rs] gsserge opened a new pull request #1342: Remove delimiter from csv Writer
gsserge opened a new pull request #1342:
URL: https://github.com/apache/arrow-rs/pull/1342
# Which issue does this PR close?
Closes issue https://github.com/apache/arrow-rs/issues/1328.
I think that I wrote the issue above in a bit of a rush, the part about changes to `Writer::new()`. The doc comment for `Writer::new()` clearly states that a new CsvWriter is initialized with default options, with `,` being the default delimiter.
A custom delimiter can be configured using already existing `WriterBuilder::with_delimiter`. So, in my view, the scope of the issue is to simply remove unused code without any changes to the API.
# Rationale for this change
There is no use for the `delimiter` field in csv `Writer`.
# What changes are included in this PR?
The unused `delimiter` field is removed.
# Are there any user-facing changes?
No.
--
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-rs] Dandandan commented on pull request #1342: Remove delimiter from csv Writer
Posted by GitBox <gi...@apache.org>.
Dandandan commented on pull request #1342:
URL: https://github.com/apache/arrow-rs/pull/1342#issuecomment-1046095762
> There's already a test for custom options, including the delimiter: https://github.com/apache/arrow-rs/blob/master/arrow/src/csv/writer.rs#L637
Perfect, thanks!
--
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-rs] Dandandan commented on pull request #1342: Remove delimiter from csv Writer
Posted by GitBox <gi...@apache.org>.
Dandandan commented on pull request #1342:
URL: https://github.com/apache/arrow-rs/pull/1342#issuecomment-1046092817
> ```rust
> /// Create a new `Writer`
> pub fn build<W: Write>(self, writer: W) -> Writer<W> {
> let delimiter = self.delimiter.unwrap_or(b','); // looks like it's used
> let mut builder = csv_crate::WriterBuilder::new();
> let writer = builder.delimiter(delimiter).from_writer(writer);
> ...
> ```
Yes it's used in the builder, but looks to me it's not passed to the `Writer` eventually (and so not used when writing the CSV).
--
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-rs] Dandandan commented on pull request #1342: Remove delimiter from csv Writer
Posted by GitBox <gi...@apache.org>.
Dandandan commented on pull request #1342:
URL: https://github.com/apache/arrow-rs/pull/1342#issuecomment-1046090493
I think the delimiter is not used in the builder (when converting to Writer), so I think the original issue description is correct?
--
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-rs] nevi-me merged pull request #1342: Remove delimiter from csv Writer
Posted by GitBox <gi...@apache.org>.
nevi-me merged pull request #1342:
URL: https://github.com/apache/arrow-rs/pull/1342
--
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-rs] codecov-commenter commented on pull request #1342: Remove delimiter from csv Writer
Posted by GitBox <gi...@apache.org>.
codecov-commenter commented on pull request #1342:
URL: https://github.com/apache/arrow-rs/pull/1342#issuecomment-1046089238
# [Codecov](https://codecov.io/gh/apache/arrow-rs/pull/1342?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
> Merging [#1342](https://codecov.io/gh/apache/arrow-rs/pull/1342?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (333672c) into [master](https://codecov.io/gh/apache/arrow-rs/commit/ecba7dc0830dbde6aa6dd9432519b776e40c1e85?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (ecba7dc) will **not change** coverage.
> The diff coverage is `n/a`.
[](https://codecov.io/gh/apache/arrow-rs/pull/1342?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
```diff
@@ Coverage Diff @@
## master #1342 +/- ##
=======================================
Coverage 83.04% 83.04%
=======================================
Files 181 181
Lines 52937 52937
=======================================
Hits 43960 43960
Misses 8977 8977
```
| [Impacted Files](https://codecov.io/gh/apache/arrow-rs/pull/1342?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [arrow/src/csv/writer.rs](https://codecov.io/gh/apache/arrow-rs/pull/1342/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-YXJyb3cvc3JjL2Nzdi93cml0ZXIucnM=) | `72.13% <ø> (ø)` | |
| [arrow/src/datatypes/datatype.rs](https://codecov.io/gh/apache/arrow-rs/pull/1342/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-YXJyb3cvc3JjL2RhdGF0eXBlcy9kYXRhdHlwZS5ycw==) | `66.40% <0.00%> (-0.40%)` | :arrow_down: |
| [parquet\_derive/src/parquet\_field.rs](https://codecov.io/gh/apache/arrow-rs/pull/1342/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGFycXVldF9kZXJpdmUvc3JjL3BhcnF1ZXRfZmllbGQucnM=) | `66.21% <0.00%> (-0.23%)` | :arrow_down: |
| [arrow/src/array/transform/mod.rs](https://codecov.io/gh/apache/arrow-rs/pull/1342/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-YXJyb3cvc3JjL2FycmF5L3RyYW5zZm9ybS9tb2QucnM=) | `84.65% <0.00%> (+0.13%)` | :arrow_up: |
| [arrow/src/datatypes/field.rs](https://codecov.io/gh/apache/arrow-rs/pull/1342/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-YXJyb3cvc3JjL2RhdGF0eXBlcy9maWVsZC5ycw==) | `54.10% <0.00%> (+0.30%)` | :arrow_up: |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/arrow-rs/pull/1342?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/arrow-rs/pull/1342?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Last update [ecba7dc...333672c](https://codecov.io/gh/apache/arrow-rs/pull/1342?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
--
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-rs] gsserge commented on pull request #1342: Remove delimiter from csv Writer
Posted by GitBox <gi...@apache.org>.
gsserge commented on pull request #1342:
URL: https://github.com/apache/arrow-rs/pull/1342#issuecomment-1046093112
Hm, but what about this line
```rust
let writer = builder.delimiter(delimiter).from_writer(writer);
```
If I understand correctly, `builder` here is from the `csv` crate, and it gets the delimiter configuration.
--
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-rs] gsserge commented on pull request #1342: Remove delimiter from csv Writer
Posted by GitBox <gi...@apache.org>.
gsserge commented on pull request #1342:
URL: https://github.com/apache/arrow-rs/pull/1342#issuecomment-1046090871
```rust
/// Create a new `Writer`
pub fn build<W: Write>(self, writer: W) -> Writer<W> {
let delimiter = self.delimiter.unwrap_or(b','); // looks like it's used
let mut builder = csv_crate::WriterBuilder::new();
let writer = builder.delimiter(delimiter).from_writer(writer);
...
```
--
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-rs] Dandandan commented on pull request #1342: Remove delimiter from csv Writer
Posted by GitBox <gi...@apache.org>.
Dandandan commented on pull request #1342:
URL: https://github.com/apache/arrow-rs/pull/1342#issuecomment-1046093457
> Hm, but what about this line
> ```rust
> let writer = builder.delimiter(delimiter).from_writer(writer);
> ```
> If I understand correctly, `builder` here is from the `csv` crate, and it gets the delimiter configuration.
Ah yeah, you are right 👍 missed that this was the `Writer` from the CSV crate.
Maybe we could add a test for writing a csv with another separator?
--
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-rs] gsserge commented on pull request #1342: Remove delimiter from csv Writer
Posted by GitBox <gi...@apache.org>.
gsserge commented on pull request #1342:
URL: https://github.com/apache/arrow-rs/pull/1342#issuecomment-1046094346
There's already a test for custom options, including the delimiter: https://github.com/apache/arrow-rs/blob/master/arrow/src/csv/writer.rs#L637
--
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