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

Should Tables.dictrowtable support Tables.columnnames? #344

Open
bkamins opened this issue Sep 19, 2023 · 4 comments
Open

Should Tables.dictrowtable support Tables.columnnames? #344

bkamins opened this issue Sep 19, 2023 · 4 comments

Comments

@bkamins
Copy link
Member

bkamins commented Sep 19, 2023

@quinnj Current behavior is:

julia> Tables.columnnames(Tables.dictrowtable([(a=1, b=2), (a=3, b=4), (a=5, b=6)]))
(:names, :types, :values)
@ablaom
Copy link
Contributor

ablaom commented Sep 19, 2023

I guess the API does not make any promise about Tables.columnnames(X) for X as above because columnaccess(X) == false, right?

But this behaviour will likely catch users by surprise and I don't see any harm in returning X.names instead.

@bkamins
Copy link
Member Author

bkamins commented Sep 19, 2023

Yes, API does not guarantee that Tables.columnnames would work here, but as you say - the returned value is surprising.
In general, the issue is that Tables.columnnames has a default fallback that does not check if passed object follows the contract specified by the API.

In particular the lines:

https://github.com/JuliaData/Tables.jl/blob/main/src/fallbacks.jl#L283
and
https://github.com/JuliaData/Tables.jl/blob/main/src/fallbacks.jl#L286

IMO follow the contract, but I mistakenly wanted to use Tables.columnnames for other tables (like Tables.dictrowtable) and it did not work correctly. That is why I have opened this issue.

@quinnj
Copy link
Member

quinnj commented Sep 20, 2023

Hmmmm, I don't love the idea of defining columnnames for DictRowTable because then if people start relying on it, they'll wonder why other row tables don't also support it? I'd be in favor of adding a definition like columnnames(::DictRowTable) = error(...) so it's explicit that it doesn't support it vs. getting the confusing return value you mentioned before.

@bkamins
Copy link
Member Author

bkamins commented Sep 21, 2023

I think that what is confusing users (and me) is that in the docs it says that Tables.columnnames is required for Tables.AbstractRow and Tables.AbstractColumns, but at the same it is also defined for SOME other types, that are not subtypes of these two, as we can see by calling methods(Tables.columnnames).

So this creates a confusion should this Tables.columnnames usage be restricted only to Tables.AbstractRow and Tables.AbstractColumns or actually it is OK to call it on any object and what it would mean then.

Adding an error for DictRowTable seems OK, but I think that first we need to clarify what are the cases of allowed usage of Tables.columnnames and clearly document it.

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

No branches or pull requests

3 participants