add view to filter, sort, dropmissing, and unique #2386

bkamins · 2020-08-27T07:35:36Z

This is a non-breaking proposal to add an option to filter, sort, dropmissing, and unique to return a view instead of a copy. This is relevant for very large data frames on which these operations can easily eat up all memory. It is also faster than doing a copy (of course if it is worth doing it depends on what one would want to do with the data frame later).

The drawback is that we become type unstable, but this is a case when we have a small union (DataFrame or SubDataFrame) so compiler should handle it. An alternative with Val seemed to be an overkill (though it would be type stable then).

Now I just have written an implementation and proposed docstring updates. If we think this PR is OK I will add tests and update NEWS.md.

src/abstractdataframe/abstractdataframe.jl

bkamins · 2020-09-01T18:47:30Z

@nalimilan - no rush, but do you have any opinion on this (this is the first step in deciding what we do with updating of views later)

nalimilan

Sounds like a good idea, and I don't see what else this could mean so it's quite safe to add. It's also nice to see that the implementation is quite clean.

Regarding type instability, ideally the compiler will use the fact that view=true is known at compile time (in particular as the default value for a keyword argument) to infer the type as ::DataFrame. I checked that recently for categorical's compress=false argument and it worked quite well. Can you check that it's the case? In some large functions, it could help to move some of the code into helpers, so that the top-level function in which if view appears can be inlined.

src/abstractdataframe/abstractdataframe.jl

src/abstractdataframe/sort.jl

Co-authored-by: Milan Bouchet-Valat <[email protected]>

bkamins · 2020-09-03T07:11:10Z

Actually for categorical we have that categorical([1,2,3]) is correctly inferred, but for categorical([1,2,3], compress=true) and categorical([1,2,3], compress=false) the return type is inferred as Any.

nalimilan · 2020-09-03T07:21:51Z

OK, that's not great, but I guess the compiler cannot do better in that case since there are many possible return types. For view there are only two (concrete) return types. Anyway what matters the most is that when you don't pass view you don't suffer from this.

bkamins · 2020-09-03T07:22:58Z

Regarding view keyword argument I will investigate and let you know. I to not think it is a huge problem (it is a small union and DataFrame is type unstable anyway).

We actually have the same issue with select/transfrom/combine where we have ungroup keyword argument.

src/abstractdataframe/abstractdataframe.jl

src/abstractdataframe/sort.jl

bkamins · 2020-09-03T21:40:39Z

Regarding view keyword argument I will investigate and let you know.

It seems that @inline solves the instability thanks to JuliaLang/julia#35976. I will write appropriate tests tomorrow.

Co-authored-by: Milan Bouchet-Valat <[email protected]>

bkamins · 2020-09-04T09:36:33Z

OK, with @inline on Julia nightly we have a similar situation as for categorical:

julia> df = DataFrame(rand(2,3));

julia> @code_warntype sort(df)
Variables
  #self#::Core.Const(sort, false)
  df::DataFrame

