add vcat with source; deprecate indicator in joins in favor of source

[bkamins]

Fixes #659

I was prompted by https://discourse.julialang.org/t/would-it-help-to-have-a-tool-that-automatically-determines-which-issues-should-be-closed/56274/16 :).

The PR is relatively simple so we can add it in 1.0 release I think.

[bkamins]

CI error is unrelated and is fixed in #2648

[pdeffebach]

Should this be called indicator for consistency with joins? See here

[bkamins]

Should this be called indicator for consistency with joins?

I am OK with indicator. @nalimilan - what do you think?

[nalimilan]

Consistency with joins sounds like a good reason to use indicator, even though it isn't super explicit. For now these are a bit different, but I imagine at some point we could allow passing a collection of names to *join to choose the values to use, and extend support to more than two data frames, which would make it more similar to this PR.

[nalimilan]

Probably not a big deal, but wouldn't it be more efficient (and not really more complex) to create the column only after concatenating?

[bkamins]

This is a design decision related with an empty data frame. As you can see in tests and examples, if you pass DataFrame() to vcat now a single row is created with all missing values except indicator column. Otherwise we would drop such a data frame.

But maybe dropping it is preferable. What do you think?

[bkamins]

I have thought about it and concluded that it is better to drop it. I will change the implementation.

[nalimilan]

Regarding the API, have you considered something like vcat(:A => df1, :B => df2, :C => df3, indicator=:source) instead? I guess which version is most convenient depends on whether you have all the names in a vector already or whether you write them by hand. We could support both if needed.

[bkamins]

I analyzed it before making the PR.

We could allow:

vcat(:A => df1, :B => df2, :C => df3, indicator=:source)

but I am hesitant to add it as currently:

julia> vcat(:a => DataFrame(a=1), :b => DataFrame(b=2))
2-element Vector{Pair{Symbol, DataFrame}}:
 :a => 1×1 DataFrame
 Row │ a     
     │ Int64 
─────┼───────
   1 │     1
 :b => 1×1 DataFrame
 Row │ b     
     │ Int64 
─────┼───────
   1 │     2

(and this makes sense and is expected)

So adding a kwarg would significantly affect the produced result, which I think is not desirable.

[bkamins]

I imagine at some point we could allow passing a collection of names to *join to choose the values to use, and extend support to more than two data frames, which would make it more similar to this PR.

Not doing this in join was not laziness but the issue that it is just different. If you pass e.g. three columns you can have the following outcomes:

• only 1
• only 2
• only 3
• 1 and 2
• 1 and 3
• 1, 2, and 3

Which would all have to be produced, which I thought would be more confusing than clarifying.

Similarly for two columns passed we have three values (left, right, and both) so it is not the same.

But of course the function of the kwarg is similar (clearly indicate the source data frame) so I would not oppose to syncing it between join and vcat.

Given these considerations what do you think. Should we make them consistent?

[bkamins]

and I will change the kwarg do indicator as suggested.

[nalimilan]

Looks good! I kind of wish we would have used source instead of indicator for joins, as that sounds more explicit, but well...

[bkamins]

I kind of wish we would have used source instead of indicator for joins

Same with me. Maybe we can:

• deprecate indicator in favor of source in joins, but keep it till 2.0 release? It should be non-problematic from performance perspective and would be clearer? (@oxinabox - could you comment as if I recall correctly I used indicator in join because you have mentioned this name at some point - so maybe you have some opinion here)
• in this PR use only source?

[oxinabox]

I have no recollection.
Maybe @rofinn ?

[bkamins]

Sorry for pinging then. Do you have an opinion though 😄?

[rofinn]

I also don't recall this discussion. I'm also in favour of source over indicator.

[pdeffebach]

My only comment is that source should go at the end of the data frame, not at the front.

[bkamins]

My only comment is that source should go at the end of the data frame, not at the front.

OK. I will change it. (it is the place where it goes in joins)

[bkamins]

OK - I have traced the original issue here #1412. Thank you all for responding.

[bkamins]

@nalimilan - I have added a docstring for reduce to make this option more discoverable. Could you please have a look at it before I merge? Thank you!

[bkamins]

Thank you!