Support merging DataFrames on a combo of columns and index levels (GH 14355)
State: closed

This PR implements the changes proposed and discussed with @jorisvandenbossche @shoyer @TomAugspurger @jreback in #14355.

These changes allow the `on`, `left_on`, and `right_on` parameters of `DataFrame.merge` to accept a combination of column names and index level names. Any common index levels that are merged on are preserved as index levels in the resulting DataFrame, while all other index levels are removed.

In the case of a conflict, the column takes precedence and an ambiguity `FutureWarning` is raised.

- [x] closes #14355
- [x] tests added / passed
- [x] passes `git diff upstream/master -u -- "*.py" | flake8 --diff`
- [x] documentation updated
- [x] whatsnew entry

**Note:** The new `df._get_column_or_level_values` method introduced in this PR is the same method introduced in #17361 to support sorting DataFrames on a combination of columns and index levels. I will keep this method in sync between the two PRs during the review process.


Labels:
name: Enhancement
name: API Design
name: Compat
description: pandas objects compatability with Numpy or Python functions

Comments:
# [Codecov](https://codecov.io/gh/pandas-dev/pandas/pull/17484?src=pr&el=h1) Report
> Merging [#17484](https://codecov.io/gh/pandas-dev/pandas/pull/17484?src=pr&el=desc) into [master](https://codecov.io/gh/pandas-dev/pandas/commit/c63435207375b172db9efdd0d31f32e0da555a72?src=pr&el=desc) will **increase** coverage by `0.09%`.
> The diff coverage is `100%`.

[![Impacted file tree graph](https://codecov.io/gh/pandas-dev/pandas/pull/17484/graphs/tree.svg?width=650&height=150&token=eZ4WkYLtcO&src=pr)](https://codecov.io/gh/pandas-dev/pandas/pull/17484?src=pr&el=tree)

```diff
@@            Coverage Diff             @@
##           master   #17484      +/-   ##
==========================================
+ Coverage   91.35%   91.45%   +0.09%     
==========================================
  Files         163      157       -6     
  Lines       49691    51451    +1760     
==========================================
+ Hits        45397    47052    +1655     
- Misses       4294     4399     +105
```

| Flag | Coverage Δ | |
|---|---|---|
| #multiple | `89.32% <100%> (+0.18%)` | :arrow_up: |
| #single | `40.53% <11.11%> (+0.79%)` | :arrow_up: |

| [Impacted Files](https://codecov.io/gh/pandas-dev/pandas/pull/17484?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [pandas/core/groupby.py](https://codecov.io/gh/pandas-dev/pandas/pull/17484/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvZ3JvdXBieS5weQ==) | `92.04% <100%> (+0.01%)` | :arrow_up: |
| [pandas/core/frame.py](https://codecov.io/gh/pandas-dev/pandas/pull/17484/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvZnJhbWUucHk=) | `97.81% <100%> (-0.09%)` | :arrow_down: |
| [pandas/core/reshape/merge.py](https://codecov.io/gh/pandas-dev/pandas/pull/17484/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvcmVzaGFwZS9tZXJnZS5weQ==) | `94.4% <100%> (+0.14%)` | :arrow_up: |
| [pandas/core/generic.py](https://codecov.io/gh/pandas-dev/pandas/pull/17484/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvZ2VuZXJpYy5weQ==) | `95.9% <100%> (+0.17%)` | :arrow_up: |
| [pandas/io/gbq.py](https://codecov.io/gh/pandas-dev/pandas/pull/17484/diff?src=pr&el=tree#diff-cGFuZGFzL2lvL2dicS5weQ==) | `25% <0%> (-58.34%)` | :arrow_down: |
| [pandas/io/clipboard/\_\_init\_\_.py](https://codecov.io/gh/pandas-dev/pandas/pull/17484/diff?src=pr&el=tree#diff-cGFuZGFzL2lvL2NsaXBib2FyZC9fX2luaXRfXy5weQ==) | `56.86% <0%> (-19.33%)` | :arrow_down: |
| [pandas/util/testing.py](https://codecov.io/gh/pandas-dev/pandas/pull/17484/diff?src=pr&el=tree#diff-cGFuZGFzL3V0aWwvdGVzdGluZy5weQ==) | `81.89% <0%> (-18.11%)` | :arrow_down: |
| [pandas/io/json/json.py](https://codecov.io/gh/pandas-dev/pandas/pull/17484/diff?src=pr&el=tree#diff-cGFuZGFzL2lvL2pzb24vanNvbi5weQ==) | `91.75% <0%> (-8.25%)` | :arrow_down: |
| [pandas/io/formats/style.py](https://codecov.io/gh/pandas-dev/pandas/pull/17484/diff?src=pr&el=tree#diff-cGFuZGFzL2lvL2Zvcm1hdHMvc3R5bGUucHk=) | `96.21% <0%> (-3.79%)` | :arrow_down: |
| [pandas/core/dtypes/missing.py](https://codecov.io/gh/pandas-dev/pandas/pull/17484/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvZHR5cGVzL21pc3NpbmcucHk=) | `90.24% <0%> (-0.5%)` | :arrow_down: |
| ... and [37 more](https://codecov.io/gh/pandas-dev/pandas/pull/17484/diff?src=pr&el=tree-more) | |

------

[Continue to review full report at Codecov](https://codecov.io/gh/pandas-dev/pandas/pull/17484?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/pandas-dev/pandas/pull/17484?src=pr&el=footer). Last update [c634352...f3b95fe](https://codecov.io/gh/pandas-dev/pandas/pull/17484?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).

Thanks for the quick feedback @gfyoung. I think I've incorporated all of it in this last commit. Let me know if I've missed anything or if there are additional changes you'd like to see.
We may need a common framework here to incorporate the ``.groupby`` functionaility of the same name. Adding this many code branches is making things pretty complex.
Thanks for the feedback @jreback. I'll address your comments and take another stab at driving down the complexity. If you, or anyone else, have some ideas for a more unified way to go about this (mixing columns and index levels in merge, sort_values, groupby, etc) I'd be happy to take a step back and consider a change of course. 
Ok, after meditating on this section of the code again I think I found a simpler approach.  I created a new set of DataFrame helpers to support this change and I refactored the existing groupby column/index logic (#14432) to use these as well. I haven't revisited my ``sort_values`` updates in #17361 yet, but I should be able to rely on these same helpers there too. 

Does this general approach look reasonable @jreback, @jorisvandenbossche, @gfyoung, and @TomAugspurger?  Thanks!
Just resolved conflicts with master.  @jreback do you think my updates since your review last month are heading in the right direction?  I'm happy to keep iterating as you have time to provide feedback. 

@jorisvandenbossche it's been a while since we first talked about these changes. Does this approach look alright to you?

Thanks!
I'm having trouble interpreting the circleci failure above. Is this something my changes caused?
@jmmease : The Circle CI builds just died...let's give it another run to see if it passes.
Thanks, @gfyoung 
Ping @jreback for review for @jmmease .
Thanks for the comments @jreback, I'll get to them soon.
Checks green. Ready for review @jreback @gfyoung 
Resolved whatsnew conflict.  At this point should we wait until post-0.21 for this? I can make whatever changes are needed tonight and this weekend if we can still get it in 0.21. If not I'll just remerge and ping after the 0.21 release (Will that be 0.22 or 1.0?). Thanks!
@jreback @TomAugspurger @jorisvandenbossche 
yes, this will wait for post 0.21.0. going to be 0.22 (but no doc-notes yet), so hold off
Sounds good, thanks
Moved whatsnew entry to 0.22. This is ready for continued review as you all have time. Thanks.
@jreback @gfyoung @jorisvandenbossche
@jmmease : Can you rebase onto `master` to get all builds into the green?
I just merged master.  I merged instead of rebased for review continuity, but let me know if you'd prefer a rebase. Thanks.
> I just merged master. I merged instead of rebased for review continuity, but let me know if you'd prefer a rebase. Thanks.

that's fine, in reality since we rebase & squash on merge anyhow it doesn't make much difference.
Thanks, @jreback for the helpful feedback.  I believed I have addressed all of your comments, but let me know if I've missed anything or if there are other changes you'd like to see.
i merged the change which adds that generic test dir
so rebase when u can
merged and pushed.
Just wanted to follow up on this. Do you think we'll be able to get this in for 0.22?
Thanks for all of the feedback already provided. @jreback @gfyoung @jorisvandenbossche 
@jmmease lgtm. sorry, been flooded. pinging is good way to get attention!

@jorisvandenbossche @TomAugspurger 

I think we could put all of the added code that you put in generic.py in *another* module. maybe references and make it into a mix-in class (that you just include into generic).

?
> I think we could put all of the added code that you put in generic.py in another module. maybe references and make it into a mix-in class (that you just include into generic).

actually this is ok for now. will think splitting generic.py up (in another issue).
No problem @jreback , thanks for all you're doing to keep pandas going strong!

Just let me know if you would like something moved before merging.
Just merged to fix whatsnew conflict.  Is this all set @jreback?
@jmmease small linting error (we just added this, it now looks for list-comprehensions that are not generators) :>

and want @jorisvandenbossche @TomAugspurger to have a look.
Ok, I think I fixed the lint issues. Thanks for pointing me in the right direction @jreback 

I'll stand-by for any feedback from @jorisvandenbossche and @TomAugspurger 
Thanks for looking things over @jorisvandenbossche. I'll address the specific comments in an update soon but wanted to follow up on a few of your general points.

1) Yes, I agree that it makes sense for ambiguous index/column combinations to eventually fail in the same way that duplicate columns now fail.  Let me know if there's any language in the `FutureWarning` or in my comments in the code that you'd like me to change to make this future intention clearer.

2) I'm planning to use the `axis=1` case for a couple of these new methods in my next update to #17361 (The `sort_values` analog of this PR) since `sort_values` supports that `axis` parameter.  My hope was that these methods would make it easier to add mixed column/index support to other operations in the future, though `sort_values` is the only remaining operation on my personal wishlist at the moment.

3) I restricted index level references to strings at the moment partially because I wasn't clear on how to check for the full set of valid label/level reference types.  I'm happy to open this up if there's a clear way to check this.

4) I'll need to think some more about `join`. Does anyone else have thoughts on this?