Correctly handle Tables.AbstractRow in operation specficiation #3348
Conversation
|
This is mildly breaking, but I assume it is OK to add it in 1.6 release. The point is that I assume that when someone uses |
| @@ -770,6 +773,15 @@ function _add_multicol_res(res::DataFrameRow, newdf::DataFrame, df::AbstractData | |||
| _insert_row_multicolumn(newdf, df, allow_resizing_newdf, colnames, res) | |||
| end | |||
|
|
|||
| function _add_multicol_res(res::Tables.AbstractRow, newdf::DataFrame, df::AbstractDataFrame, | |||
There was a problem hiding this comment.
While we're at it, maybe we should make DataFrameRow <: AbstractRow? That would avoid duplicating a few methods and simplifying type unions.
There was a problem hiding this comment.
I was thinking about it.
First (for future readers) DataFrameRow fully supports Tables.AbstractRow interface, so this is just an issue of code design.
Pros of doing subtyping:
- Less code.
- It is more clear which functionalities are on more "abstract level".
Cons of doing subtyping:
- Most of methods for
DataFrameRowandTables.AbstractRoware the same. However, not all of them. Some methods are different, becauseDataFrameRowhas a richer functionality thanTables.AbstractRow. The challenge is that keepingDataFrameRowandTables.AbstractRowseparate makes it easier (at least for me) in the future to find all places in the source code whereDataFrameRowis used. I know this is not a super strong reason but with the size of the code that we have I often end up doing updates of code by running "find in all files" of a certain code pattern (as otherwise it is easy to forget about some place when some functionality is used).
(for a reference: I started implementing this change and noticed that it would affect more code than only this PR and after this change)
So - we could add it, but it also has some practical downsides. What do you think, given this, we should do?
There was a problem hiding this comment.
I don't have a strong opinion. Given that Tables.jl uses duck typing anyway it's not super important to have DataFrameRow <: AbstractRow, and indeed nobody has requested it.
There was a problem hiding this comment.
Yes - duck typing is my main reason for sticking with what we have.
| @@ -2939,4 +2939,82 @@ end | |||
| end | |||
| end | |||
|
|
|||
| @testset "Tables.AbstractRow interface" begin | |||
There was a problem hiding this comment.
Maybe this should also be tested in other tests where we cover DataFrameRow?
There was a problem hiding this comment.
I have added more such tests (where I managed to track them down).
Co-authored-by: Milan Bouchet-Valat <nalimilan@club.fr>
|
The 32-bit failure seems unrelated but real? |
|
Thank you (I will fix the 32-bit error in a separate PR) |
Fixes #3335
After this PR
Tables.AbstractRowis treated in the same way asDataFrames.DataFrameRowin allcombine/select/transformoperations.