Skip to content
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

Use DataAPI.rownumber #334

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

Use DataAPI.rownumber #334

wants to merge 3 commits into from

Conversation

VEZY
Copy link

@VEZY VEZY commented Apr 23, 2023

Now using the newly proposed DataAPI.rownumber interface (JuliaData/DataAPI.jl#60). We add an implementation for: Tables.ColumnsRow, Tables.DictRow, Tables.IteratorRow and Tables.MatrixRow.

Note: a similar function (getrow) existed already for ColumnsRow and IteratorRow. These functions were not removed as a precaution, but rather used for implementing DataAPI.rownumber (i.e. rownumber(c::ColumnsRow) = getrow(c))

The implementation for Tables.Row is just rownumber(x::Row) = rownumber(x.x). Maybe we should add a fallback or throw an explicit error if the row type does not implement rownumber ?

@@ -1,7 +1,7 @@
name = "Tables"
uuid = "bd369af6-aec1-5ad0-b16a-f7cc5008161c"
authors = ["quinnj <[email protected]>"]
version = "1.10.1"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also DataAPI.jl version requirement needs to be changed below.

Copy link
Author

@VEZY VEZY Apr 23, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ho yes I forgot this you're right. Done in aefedf6

@bkamins
Copy link
Member

bkamins commented Apr 23, 2023

Can you please add tests for all 5 types?

@VEZY
Copy link
Author

VEZY commented Apr 23, 2023

Hoo, when making the tests I did realise I was completely misinterpreting the row argument, it was not an index, but the actual data of the row... That's why tests are so valuable ^^.

So I don't think we can get the row number without adding it as a field of the rows structures for: Tables.ColumnsRow, Tables.DictRow, Tables.IteratorRow and Tables.MatrixRow.

There's two choices from here, either we add a field for the row index into these structures, or we don't define DataAPI.rownumber for these.

I made an example implementation of a row number in DictRow (5f091b0). If you think that's a good path forward, I'll implement it the same way for the others.

@bkamins
Copy link
Member

bkamins commented Apr 23, 2023

There's two choices from here, either we add a field for the row index into these structures, or we don't define DataAPI.rownumber for these.

Let us wait for @quinnj to decide on this. My thinking is that:

  • it should not add performance overhead
  • having this information can be useful for users

so I would be OK to add it.

The only downside could be that in some cases these objects could be generated from sources that do not have a proper row numbering. However, I am not sure if this is possible. @quinnj should know.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants