REGR: allow merging on object boolean columns
State: closed

Fixes https://github.com/pandas-dev/pandas/issues/21119

This is a quick/hacky fix for the specific case in that issue, i.e. merging boolean / object boolean columns. 

However, I have the feeling we should do a more general solution for cases where object dtyped columns might be actually mergeable. The problem is that inferring the type can be costly? (and eg when having actual strings object columns, this inferring was also unnecessary and thus will unneeded slow down `merge`?)

Labels:
name: Reshaping
description: Concat, Merge/Join, Stack/Unstack, Explode
name: Regression
description: Functionality that used to work in a prior pandas version

Comments:
# [Codecov](https://codecov.io/gh/pandas-dev/pandas/pull/21310?src=pr&el=h1) Report
> :exclamation: No coverage uploaded for pull request base (`master@05e55aa`). [Click here to learn what that means](https://docs.codecov.io/docs/error-reference#section-missing-base-commit).
> The diff coverage is `100%`.

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

```diff
@@            Coverage Diff            @@
##             master   #21310   +/-   ##
=========================================
  Coverage          ?   91.89%           
=========================================
  Files             ?      153           
  Lines             ?    49579           
  Branches          ?        0           
=========================================
  Hits              ?    45559           
  Misses            ?     4020           
  Partials          ?        0
```

| Flag | Coverage Δ | |
|---|---|---|
| #multiple | `90.29% <100%> (?)` | |
| #single | `41.85% <0%> (?)` | |

| [Impacted Files](https://codecov.io/gh/pandas-dev/pandas/pull/21310?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [pandas/core/reshape/merge.py](https://codecov.io/gh/pandas-dev/pandas/pull/21310/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvcmVzaGFwZS9tZXJnZS5weQ==) | `94.25% <100%> (ø)` | |

------

[Continue to review full report at Codecov](https://codecov.io/gh/pandas-dev/pandas/pull/21310?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/21310?src=pr&el=footer). Last update [05e55aa...ad845d7](https://codecov.io/gh/pandas-dev/pandas/pull/21310?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).

this is not hacky it’s inline with what we normally do 

i would add a comment


> this is not hacky it’s inline with what we normally do

What I mean is that it only solves a very specific case, while there might be other cases that pop-up later that might need similar fixes (although that the boolean case probably is the most important one). 

There are other reasons why you could have for some reason a column of `object` dtype, where you could perfectly merge it with another column of properly typed data, was it not for us prematurely erroring on it.

A more 'permissive' solution would be to only error for "clear" errors (eg the int on datetime, or tz-aware with non-tz aware), and leave anything with object columns as it was before (leave it to the merge algorithm to see whether there is something to merge).
@jorisvandenbossche I don't know about that. We are pretty strict and only error on obvious things (e.g. we allow ints to merge regardless of kinds). so ok with this for now.
> only error on obvious things

object dtyped columns are not *that* obvious, they can occur for many reasons. 

Eg in geopandas I had the problem that we created a columns of NaNs that was of object dtype in a specific case (due to a previous merge of an empty and non empty frame (and if you create an empty frame it gets object dtype if you don't specify anything specific), and then merging that again gave this error, while before it perfectly worked, as merging object NaNs on another numerical column is no problem.

Typically those issues are solvable (as my example case), but can be tricky to find, and leads to annoying errors in working code in downstream users before it gets fixed.
For example, we could also have it as warnings instead of errors, to indicate something might be going wrong with your dtypes.
we have been erroring in this for a while
warnings are a backwards step and imho generally ignored anyhow
> we have been erroring in this for a while

I don't think that is correct. As far as I can see, we only started erroring in 0.23.0: https://github.com/pandas-dev/pandas/pull/18674
that was in 0.22
> that was in 0.22

That's weird. The test case works in `0.22.0` but fails in `0.23.0`.
@jreback on the time of merging it was 0.22, but that became 0.23 eventually :) Anyway, so it is only from the last release.
To be more explicit about the the alternative I was thinking about, something like this (using pseudo-code):

```
if (is_object_dtype(left) and not is_object_dtype(right)) or (the other way around):
    warn("You are trying to merge columns of object and {other.dtype} dtype.
          This might indicate an error ...")
    coerce non-object to object (which is what was happening before)
    and continue merging
else:
    ...
    the existing more specific checks for dtypes that are *certainly* incompatible that raise errors (eg in case of merging int on datetime64)
```

that would keep the dubious cases (where one of both columns is object dtype) working but with a warning to indicate to users they need to check their dtypes. But also keeps the informative errors introduced in 0.23.0 for those cases where we are sure it is incompatible.
I don't think warnings are a good idea. if you want to merge this, pls make the changes.
> I don't think warnings are a good idea.

Can you explain why? 
A warning instead of error can give a similar indication of "there is something wrong with your dtypes", but without breaking working code.
because we don't do this for anything else, it is simply out of place here. If you start warning for this, then you need to warn for practiclaly any non-matched but ok to merge errors. that would be so much noise it would be incomprehensible. Either a merge is good or its an error.
> Either a merge is good or its an error.

The thing is that a merge *is* often good (meaning "working as expected", eg this broke a case in geopandas that was perfectly working before), and that it is very difficult for pandas to judge wether a merge is good or not.  
I proposed a warning to at least be somewhat informative that there is something fishy about the types. 
how is object type with not object type not an error? 

note i wouldn’t object to allowing extension types to be a separate clause here (which may solve the issue for geopandas)
> how is object type with not object type not an error?

Because object can be by definition "anything". Whether we regard that as an error or not is something that we can discuss, but it is not that it "should" be an error (it can work, it worked before for certain cases). 

> note i wouldn’t object to allowing extension types to be a separate clause here (which may solve the issue for geopandas)

merging on two extension types will already work if they are equal dtypes. 
But do you mean with the above that you would not error on merging "object + extension type" ?
Could we agree on shipping a minimal solution that fixes the described bug and discuss a broader / more general solution in another issue? I could take over this PR if @jorisvandenbossche wants.
Yes, this will certainly be merged for 0.23.1 so at least the boolean case will be fixed. Updated the PR now (was just waiting with it in case we could get agreement on a more thorough solution).
lgtm @jorisvandenbossche if you want to create an issue to show a warning as suggested above then ok, otherwise merge on pass.
(travis passed)