Ensure FilePaths work #511

morris25 · 2019-10-07T20:47:39Z

The CSV.write docstring claims that it should work with FIlePaths

Write a Tables.jl interface input (https://github.com/JuliaData/Tables.jl) to a csv file, given as an IO argument or String/FilePaths.jl type representing the file name to write to.

However, when I try to write to a Path I get

CSV.write(mktmpdir() / "test.txt", DataFrame(:a=>[1,2,3]))
ERROR: MethodError: no method matching with(::getfield(CSV, Symbol("##48#49")){Bool,Tables.Schema{(:a,),Tuple{Int64}},Tables.RowIterator{NamedTuple{(:a,),Tuple{Array{Int64,1}}}},CSV.Options{UInt8,UInt8,Nothing,Tuple{}},Tuple{Symbol},Int64,Int64,Array{UInt8,1}}, ::PosixPath, ::Bool)
Closest candidates are:
  with(::Function, ::Union{DevNull, Pipe, PipeEndpoint, TTY}, ::Any) at /Users/sam/.julia/dev/CSV/src/write.jl:136
  with(::Function, ::IO, ::Any) at /Users/sam/.julia/dev/CSV/src/write.jl:130
  with(::Function, ::String, ::Any) at /Users/sam/.julia/dev/CSV/src/write.jl:140
Stacktrace:
 [1] #write#47(::Bool, ::Bool, ::Array{String,1}, ::Function, ::Tables.Schema{(:a,),Tuple{Int64}}, ::Tables.RowIterator{NamedTuple{(:a,),Tuple{Array{Int64,1}}}}, ::PosixPath, ::CSV.Options{UInt8,UInt8,Nothing,Tuple{}}) at /Users/sam/.julia/dev/CSV/src/write.jl:73
 [2] write(::Tables.Schema{(:a,),Tuple{Int64}}, ::Tables.RowIterator{NamedTuple{(:a,),Tuple{Array{Int64,1}}}}, ::PosixPath, ::CSV.Options{UInt8,UInt8,Nothing,Tuple{}}) at /Users/sam/.julia/dev/CSV/src/write.jl:68
 [3] #write#46(::Char, ::Char, ::Nothing, ::Nothing, ::Char, ::Char, ::Char, ::Nothing, ::Bool, ::String, ::Base.Iterators.Pairs{Union{},Union{},Tuple{},NamedTuple{(),Tuple{}}}, ::typeof(CSV.write), ::PosixPath, ::DataFrame) at /Users/sam/.julia/dev/CSV/src/write.jl:60
 [4] write(::PosixPath, ::DataFrame) at /Users/sam/.julia/dev/CSV/src/write.jl:53
 [5] top-level scope at none:0

These changes should ensure CSV read/write functions work for any AbstractPath.

nickrobinson251

👍

Project.toml

codecov · 2019-10-07T21:22:00Z

Codecov Report

Merging #511 into master will decrease coverage by 0.93%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master     #511      +/-   ##
==========================================
- Coverage   84.52%   83.58%   -0.94%     
==========================================
  Files           7        7              
  Lines        1402     1395       -7     
==========================================
- Hits         1185     1166      -19     
- Misses        217      229      +12

Impacted Files	Coverage Δ
src/CSV.jl	`85.23% <ø> (ø)`	⬆️
src/write.jl	`85.14% <ø> (ø)`	⬆️
src/utils.jl	`74.43% <100%> (-1.57%)`	⬇️
src/rows.jl	`84.15% <0%> (-9.42%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 164888e...a024a72. Read the comment docs.

mattBrzezinski · 2019-10-07T21:27:03Z

Looks good to me, only nice to have would be a test case with your example from the CR description:

CSV.write(mktmpdir() / "test.txt", DataFrame(:a=>[1,2,3]))

quinnj · 2019-10-08T06:02:53Z

src/utils.jl

@@ -174,6 +174,8 @@ function getsource(source, use_mmap)
        return source
    elseif source isa Cmd
        return Base.read(source)
+    elseif source isa AbstractPath
+        return Base.read(Base.open(source))


Hmmm, I don't love this because we're not matching the behavior when source is a String, which I think should be consistent. When source is a String and we have use_mmap, then we Mmap.mmap, otherwise, we make our own mmapped vector and copy the file contents into it. I'd rather rearrange to ensure strings and abstractpath sources are treated the same.

Note that some FilePaths can be mmaps directly, in particular SystemPaths
other cannot, like AWS3.S3Paths
But I guess that can be fairly easily handled by treating SystemPaths like strings,
and all other AbstractPaths like Cmds etc.

rofinn/FilePathsBase.jl#51

SystemPaths don't directly support mmap and have to be opened first. We're also stuck using open on non-System Paths since that guarantees us the right type coming out of read (S3Path right now returns a String).

Also why are we checking slurp(source isa IO ? source : open(String(source))) after the elseif !isa(source, IO) in this function?

oxinabox · 2019-10-08T09:54:32Z

The previous PR to add FilePaths support was
#394

It probably should have added FilePaths as a Test dependendcy, so that we would have been better at not breaking it

src/utils.jl

src/write.jl

Manifest.toml

Project.toml

nickrobinson251 · 2019-10-16T15:04:12Z

test/write.jl

@@ -197,4 +202,9 @@ using CSV, Dates, WeakRefStrings, CategoricalArrays, Tables
    end

    @test read(rd, String) == "col1,col2,col3\n1,4,7\n2,5,8\n3,6,9\n"
+
+    mktmpdir() do tmp


just as a heads up: this will be deprecated to mktempdir in a future FilePathsBase release (rofinn/FilePathsBase.jl@v0.6.2...master). Just mentioning in case we want to bug @rofinn to release that change and increase the lower-bound on FPB compat here

mktmdir() wasn't deprecated, it's just an alias for mktempdir(SystemPath) now.

oxinabox · 2019-10-16T20:24:50Z

src/utils.jl

    elseif !isa(source, IO)
-        m = Mmap.mmap(source)
+        m = Mmap.mmap(source isa AbstractPath ? open(source) : source)


I think this is effectively the same as

Suggested change

m = Mmap.mmap(source isa AbstractPath ? open(source) : source)

m = open(Mmap.mmap, source)

since all Mmap.mmap(source::String) does is basically open(Mmap.mmap, source) anyway

oxinabox · 2019-10-16T20:30:00Z

My alternative dispatch based getsource:
First two cold be combined via a Union

getsource(source::Cmd, ::Any) = Base.read(open(source))
getsource(source::AbstractPath, ::Any) = Base.read(open(source))
getsource(source::Vector{UInt8}, ::Any) = source
getsource(source::IO, ::Any) = slurp(source)
getsource(source::SystemPath, use_mmap) = getsource(string(source), use_mmap)
function getsource(source, use_mmap)
    m = Mmap.mmap(source)
    use_mmap && return m
        
    m2 = Mmap.mmap(Vector{UInt8}, length(m))
    copyto!(m2, 1, m, 1, length(m))
    finalize(m)
    return m2
end

I think this is the same as, and clear than the chain of conditionals

quinnj · 2019-10-16T21:16:52Z

Also, you'll probably need to rebase off current master head/tip to get windows CI fixes for travis.

quinnj · 2019-10-18T20:00:01Z

Bump; is there anything else @morris25 want to do on this? Any concerns @mattBrzezinski or @oxinabox? If this can be rebased, then I think CI should be clean and I'm happy to merge and do a new release.

morris25 · 2019-10-18T20:41:30Z

That should be everything now! Sorry I didn't get back to this sooner

mattBrzezinski

Looks good to me, ship it!

mattBrzezinski · 2019-10-18T20:42:43Z

src/utils.jl

@@ -175,27 +175,25 @@ function slurp(source)
    return final
 end

+getsource(source::Vector{UInt8}, ::Any) = source


I was just typing this solution up as you pushed your commit.

Thank you for making this 100% cleaner and easier to read!! 🚀 💯

nickrobinson251 reviewed Oct 7, 2019

View reviewed changes

Project.toml Outdated Show resolved Hide resolved

quinnj reviewed Oct 8, 2019

View reviewed changes

oxinabox reviewed Oct 8, 2019

View reviewed changes

src/utils.jl Outdated Show resolved Hide resolved

nalimilan reviewed Oct 15, 2019

View reviewed changes

src/write.jl Outdated Show resolved Hide resolved

morris25 force-pushed the sm/filepaths branch from f42d65a to 6665922 Compare October 15, 2019 20:10

quinnj reviewed Oct 16, 2019

View reviewed changes

Manifest.toml Outdated Show resolved Hide resolved

morris25 force-pushed the sm/filepaths branch from ef5b140 to 0b2c95a Compare October 16, 2019 04:31

nickrobinson251 reviewed Oct 16, 2019

View reviewed changes

Project.toml Show resolved Hide resolved

nickrobinson251 reviewed Oct 16, 2019

View reviewed changes

morris25 force-pushed the sm/filepaths branch from 26bff2a to 0bd1896 Compare October 16, 2019 16:23

oxinabox mentioned this pull request Oct 16, 2019

Remove redunctance check in getsource #517

Closed

oxinabox reviewed Oct 16, 2019

View reviewed changes

morris25 added 6 commits October 18, 2019 15:39

Ensure FilePaths work

5738587

Use FilePathsBase instead of FilePaths

851b375

Enables mmap on SystemPaths

de9e552

Adds test for FilePaths with DataFrame

ee778f7

Adds compat for FilePathsBase

863c088

Switches to dispatch-based approach

a024a72

morris25 force-pushed the sm/filepaths branch from 0bd1896 to a024a72 Compare October 18, 2019 20:39

mattBrzezinski approved these changes Oct 18, 2019

View reviewed changes

oxinabox approved these changes Oct 18, 2019

View reviewed changes

quinnj merged commit 745bb25 into JuliaData:master Oct 19, 2019

morris25 deleted the sm/filepaths branch October 19, 2019 15:12

oxinabox mentioned this pull request Jun 9, 2020

Bump [compat] FilePathsBase="0.6,0.7,0.8,0.9" #632

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Ensure FilePaths work #511

Ensure FilePaths work #511

morris25 commented Oct 7, 2019

nickrobinson251 left a comment

codecov bot commented Oct 7, 2019 •

edited

Loading

mattBrzezinski commented Oct 7, 2019

quinnj Oct 8, 2019

oxinabox Oct 8, 2019

morris25 Oct 15, 2019

oxinabox commented Oct 8, 2019

nickrobinson251 Oct 16, 2019 •

edited

Loading

rofinn Oct 16, 2019

nickrobinson251 Oct 16, 2019

oxinabox Oct 16, 2019

oxinabox commented Oct 16, 2019 •

edited

Loading

quinnj commented Oct 16, 2019

quinnj commented Oct 18, 2019

morris25 commented Oct 18, 2019

mattBrzezinski left a comment

mattBrzezinski Oct 18, 2019

	m = Mmap.mmap(source isa AbstractPath ? open(source) : source)
	m = open(Mmap.mmap, source)

Ensure FilePaths work #511

Ensure FilePaths work #511

Conversation

morris25 commented Oct 7, 2019

nickrobinson251 left a comment

Choose a reason for hiding this comment

codecov bot commented Oct 7, 2019 • edited Loading

Codecov Report

mattBrzezinski commented Oct 7, 2019

quinnj Oct 8, 2019

Choose a reason for hiding this comment

oxinabox Oct 8, 2019

Choose a reason for hiding this comment

morris25 Oct 15, 2019

Choose a reason for hiding this comment

oxinabox commented Oct 8, 2019

nickrobinson251 Oct 16, 2019 • edited Loading

Choose a reason for hiding this comment

rofinn Oct 16, 2019

Choose a reason for hiding this comment

nickrobinson251 Oct 16, 2019

Choose a reason for hiding this comment

oxinabox Oct 16, 2019

Choose a reason for hiding this comment

oxinabox commented Oct 16, 2019 • edited Loading

quinnj commented Oct 16, 2019

quinnj commented Oct 18, 2019

morris25 commented Oct 18, 2019

mattBrzezinski left a comment

Choose a reason for hiding this comment

mattBrzezinski Oct 18, 2019

Choose a reason for hiding this comment

codecov bot commented Oct 7, 2019 •

edited

Loading

nickrobinson251 Oct 16, 2019 •

edited

Loading

oxinabox commented Oct 16, 2019 •

edited

Loading