add in for GroupKeys

[bkamins]

Fixes #2305

This is the last thing that was left from that issue. I was not sure if 1 in keys(gdf) should return true or false. On one hand haskey(gdf, 1) is true, but it seems unintuitive. Maybe we should remove haskey support for integers?

[bkamins]

@oxinabox - are there conclusions from custom indexing BOF during JuliaCon2020 for such cases.

To be precise:

1. we have a collection (GroupedDataFrame) that can be indexed by integers and also by values of other type (in this case Tuple, NamedTuple, Dict and GroupKey)
2. we have a GroupKeys object that is returned by keys(::GroupedDataFrame)

The question is:

should in(::Integer, ::GroupKeys) produce always false (this is what we have now), or potentially return true (if the integer is in range), or throw an error (for safety)?

Also maybe there are other recommendations that it would be good to be aware of. Thank you!

[oxinabox]

the fancy indexing BoF did not focus on conclusions, it focused on desires.
Here are the notes from that BoF
https://docs.google.com/document/d/1imBX3k0EEejauWVyXONZDRj8LTr0PeLOJNGEgo6ow1g/edit#heading=h.qrm4q6q56yxm

If we wanted to match the behavour of NamedTuples it seems to me it should return false.
but idk if the behavour of NamedTuples actuals makes sense in the first place, since it has haskey(nt, x) and x in keys(nt) disagreeing.

julia> (;a=1)
(a = 1,)

julia> keys((;a=1))
(:a,)

julia> 1 ∈ keys((;a=1))
false

julia> nt = (;a=10, b=20);

julia> nt[1]
10

julia> 1 in keys(nt)
false

julia> haskey(nt, 1)
true

[bkamins]

Thank you for looking into this. Let us wait what Julia core team says.

[bkamins]

Given the discussion on Slack I understand that in(::Integer, ::GroupKeys) would make sense to return true if index is valid (although this is not what currently Base does). Do you agree with this @oxinabox?

The interpretation is that keys should encompass a concept of "any key", but when iterating keys obviously only one kind of key is iterated over. Is this thinking correct?

[oxinabox]

I think that is correct

[bkamins]

OK - changed!

[nalimilan]

Maybe we should wait and see whether Base does the same change for NamedTuple before merging this?

[bkamins]

I think that before 2.0 it is not going to change in Base as:

julia> x = (a=1,b=2)
(a = 1, b = 2)

julia> keys(x)
(:a, :b)

so as @oxinabox mentioned the type of object returned by keys would have to change and this is a relatively big breakage IMO.

As GroupKeys is our own type I think we are free to do what we think is right and it was agreed that x in keys(y) should be true if y[x] gives you a valid result.

[bkamins]

@nalimilan - so what do you think of this?

[bkamins]

@nalimilan - bump. I think it would be better to make a decision on this before 1.0 release.

[nalimilan]

Sorry, I had totally forgotten this one. I don't have a strong opinion. It seems that whatever we do there will be a small inconsistency between isequal, in and haskey.

[bkamins]

Thank you!

[bkamins]

@nalimilan - we should then additionally decide how == and isequal should work when mixing GroupKey and other types. I think we should not compare as true to vectors or Tuple, but we can consider comparing as isequal (and consequently ==) to NamedTuple. If we decided to go this way then hash of GroupKey must match hash of NamedTuple (which will be a bit expensive, but acceptable). What do you think?

[bkamins]

sorry - this should be in #2639 (comment)