Body::DataFrame
1 ─      nothing
│   %2 = Base.vect()::Vector{Any}
│   %3 = (#self#)(df, %2)::DataFrame
└──      return %3

julia> @code_warntype sort(df, view=true)
Variables
  #s211::Core.Const(Base.var"#sort##kw"(), false)
  @_2::NamedTuple{(:view,),Tuple{Bool}}
  @_3::Core.Const(sort, false)
  df::DataFrame

Body::Union{DataFrame, SubDataFrame{DataFrame,DataFrames.Index,Vector{Int64}}}
1 ─ %1 = Base.vect()::Vector{Any}
│   %2 = (#s211)(@_2, @_3, df, %1)::Union{DataFrame, SubDataFrame{DataFrame,DataFrames.Index,Vector{Int64}}}
└──      return %2

(that is - sometimes we have a proper inference and sometimes we do not unfortunately)

bkamins · 2020-09-04T10:00:52Z

I have added tests (and caught and old bug in filter with SubDataFrames which is fixed now)

bkamins · 2020-09-04T19:36:25Z

@nalimilan - this should be good to have a look at. Thank you!

src/abstractdataframe/abstractdataframe.jl

src/abstractdataframe/sort.jl

nalimilan · 2020-09-05T10:47:56Z

test/data.jl

+        @test fun(view(df, 1:2, 1:2)) isa DataFrame
+        @test fun(df, view=false) isa DataFrame
+        @test fun(view(df, 1:2, 1:2), view=false) isa DataFrame
+        @test fun(df, view=true) isa SubDataFrame
+        @test fun(view(df, 1:2, 1:2), view=true) isa SubDataFrame


Also test returned value? Or is that covered elsewhere?

It would also be nice to use @inferred when view=true/false isn't specified to prevent any regression: it would be easy to remove one of the @inlined without realizing why they are here.

I have added the test and @inferred (I thought it would fail on Julia 1.0, but it passes - which is good)

nalimilan · 2020-09-05T10:49:54Z

(that is - sometimes we have a proper inference and sometimes we do not unfortunately)

It's not just "sometimes", right? If you specify view, the type of the result is a Union, and otherwise it's a concrete type (including if you specify other keyword arguments but not view), right? At least that means this PR doesn't introduce any regression. It's not ideal that view=true implies a type instability, but that shouldn't be too bad and hopefully the compiler will improve in the future.

bkamins · 2020-09-05T21:17:18Z

and otherwise it's a concrete type (including if you specify other keyword arguments but not view)

I had to do some refactoring of filter, but after these changes the answer is "yes".

Co-authored-by: Milan Bouchet-Valat <[email protected]>

nalimilan · 2020-09-07T12:08:58Z

src/abstractdataframe/abstractdataframe.jl

-
-@inline function _filter_helper(df::AbstractDataFrame, f, cols...; view::Bool)
+@inline function Base.filter(f, df::AbstractDataFrame; view::Bool=false)
+    rowidxs::BitVector = _filter_helper(f, eachrow(df))


It's weird that inference fails here. Anyway, maybe better put the type assertion directly in _filter_helper itself?

Actually I have tried this and this was crashing my Julia earlier. But now it seems to work, so probably I had some weird problem earlier. I pushed the fix for this. Also I think it is not a problem to add this conversion and assertion as it is no-op and at least we signal what we intend to return.

OK. Do we have tests to catch an inference failure if somebody removed the assertion?

Now I remember why I did it the other way - it was because nightly fails this way, see e.g. https://travis-ci.org/github/JuliaData/DataFrames.jl/jobs/725236829 (I have not tested this in 100% detail to nail down the reason as I have a lot of classes this week 😩).

@JeffBezanson + @Keno : this is a regression on nightly vs Julia 1.5.1 - some problems with type inference. Please let me know if you need more information to investigate.

Use this branch on Julia nightly and run the following to reproduce (interestingly - a correct result is produced, but an error is thrown in the process):

julia> using DataFrames julia> df = DataFrame(rand(10,1)) 10×1 DataFrame │ Row │ x1 │ │ │ Float64 │ ├─────┼───────────┤ │ 1 │ 0.389567 │ │ 2 │ 0.0901768 │ │ 3 │ 0.179161 │ │ 4 │ 0.0274051 │ │ 5 │ 0.696103 │ │ 6 │ 0.321395 │ │ 7 │ 0.476313 │ │ 8 │ 0.628184 │ │ 9 │ 0.610729 │ │ 10 │ 0.048924 │ julia> filter(:x1 => >(0.5), df) Internal error: encountered unexpected error in runtime: BoundsError(a=Array{UInt64, (68,)}[0x0000000000000001, 0x0000000000000002, 0x0000000000000000, 0x0000000000000003, 0x0000000000000004, 0x0000000000000005, 0x0000000000000006, 0x0000000000000007, 0x0000000000000008, 0x0000000000000009, 0x000000000000000a, 0x000000000000000b, 0x0000000000000000, 0x0000000000000000, 0x0000000000000000, 0x0000000000000000, 0x0000000000000000, 0x0000000000000000, 0x0000000000000000, 0x0000000000000000, 0x0000000000000000, 0x0000000000000000, 0x0000000000000000, 0x0000000000000000, 0x0000000000000000, 0x0000000000000000, 0x0000000000000000, 0x0000000000000000, 0x0000000000000000, 0x0000000000000000, 0x0000000000000000, 0x0000000000000000, 0x0000000000000000, 0x0000000000000000, 0x0000000000000000, 0x0000000000000000, 0x0000000000000000, 0x0000000000000000, 0x000000000000000c, 0x000000000000000d, 0x000000000000000e, 0x0000000000000000, 0x0000000000000000, 0x0000000000000000, 0x000000000000000f, 0x0000000000000010, 0x0000000000000011, 0x0000000000000012, 0x0000000000000013, 0x0000000000000000, 0x0000000000000014, 0x0000000000000015, 0x0000000000000016, 0x0000000000000017, 0x0000000000000000, 0x0000000000000018, 0x0000000000000019, 0x000000000000001a, 0x000000000000001b, 0x0000000000000000, 0x000000000000001c, 0x000000000000001d, 0x000000000000001e, 0x000000000000001f, 0x0000000000000020, 0x0000000000000021, 0x0000000000000022, 0x0000000000000023], i=(69,)) jl_bounds_error_ints at /buildworker/worker/package_linux64/build/src/rtutils.c:183 getindex at ./array.jl:809 [inlined] getindex at ./abstractarray.jl:1122 [inlined] DFS at ./compiler/ssair/domtree.jl:197 SNCA at ./compiler/ssair/domtree.jl:269 construct_domtree at ./compiler/ssair/domtree.jl:121 run_passes at ./compiler/ssair/driver.jl:131 optimize at ./compiler/optimize.jl:172 typeinf at ./compiler/typeinfer.jl:32 typeinf_edge at ./compiler/typeinfer.jl:522 abstract_call_method at ./compiler/abstractinterpretation.jl:453 abstract_call_gf_by_type at ./compiler/abstractinterpretation.jl:129 abstract_call_known at ./compiler/abstractinterpretation.jl:991 abstract_call at ./compiler/abstractinterpretation.jl:1014 abstract_call at ./compiler/abstractinterpretation.jl:998 abstract_eval_statement at ./compiler/abstractinterpretation.jl:1119 typeinf_local at ./compiler/abstractinterpretation.jl:1375 typeinf_nocycle at ./compiler/abstractinterpretation.jl:1431 typeinf at ./compiler/typeinfer.jl:12 typeinf_ext at ./compiler/typeinfer.jl:609 typeinf_ext_toplevel at ./compiler/typeinfer.jl:642 typeinf_ext_toplevel at ./compiler/typeinfer.jl:638 jfptr_typeinf_ext_toplevel_10082.clone_1 at /home/bkamins/Downloads/julia-latest-linux64/julia-371bfa89d4/lib/julia/sys.so (unknown line) jl_apply at /buildworker/worker/package_linux64/build/src/julia.h:1750 [inlined] jl_type_infer at /buildworker/worker/package_linux64/build/src/gf.c:300 jl_generate_fptr at /buildworker/worker/package_linux64/build/src/jitlayers.cpp:293 jl_compile_method_internal at /buildworker/worker/package_linux64/build/src/gf.c:1890 jl_compile_method_internal at /buildworker/worker/package_linux64/build/src/gf.c:1841 [inlined] _jl_invoke at /buildworker/worker/package_linux64/build/src/gf.c:2145 [inlined] jl_apply_generic at /buildworker/worker/package_linux64/build/src/gf.c:2336 jl_apply at /buildworker/worker/package_linux64/build/src/julia.h:1750 [inlined] do_call at /buildworker/worker/package_linux64/build/src/interpreter.c:117 eval_value at /buildworker/worker/package_linux64/build/src/interpreter.c:206 eval_stmt_value at /buildworker/worker/package_linux64/build/src/interpreter.c:157 [inlined] eval_body at /buildworker/worker/package_linux64/build/src/interpreter.c:551 jl_interpret_toplevel_thunk at /buildworker/worker/package_linux64/build/src/interpreter.c:659 top-level scope at REPL[15]:1 jl_toplevel_eval_flex at /buildworker/worker/package_linux64/build/src/toplevel.c:838 jl_toplevel_eval_flex at /buildworker/worker/package_linux64/build/src/toplevel.c:788 jl_toplevel_eval_in at /buildworker/worker/package_linux64/build/src/toplevel.c:881 eval at ./boot.jl:344 eval_user_input at /buildworker/worker/package_linux64/build/usr/share/julia/stdlib/v1.6/REPL/src/REPL.jl:134 repl_backend_loop at /buildworker/worker/package_linux64/build/usr/share/julia/stdlib/v1.6/REPL/src/REPL.jl:195 start_repl_backend at /buildworker/worker/package_linux64/build/usr/share/julia/stdlib/v1.6/REPL/src/REPL.jl:180 #run_repl#41 at /buildworker/worker/package_linux64/build/usr/share/julia/stdlib/v1.6/REPL/src/REPL.jl:311 run_repl at /buildworker/worker/package_linux64/build/usr/share/julia/stdlib/v1.6/REPL/src/REPL.jl:299 #844 at ./client.jl:386 jfptr_YY.844_30029.clone_1 at /home/bkamins/Downloads/julia-latest-linux64/julia-371bfa89d4/lib/julia/sys.so (unknown line) jl_apply at /buildworker/worker/package_linux64/build/src/julia.h:1750 [inlined] do_apply at /buildworker/worker/package_linux64/build/src/builtins.c:655 jl_f__apply_latest at /buildworker/worker/package_linux64/build/src/builtins.c:705 #invokelatest#2 at ./essentials.jl:718 [inlined] invokelatest at ./essentials.jl:717 [inlined] run_main_repl at ./client.jl:371 exec_options at ./client.jl:301 _start at ./client.jl:484 jfptr__start_22499.clone_1 at /home/bkamins/Downloads/julia-latest-linux64/julia-371bfa89d4/lib/julia/sys.so (unknown line) jl_apply at /buildworker/worker/package_linux64/build/ui/../src/julia.h:1750 [inlined] true_main at /buildworker/worker/package_linux64/build/ui/repl.c:106 main at /buildworker/worker/package_linux64/build/ui/repl.c:227 __libc_start_main at /lib/x86_64-linux-gnu/libc.so.6 (unknown line) _start at /home/bkamins/Downloads/julia-latest-linux64/julia-371bfa89d4/bin/julia (unknown line) 3×1 DataFrame │ Row │ x1 │ │ │ Float64 │ ├─────┼──────────┤ │ 1 │ 0.696103 │ │ 2 │ 0.628184 │ │ 3 │ 0.610729 │

Do we have tests to catch an inference failure if somebody removed the assertion?

Yes - tests will catch this then.

test/data.jl

bkamins · 2020-09-08T20:16:07Z

Thank you for approving, but I think let us wait if we get a feedback from core devs on nightly issues.

bkamins · 2020-09-09T06:58:45Z

@nalimilan - added NEWS.md entry. Can you have a quick look please before merging?

bkamins · 2020-09-09T08:51:36Z

Thank you!

add view to filter, sort, dropmissing, and unique

5caad13

bkamins added feature performance non-breaking The proposed change is not breaking labels Aug 27, 2020

bkamins added this to the 1.0 milestone Aug 27, 2020

fix typo

d0db00b

bkamins commented Aug 27, 2020

View reviewed changes

src/abstractdataframe/abstractdataframe.jl Outdated Show resolved Hide resolved

bkamins commented Aug 27, 2020

View reviewed changes

src/abstractdataframe/abstractdataframe.jl Outdated Show resolved Hide resolved

Apply suggestions from code review

045e3c6

bkamins commented Aug 27, 2020

View reviewed changes

src/abstractdataframe/abstractdataframe.jl Outdated Show resolved Hide resolved

Update src/abstractdataframe/abstractdataframe.jl

6ad1f8e

bkamins mentioned this pull request Sep 1, 2020

filter(df, :x => f) would be useful to have #2187

Closed

bkamins mentioned this pull request Sep 1, 2020

Creating new columns on a view should fill in missings everywhere else. #2211

Closed

nalimilan reviewed Sep 2, 2020

View reviewed changes

Apply suggestions from code review

e46dde5

Co-authored-by: Milan Bouchet-Valat <[email protected]>

fix docstrings and change sort implementation

b66b99e

nalimilan reviewed Sep 3, 2020

View reviewed changes

bkamins and others added 4 commits September 3, 2020 23:41

Apply suggestions from code review

3e5da8f

Co-authored-by: Milan Bouchet-Valat <[email protected]>

partially add @inline

5ba57a3

Merge remote-tracking branch 'origin/filter_view' into filter_view

f085987

add inlining

39088db

fix filter and filter! for SubDataFrame

adac2a4

bkamins added the bug label Sep 4, 2020

nalimilan reviewed Sep 5, 2020

View reviewed changes

bkamins and others added 5 commits September 5, 2020 23:17

refactor filter

e5a1dd3

Apply suggestions from code review

2c2c0fa

Co-authored-by: Milan Bouchet-Valat <[email protected]>

Merge remote-tracking branch 'origin/filter_view' into filter_view

344e586

fix indentation, add @inferred, add correctness test

faca250

fix duplicate column name in filter case

3ccc110

nalimilan reviewed Sep 7, 2020

View reviewed changes

move BitVector to helper function

cde5c88

nalimilan approved these changes Sep 8, 2020

View reviewed changes

bkamins mentioned this pull request Sep 8, 2020

Internal BoundsError in the compiler (in getfield_elim_pass!) JuliaLang/julia#37182

Closed

bkamins added 2 commits September 9, 2020 08:55

Merge branch 'master' into filter_view

5e22a87

add NEWS.md entry

860d0c2

nalimilan approved these changes Sep 9, 2020

View reviewed changes

bkamins merged commit 63b5025 into JuliaData:master Sep 9, 2020

bkamins deleted the filter_view branch September 9, 2020 08:51

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add view to filter, sort, dropmissing, and unique #2386

add view to filter, sort, dropmissing, and unique #2386

bkamins commented Aug 27, 2020

bkamins commented Sep 1, 2020

nalimilan left a comment

bkamins commented Sep 3, 2020

nalimilan commented Sep 3, 2020

bkamins commented Sep 3, 2020

bkamins commented Sep 3, 2020

bkamins commented Sep 4, 2020

bkamins commented Sep 4, 2020

bkamins commented Sep 4, 2020

nalimilan Sep 5, 2020

bkamins Sep 5, 2020

nalimilan commented Sep 5, 2020 •

edited

Loading

bkamins commented Sep 5, 2020

nalimilan Sep 7, 2020

bkamins Sep 8, 2020

nalimilan Sep 8, 2020

bkamins Sep 8, 2020

bkamins Sep 8, 2020

bkamins commented Sep 8, 2020

bkamins commented Sep 9, 2020

bkamins commented Sep 9, 2020

add view to filter, sort, dropmissing, and unique #2386

add view to filter, sort, dropmissing, and unique #2386

Conversation

bkamins commented Aug 27, 2020

bkamins commented Sep 1, 2020

nalimilan left a comment

Choose a reason for hiding this comment

bkamins commented Sep 3, 2020

nalimilan commented Sep 3, 2020

bkamins commented Sep 3, 2020

bkamins commented Sep 3, 2020

bkamins commented Sep 4, 2020

bkamins commented Sep 4, 2020

bkamins commented Sep 4, 2020

nalimilan Sep 5, 2020

Choose a reason for hiding this comment

bkamins Sep 5, 2020

Choose a reason for hiding this comment

nalimilan commented Sep 5, 2020 • edited Loading

bkamins commented Sep 5, 2020

nalimilan Sep 7, 2020

Choose a reason for hiding this comment

bkamins Sep 8, 2020

Choose a reason for hiding this comment

nalimilan Sep 8, 2020

Choose a reason for hiding this comment

bkamins Sep 8, 2020

Choose a reason for hiding this comment

bkamins Sep 8, 2020

Choose a reason for hiding this comment

bkamins commented Sep 8, 2020

bkamins commented Sep 9, 2020

bkamins commented Sep 9, 2020

nalimilan commented Sep 5, 2020 •

edited

Loading