Fix select/drop on CSV.Rows #866
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
The new column-widening functionality regressed using select/drop keyword args for `CSV.Rows`. May fix #858.
Codecov Report
@@ Coverage Diff @@
## main #866 +/- ##
=======================================
Coverage 90.88% 90.88%
=======================================
Files 9 9
Lines 1898 1898
=======================================
Hits 1725 1725
Misses 173 173
Continue to review full report at Codecov.
|
quinnj
added a commit
that referenced
this pull request
Aug 19, 2021
* Fix select/drop on CSV.Rows The new column-widening functionality regressed using select/drop keyword args for `CSV.Rows`. May fix #858. * fix tests
quinnj
added a commit
that referenced
this pull request
Aug 20, 2021
* lots of updates to manual/docs * fix * Fix select/drop on CSV.Rows (#866) * Fix select/drop on CSV.Rows The new column-widening functionality regressed using select/drop keyword args for `CSV.Rows`. May fix #858. * fix tests * Add automatic support for reading gzipped csv files (#864) * Add automatic support for reading gzipped csv files Also fixes #733 by using a performant method for processing gzipped files. The motivation here is to make this more "automatic" for users so they don't have to fumble around and question whether they should use GZip.jl or CodecZlib.jl or Zlib.jl or whatever. We know which is the fastest (CodecZlib.jl), and it's a mature, properly binary-built package at this point that works cross platform, so I submit that it's a non-risky dependency to take on for the benefit it provides. In terms of implementation, we take the same approach as data.table `fread` here by decompressing the gzip file to a temp file, then mmapping it. We track the temp files generated in this way and clean them up at the end of `CSV.File` (I need to check `CSV.Rows` as well), and we can also rely on having the default of `cleanup=true` for `tempname()`, which means the julia process itself will try and delete all its generated temp files when it exits. Alternative approaches involving reading chunks of a `GzipDecompressorStream` or having a new `BufferedIOVector <: AbstractVector{UInt8}` which would buffer reads from a `GzipDecompressorStream` input were both considered, but at the end of the day, CSV.jl is hard-coded in several places to work on the input as a `Vector{UInt8}` explicitly. Parsers.jl is more flexible, but it would take non-trivial work to allow CSV.jl the same flexibility. Another factor to consider here is that if we allowed many different `buf` types (other than just `Vector{UInt8}`), then we dig up the recompilation problems all over again (that we literally just finished burying with the removal of type parameters from `Parsers.Options`). That's probably the biggest factor for me personally to take the `fread` approach of using tempfiles, in addition to being very performant. So follow ups before merging are: * Check that `CSV.Rows` can clean up tempfiles * We can be more strict in a few places of requiring `Vector{UInt8}` in CSV.jl * Update doc examples to not need `transcode` explicitly * add buffer_in_memory keyword arg and ensure _any_ source that is gzip will be decompressed automatically * use mktemp instead of tempname * make sure we hit buffer_in_memory codepath * lots of updates to manual/docs * add more writing examples
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The new column-widening functionality regressed using select/drop
keyword args for
CSV.Rows. May fix #858.