allow push!/pushfirst!/append!/prepend! with multiple values #3372

bkamins · 2023-08-20T16:46:57Z

Note that, for consistency, I add support for cols in push!/pushfirst! when a collection without column names is pushed.

bkamins · 2023-08-20T16:47:48Z

Follow up of https://discourse.julialang.org/t/construct-dataframe-from-uneven-named-tuples/102970/18 discussion

src/dataframe/insertion.jl

nalimilan · 2023-09-02T16:01:40Z

test/insertion.jl

+              DataFrame(a=[3, 5, 1], b=[4, 6, 2])
+        @test pushfirst!(copy(df), x, y, z) ==
+              DataFrame(a=[3, 5, 7, 1], b=[4, 6, 8, 2])
+        for cols in (:orderequal, :setequal)


Also test cases that error?

I already test them at the very bottom of the new tests (I cannot test them here as in some cases they are allowed - note that these tests test mixing of named and unnamed rows).

I see tests with tuples, but nothing with named tuples or DataFrameRow (which is needed to check that order is correct).

These tests are already done in old tests.

nalimilan · 2023-09-02T16:04:00Z

src/dataframe/insertion.jl

@@ -355,6 +421,10 @@ following way:
  added to `df` (using `missing` for existing rows) and a `missing` value is
  pushed to columns missing in `row` that are present in `df`.

+If `row` is not a `DataFrameRow`, `NamedTuple`, `AbstractDict`, or `Tables.AbstractRow`
+the value of `cols` argument is ignored and it is only allowed to set it to
+`:setequal` or `:orderequal`.


Should we allow :orderequal? I imagine this value is used only when you want to protect yourself from possible inversions of columns, and without names we cannot guarantee that. (Of course :setequal is also a bit weird but we need to allow the default.)

The issue is that when we allow for push!(df, (1,2), (a=1, b=2), cols=:orderequal). That is - we allow for mixing rows with names and without names, in which case cols=:orderequal makes sense in some cases. That is why I allowed for cols in the first place.

If we wanted to disallow this (i.e. disallow mixing named and unnamed containers, which I would also be OK with) then I will redesign the proposal and disallow cols when unnamed containers are passed.

If we wanted to disallow this (i.e. disallow mixing named and unnamed containers, which I would also be OK with) then I will redesign the proposal and disallow cols when unnamed containers are passed.

If that's not too complex, it would be safer to do that, yes. It shouldn't be super common to add several rows of different types at the same time, and people can use two calls if needed.

I have updated the code to disallow mixing.

Also I want to check with you the idea that we could allow to write:

julia> pushfirst!(DataFrame(), ByRow([(;a=1), (;b=2)]), cols=:union) 2×2 DataFrame Row │ b a │ Int64? Int64? ─────┼────────────────── 1 │ missing 1 2 │ 2 missing

instead of current:

pushfirst!(DataFrame(), [(;a=1), (;b=2)]..., cols=:union)

as the latter gets problematic for a lot of passed arguments case:

julia> @time pushfirst!(DataFrame(), ByRow(repeat([(;a=1), (;b=2)], 10000)), cols=:union); 0.009458 seconds (277.47 k allocations: 12.276 MiB) julia> @time pushfirst!(DataFrame(), repeat([(;a=1), (;b=2)], 10000)..., cols=:union); 1.969564 seconds (397.48 k allocations: 5.976 GiB, 16.87% gc time)

This is a bit of overuse of ByRow (which was designed for other purpose), but I found it a natural name. What do you think? (if you think it is OK, then the same decision is with append! and prepend! - do we also want to allow for the ByRow option instead of splatting multiple tables?)

I'm not a fan TBH. If you have a collection of rows, wouldn't it be more logical to use append! or prepend!? Currently you can do append!(DataFrame(), Tables.dictrowtable(repeat([(;a=1), (;b=2)], 10000)), cols=:union), right? It's not super easy to discover, but the ByRow trick isn't either.

Maybe we could simplify this somehow? For example, would there be a way to make append!(DataFrame(), repeat([(;a=1), (;b=2)], 10000), cols=:union) automatically wrap the input vector in a Tables.dictrowtable if needed?

