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.
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
allow push!/pushfirst!/append!/prepend! with multiple values #3372
allow push!/pushfirst!/append!/prepend! with multiple values #3372
Changes from 4 commits
6c70d9e
1f52a91
849967a
0deac64
caac620
da4f254
0ef997e
cd1c206
b107105
d7f8fff
58c988d
269cc55
c129cff
f213d3f
5f57ac8
a2c4873
d5c7979
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 casecols=:orderequal
makes sense in some cases. That is why I allowed forcols
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.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have updated the code to disallow mixing.
Also I want to check with you the idea that we could allow to write:
instead of current:
as the latter gets problematic for a lot of passed arguments case:
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 withappend!
andprepend!
- do we also want to allow for theByRow
option instead of splatting multiple tables?)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not a fan TBH. If you have a collection of rows, wouldn't it be more logical to use
append!
orprepend!
? Currently you can doappend!(DataFrame(), Tables.dictrowtable(repeat([(;a=1), (;b=2)], 10000)), cols=:union)
, right? It's not super easy to discover, but theByRow
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 aTables.dictrowtable
if needed?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In short: we are looking for equivalent of
Tables.dictrowtable
that would be lazy and allocate less (and it is enough that it supportsTables.AbstractRow
interface, it does not have to be a dictionary.In detail: When you do:
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 createsDict
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:
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: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.cols == :orderequal
then rows must contain the same columns in the same ordercols == :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).cols == :subset
then the behavior is like for:intersect
but if some column is missing in rows then amissing
value is pushed.cols == :union
then column unioning is performedOf 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:compare it to:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note that the upper bound for what is achievable for the example data is:
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 thanTables.dictrowtable
, so I expect that it is possible to get a sub-second performance.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes I was thinking that
append!(..., cols=:union)
would callDataFrame(Tables.column(t, cols=:union))
. It seems enough to haveTables.columns
supportcols=:union
for that.cols=:orderequal
andcols=:setequal
would also make sense but I wouldn't use them inappend!
.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 itcols=:intersect
could be confusing as I would expect it to only retain columns that appear in all rows.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Now I realized that we already have
Tables.dictcolumntable
which is (as expected) much faster:There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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 callDataFrame(Tables.dictcolumntable(t))
whencols=:union
. This should be a good enough solution.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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