Definition of DictColumns

[bkamins]

@quinnj in:

const DictColumns = AbstractDict{K, V} where {K <: Union{Symbol, String}, V <: AbstractVector}

we require string to be String (and not AbstractString). What is the rationale behind this?

CC @nalimilan @ablaom

[quinnj]

It was a bit arbitrary, no strong reason to not make it AbstractString other than I didn't think/test non-String case specifically.

[bkamins]

In some scenarios users might have SubString (e.g. as a result of calling split on a header string).

[quinnj]

If someone wants to put up a PR, it should be pretty easy to make this change; or I can get around to it in the next few days.

[bkamins]

I can make a PR, but first I would let @nalimilan and @ablaom to comment (especially in relation to design decisions in JuliaAI/ScientificTypes.jl#189).

The crucial question is (as asked some time ago on Slack):

what are the intended use cases of Tables.istable

The point is that as long as users do not assume that if Tables.istable(x) returns false then x is not a table this issue is less pressing. However, e.g. MLJ.jl currently assumes this.

[nalimilan]

Yes I think in general AbstractString should be allowed anywhere String is used.

[ablaom]

I guess expanding to AbstractString is fine. But, again I view this as breaking: istable(X) will return true (on a Julia native type even) where previously it returned false. If there is no intention to tag this as breaking, then I'll wait for a release before finishing up JuliaAI/ScientificTypes.jl#189 .

The point is that as long as users do not assume that if Tables.istable(x) returns false then x is not a table this issue is less pressing. However, e.g. MLJ.jl currently assumes this.

Yes, in ScientificTypes scitype(X) cannot be <:ScientificTypes.Table unless istable(X) == true. I am aware that istable(X) == false does not preclude X implementing the Tables.jl API, and I think this has been documented well. For now we live with this.

[quinnj]

If there is no intention to tag this as breaking

At this point, we're not going to change back. I think overall it's a justifiable change based on previous points: it should have been considered table from the beginning, but was a slight miss in the original Dict support; we reserve the right, like Base, to enable functionality that previously didn't work; and lastly, I've tried to think decently long about the possibilities here, and I really don't foresee something like this happening again. It's only happened here since it involved a Base type where we missed an initial check on a type that should have been supported.

[bkamins]

I agree - the point is that conceptually it was a bug in my opinion.
Unfortunately, since the problem is with type defined in Base Julia and commonly used, we have hit a conflict.

In summary - I understand we are in agreement that the PR fixing the raised issue should be made. If yes, I will make it and bump the package version in it. Then @quinnj will tag the release. OK?

[quinnj]

SGTM