In short: we are looking for equivalent of Tables.dictrowtable that would be lazy and allocate less (and it is enough that it supports Tables.AbstractRow interface, it does not have to be a dictionary.

In detail: When you do:

julia> x = repeat([(;a=1), (;b=2)], 10^6); julia> @time Tables.dictrowtable(x) 4.747105 seconds (42.48 M allocations: 2.623 GiB, 21.35% gc time, 6.76% compilation time) Tables.DictRowTable([:a, :b], Dict{Symbol, Type}(:a => Union{Missing, Int64}, :b => Union{Missing, Int64}), Dict{Symbol, Any}[Dict(:a => 1), Dict(:b => 2), Dict(:a => 1), Dict(:b => 2), Dict(:a => 1), Dict(:b => 2), Dict(:a => 1), Dict(:b => 2), Dict(:a => 1), Dict(:b => 2) … Dict(:a => 1), Dict(:b => 2), Dict(:a => 1), Dict(:b => 2), Dict(:a => 1), Dict(:b => 2), Dict(:a => 1), Dict(:b => 2), Dict(:a => 1), Dict(:b => 2)]) julia> @time Tables.dictrowtable(x) 4.960894 seconds (42.00 M allocations: 2.593 GiB, 30.86% gc time) Tables.DictRowTable([:a, :b], Dict{Symbol, Type}(:a => Union{Missing, Int64}, :b => Union{Missing, Int64}), Dict{Symbol, Any}[Dict(:a => 1), Dict(:b => 2), Dict(:a => 1), Dict(:b => 2), Dict(:a => 1), Dict(:b => 2), Dict(:a => 1), Dict(:b => 2), Dict(:a => 1), Dict(:b => 2) … Dict(:a => 1), Dict(:b => 2), Dict(:a => 1), Dict(:b => 2), Dict(:a => 1), Dict(:b => 2), Dict(:a => 1), Dict(:b => 2), Dict(:a => 1), Dict(:b => 2)])

it is very slow and allocates a lot. The reason is that Tables.DictRowTable has three fields:
:names, :types, and :values. And :values is eager and creates Dict for each entry.

The lazy variant could have :names and :types fields but :values could just point lazily to the source table and later iterate its rows when needed. In particular if source table is columnar creating such an iterator should be super cheap (as we do not even need to iterate it most likely). If the source table has row-wise storage then we would need to iterate it once.

This was the initial idea. In short, to reduce the cost of Tables.dictrowtable.

Now regarding your proposal:

Some modification to Tables.rows or Tables.columns that would do column-unioning if requested?

This is a valid way to implement it and maybe indeed better and sufficient (i.e. we do not have to be lazy as e.g Tables.columns can materialize the columns provided it is efficient).
The point is that if we could pass Tables.columns(x, cols=:union) in the original code this is exactly what is needed.

Then the cols kwarg ideally could have the following values:

If cols == :setequal (this is the default) then rows must contain exactly the same columns (but possibly in a different order), order is defined by the first row.

If cols == :orderequal then rows must contain the same columns in the same order

If cols == :intersect then rows may contain more columns than first row, but all column names that are present in first row must be present in all rows and only they are used to populate a new rows (this is the current behavior).

If cols == :subset then the behavior is like for :intersect but if some column is missing in rows then a missing value is pushed.

If cols == :union then column unioning is performed

Of course we do not have to support all. I think natural options that must be supported are:

:intersect (as this is the current behavior)

:union (this is the most commonly requested extension)

:orderequal (this is what probably people typically expect by default as they even do not know that the current behavior is :intersect)

The reason is that we then could call e.g. DataFrame(Tables.columns(x, cols=:union)) and it would be very fast, while now the operation is super slow:

julia> @time DataFrame(Tables.dictrowtable(x)); 6.498086 seconds (47.27 M allocations: 2.757 GiB, 17.54% gc time, 6.79% compilation time) julia> @time DataFrame(Tables.dictrowtable(x)); 4.213657 seconds (46.00 M allocations: 2.686 GiB, 19.43% gc time)

compare it to:

julia> y = repeat([(;a=1, b=2)], 2*10^6); julia> @time DataFrame(y); 0.102461 seconds (160.85 k allocations: 41.335 MiB, 93.52% compilation time) julia> @time DataFrame(y); 0.008306 seconds (22 allocations: 30.519 MiB)

Note that the upper bound for what is achievable for the example data is:

julia> x = repeat([(;a=1), (;b=2)], 10^6); julia> @time (df = DataFrame(); foreach(row -> push!(df, row, cols=:union), x)) 1.242811 seconds (28.00 M allocations: 1.204 GiB, 1.52% compilation time)

This is still a bit inefficient (as we are adding data to the df row by row, trying to do union each time), but it is already much faster than Tables.dictrowtable, so I expect that it is possible to get a sub-second performance.

Yes I was thinking that append!(..., cols=:union) would call DataFrame(Tables.column(t, cols=:union)). It seems enough to have Tables.columns support cols=:union for that. cols=:orderequal and cols=:setequal would also make sense but I wouldn't use them in append!.

OTC, adding cols=:subset doesn't sound like a good idea to me as it seems weird to me to take the first row as a reference. The fact that the current behavior does that isn't great IMO. At any rate calling it cols=:intersect could be confusing as I would expect it to only retain columns that appear in all rows.

Now I realized that we already have Tables.dictcolumntable which is (as expected) much faster:

julia> @time DataFrame(Tables.dictcolumntable(x)); 1.057388 seconds (10.00 M allocations: 297.543 MiB) julia> @time DataFrame(Tables.dictcolumntable(x)); 1.206029 seconds (10.00 M allocations: 297.543 MiB)

@nalimilan - let us merge this PR (if you are OK with it). Then I will make a separate PR that instead of your proposed DataFrame(Tables.column(t, cols=:union)) will call DataFrame(Tables.dictcolumntable(t)) when cols=:union. This should be a good enough solution.

Co-authored-by: Milan Bouchet-Valat <nalimilan@club.fr>

bkamins · 2023-09-03T11:02:43Z

src/dataframe/insertion.jl

    pushfirst!(df::DataFrame, row::Union{DataFrameRow, NamedTuple, AbstractDict,
                                         Tables.AbstractRow};
               cols::Symbol=:setequal, promote::Bool=(cols in [:union, :subset]))
+    pushfirst!(df::DataFrame, rows...;


Note that I have reverted this part of your update of the docs. The reason is that rows... allows for mixing of the first and second style (named and unnamed rows) in the current design. I could disallow this if you prefer, see the comment https://github.com/JuliaData/DataFrames.jl/pull/3372/files#r1314234174

bkamins · 2023-09-04T20:00:14Z

@nalimilan - if you like the ByRow idea here we could also apply it to vcat, Currently one has to write reduce(vcat, collection_of_data_frames) and users have a hard time discovering and understanding it. Maybe we could add vcat(ByRow(collection_of_data_frames)).

src/dataframe/insertion.jl

nalimilan · 2023-09-08T08:05:55Z

src/dataframe/insertion.jl

@@ -355,6 +421,10 @@ following way:
  added to `df` (using `missing` for existing rows) and a `missing` value is
  pushed to columns missing in `row` that are present in `df`.

+If `row` is not a `DataFrameRow`, `NamedTuple`, `AbstractDict`, or `Tables.AbstractRow`
+the value of `cols` argument is ignored and it is only allowed to set it to
+`:setequal` or `:orderequal`.


I'm not a fan TBH. If you have a collection of rows, wouldn't it be more logical to use append! or prepend!? Currently you can do append!(DataFrame(), Tables.dictrowtable(repeat([(;a=1), (;b=2)], 10000)), cols=:union), right? It's not super easy to discover, but the ByRow trick isn't either.

Maybe we could simplify this somehow? For example, would there be a way to make append!(DataFrame(), repeat([(;a=1), (;b=2)], 10000), cols=:union) automatically wrap the input vector in a Tables.dictrowtable if needed?

Co-authored-by: Milan Bouchet-Valat <nalimilan@club.fr>

…mes.jl into bk/multiinsert

bkamins · 2023-09-26T07:22:39Z

@nalimilan - I have removed the ByRow option. The PR should be good to review now.

bkamins · 2023-10-01T17:54:29Z

bump

src/dataframe/insertion.jl

Co-authored-by: Milan Bouchet-Valat <nalimilan@club.fr>

bkamins · 2023-10-05T20:27:32Z

Thank you!

allow push!/pushfirst!/append!/prepend! with multiple values

6c70d9e

bkamins requested a review from nalimilan August 20, 2023 16:47

bkamins added 3 commits August 20, 2023 18:49

update news and version

1f52a91

improve test coverage

849967a

more tests

0deac64

nalimilan reviewed Sep 2, 2023

View reviewed changes

bkamins and others added 2 commits September 3, 2023 12:53

Apply suggestions from code review

caac620

Co-authored-by: Milan Bouchet-Valat <nalimilan@club.fr>

update after code review

da4f254

bkamins commented Sep 3, 2023

View reviewed changes

bkamins added 2 commits September 3, 2023 23:18

introduce ByRow

0ef997e

fix typo

cd1c206

nalimilan reviewed Sep 8, 2023

View reviewed changes

bkamins and others added 6 commits September 25, 2023 15:07

Apply suggestions from code review

b107105

Co-authored-by: Milan Bouchet-Valat <nalimilan@club.fr>

remove ByRow proposal

d7f8fff

Merge branch 'bk/multiinsert' of https://github.com/JuliaData/DataFra…

58c988d

…mes.jl into bk/multiinsert

small fixes

269cc55

fix docstring

c129cff

Merge branch 'main' into bk/multiinsert

f213d3f

bkamins mentioned this pull request Sep 25, 2023

Allow column unioning in Tables.columns JuliaData/Tables.jl#346

Open

bkamins added 2 commits September 26, 2023 09:20

improve test coverage

5f57ac8

Merge branch 'bk/multiinsert' of https://github.com/JuliaData/DataFra…

a2c4873

…mes.jl into bk/multiinsert

bkamins mentioned this pull request Oct 1, 2023

Column unioning when appending data #3382

Open

nalimilan approved these changes Oct 3, 2023

View reviewed changes

src/dataframe/insertion.jl Outdated Show resolved Hide resolved

src/dataframe/insertion.jl Outdated Show resolved Hide resolved

Apply suggestions from code review

d5c7979

Co-authored-by: Milan Bouchet-Valat <nalimilan@club.fr>

bkamins merged commit 0e2d3df into main Oct 5, 2023
6 of 7 checks passed

bkamins deleted the bk/multiinsert branch October 5, 2023 20:27

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

allow push!/pushfirst!/append!/prepend! with multiple values #3372

allow push!/pushfirst!/append!/prepend! with multiple values #3372

bkamins commented Aug 20, 2023

bkamins commented Aug 20, 2023

nalimilan Sep 2, 2023

bkamins Sep 3, 2023

nalimilan Sep 3, 2023

bkamins Sep 3, 2023

nalimilan Sep 2, 2023

bkamins Sep 3, 2023

nalimilan Sep 3, 2023

bkamins Sep 3, 2023

nalimilan Sep 8, 2023

bkamins Sep 16, 2023 •

edited

Loading

bkamins Sep 16, 2023

nalimilan Sep 24, 2023

bkamins Oct 1, 2023

bkamins Oct 1, 2023 •

edited

Loading

bkamins Sep 3, 2023

bkamins commented Sep 4, 2023

nalimilan Sep 8, 2023

bkamins commented Sep 26, 2023

bkamins commented Oct 1, 2023

bkamins commented Oct 5, 2023

allow push!/pushfirst!/append!/prepend! with multiple values #3372

allow push!/pushfirst!/append!/prepend! with multiple values #3372

Conversation

bkamins commented Aug 20, 2023

bkamins commented Aug 20, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bkamins Sep 16, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bkamins Oct 1, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bkamins commented Sep 4, 2023

Choose a reason for hiding this comment

bkamins commented Sep 26, 2023

bkamins commented Oct 1, 2023

bkamins commented Oct 5, 2023

bkamins Sep 16, 2023 •

edited

Loading

bkamins Oct 1, 2023 •

edited

Loading