Deprecate All

[pdeffebach]

Maybe I'm missing something but there seems to be support in

• DataFrames: Decide if we want to rename All to Cols DataFrames.jl#2203
• DataAPI: Rename All to Union, or to Cols? #16

for deprecating All in favor of Cols.

• Cols(): No columns
• Cols(:) All columns

This is so easy with Cols that there is no need for All().

We should deprecate All.

[bkamins]

Actually Cols(:) is not needed as it is just :.

@nalimilan - are you OK to deprecate All. I am OK with this.

[bkamins]

The issue is that JuliaDB.jl would have to sync with this. CC @piever

[nalimilan]

Yes, this is the last chance to deprecate it (in DataFrames before 1.0).

[piever]

For me to understand, the key idea is that Cols(args...) can take a list of selectors and returns a selector corresponding to the union of the selections. So for JuliaDB to support this, Cols should be implemented the same as All, except for the empty case.

On top of that, select(t, :) should select all columns, that is, : is itself a "special selector". Supporting arbitrary ranges, like select(t, 1:3), would be a bit messy, as they currently have a different meaning, that is select(t, v::AbstractVector) == v). OTOH, just select(t, :) seems reasonable.

In terms of what's breaking though, I would argue that adding this depwarn to DataAPI is breaking, as it would cause a nonsensical warning on older versions of packages that implement All but not yet Cols. Maybe it'd be best to bump DataAPI version when introducing the depwarn, but not when removing All.

[bkamins]

Maybe it'd be best to bump DataAPI version when introducing the depwarn

This is the plan.

So for JuliaDB to support this, Cols should be implemented the same as All, except for the empty case.

Yes

:

You do not have to support it, but it feels natural to allow :, as it is a common pattern in Julia Base and other languages. Colon() is is not AbstractVector.

[piever]

Well, if we do not support it, then Cols(:) (which is the recommended deprecation) would not work out of the box, so that would introduce other special casing / inconsistencies. I actually wondered why the depwarn does not just recommend :.

My only doubt is whether it is sufficiently universal that : means "take all the columns", or if instead we want to keep the empty argument variant All(). This removes the inconsistency:All is no longer the union, it is just all the columns, and select(t, All()) reads reasonably well. On the other hand, it may be redundant.

I don't have a clear view on this, but I can open an issue about it in IndexedTables to see what the current maintainers think (I haven't touched it while).

[bkamins]

I am OK to leave All(), but deprecate All(args...). I will just update the PR in this way.

[nalimilan]

I'd be inclined to wait a bit to see what happens in IndexedTables. It would be simpler to fully deprecate All() if : can replace it. Though if a decision isn't made soon we can still deprecate All(args...) and see later whether we extend the deprecation to All().

[bkamins]

Fortunately this does not affect DataFrames.jl very much 😄.