Merge nonstring columns
State: closed

- [X] closes https://github.com/pandas-dev/pandas/issues/46885
- [X] [Tests added and passed](https://pandas.pydata.org/pandas-docs/dev/development/contributing_codebase.html#writing-tests) if fixing a bug or adding a new feature
- [x] All [code checks passed](https://pandas.pydata.org/pandas-docs/dev/development/contributing_codebase.html#pre-commit).
- [X] Added an entry in the latest `doc/source/whatsnew/vX.X.X.rst` file if fixing a bug or adding a new feature.


Labels:
name: Enhancement
name: Reshaping
description: Concat, Merge/Join, Stack/Unstack, Explode
name: Needs Discussion
description: Requires discussion from core team before further action
name: Closing Candidate
description: May be closeable, needs more eyeballs

Comments:
Hello @ericman93! Thanks for updating this PR. We checked the lines you've touched for [PEP 8](https://www.python.org/dev/peps/pep-0008) issues, and found:













There are currently no PEP 8 issues detected in this Pull Request. Cheers! :beers: 

##### Comment last updated at 2022-06-13 09:36:13 UTC
Thanks for the PR. Is there an open issue for this?
@WillAyd not that I could find
Thanks for the PR. As mentioned, for changes like this, it's best that this PR as associated with an existing issue since it's unclear the motivation and context for this change.
@mroeschke here is the issue https://github.com/pandas-dev/pandas/issues/46885
@ericman93 reading through your original issue does passing `suffixes=None` not do what you want? 
@WillAyd it is, but passing `suffixes=None` with overlap columns will raise an exception
```
    if not lsuffix and not rsuffix:
        raise ValueError(f"columns overlap but no suffix specified: {to_rename}")
```
I don't think this is best way of doing this, since it might mess with a lot of `types` e.g. `complex`. However, one generic change might be to implement:

```python
if x in to_rename and suffix is not None:
    try:
        return x + suffix
    except TypeError:
        return f"{x}{suffix}"
```

This would then preserve `types` such as string or int, and if you wanted you could overload your own class so that the combination of the suffix through addition was defined:

```python
class Column:
    def __init__(self, name):
        self.name = name
    def __add__(self, arg):
        return Column(name=self.name+arg.name)
```

For example if you index was tuples say `(ix1, ix2)` you could define a suffix `(1,)` and the addition would result in a tuple, `(ix1, ix2, 1)`.
@attack68 I think that trying `x + suffix` will have weird and unwanted behavior with interger columns
why?
ha never mind, you are right
I thought about a scenario where the suffix might be int, so `+` will return the math value of the addition
but suffix is string only
I'll change the code to your suggestion
even when non string suffixes are given and it might add ints that are weird, one could always input a string suffix and this reverts to past behaviour anyway. there might be cases where intger addition could be useful too
don't you think having try-catch will make things slower?
all numeric columns for example will raise an exception and will have worse performance
maybe passing callable as a suffix instead of `renamer` is a better approach?
you can measure this yourself, but it suffices to say the try/except on an int and string catching a type error is 3x slower, but the operation only costs 500ns more. Even if you had 1 million columns this would only be worth 0.5seconds performance degradation. And if you had 1million columns you could supply suffixes with an appropriate format to not raise a `TypeError` (and so avoid any degradation at all) - or do this in a much more efficient way altogether.
@attack68 fixed according to your suggestion
> @WillAyd it is, but passing `suffixes=None` with overlap columns will raise an exception
> 
> ```
>     if not lsuffix and not rsuffix:
>         raise ValueError(f"columns overlap but no suffix specified: {to_rename}")
> ```

IMO allowing None to allow duplicate columns gives more user control than the changes here as stated in the issue https://github.com/pandas-dev/pandas/issues/46885#issue-1217864758 ...

> Regards the duplication, IMO its ok to have 2 identical columns and let the user decide how to handle it by his own

However, for even more user control, it may be worth considering allowing a renamer function or dictionary to be passed to `suffixes` instead.

otherwise agree with https://github.com/pandas-dev/pandas/pull/46879#pullrequestreview-955596300
@simonjayhawkins removing the validation for none suffix is the easiest way to go, but I think I'll change `Suffixes` to have a tuple of `Optional[Union[Callable, str]]`
what do you think?
> @simonjayhawkins removing the validation for none suffix is the easiest way to go,

I think that's also safer otherwise we risk introducing breaking changes for users that work with the current string representation for non-standard column labels.

> but I think I'll change `Suffixes` to have a tuple of `Optional[Union[Callable, str]]` what do you think?

not sure I understand, you don't mean Tuple[Optional[Union[Callable, str]], Optional[Union[Callable, str]]]? 

the docs state state...

> suffixes list-like, default is (“_x”, “_y”)
A length-2 sequence where each element is optionally a string indicating the suffix to add to overlapping column names in left and right respectively. Pass a value of None instead of a string to indicate that the column name from left or right should be left as-is, with no suffix. At least one of the values must not be None.

we can't alter the accepted type but can add additional types as an enhancement

so the current type is defined in `pandas/_typing.py` as `Suffixes = Tuple[Optional[str], Optional[str]]`. This is technically wrong, but does enforce the 2 elements which we can't do with Sequence.

The `Suffixes` alias is used in pd.merge and df.merge.

So the current alias allows `(None, None)`, which is caught by runtime validation. but this could be simply be passed as a single `None` for convienience?
> not sure I understand, you don't mean Tuple[Optional[Union[Callable, str]], Optional[Union[Callable, str]]]?

yes, this is what I meant, sorry

> So the current alias allows (None, None), which is caught by runtime validation. but this could be simply be passed as a single None for convenience?

by convenience you mean to covert `suffixes=None` to `suffxies=(None, None)`?
I think a better approach this to make Suffixes tuple not optional since you can't really pass None
> > not sure I understand, you don't mean Tuple[Optional[Union[Callable, str]], Optional[Union[Callable, str]]]?
> 
> yes, this is what I meant, sorry

ahh, so a separate callable for left and right. Yes, that's probably simpler than passing additional arguments to the callable. so it would just be `Callable[[Hashable], Hashable]`, i.e. takes a label and returns a new label.

> 
> > So the current alias allows (None, None), which is caught by runtime validation. but this could be simply be passed as a single None for convenience?
> 
> by convenience you mean to covert `suffixes=None` to `suffxies=(None, None)`? 

yes

> I think a better approach this to make Suffixes tuple not optional since you can't really pass None

urgh. yes we get `FutureWarning: Passing 'suffixes' as a <class 'NoneType'>, is not supported and may give unexpected results. Provide 'suffixes' as a tuple instead. In the future a 'TypeError' will be raised.` from #34810 so probably can't use `None` on it's own till 2.0
> > I think a better approach this to make Suffixes tuple not optional since you can't really pass None
> 
> urgh. yes we get `FutureWarning: Passing 'suffixes' as a <class 'NoneType'>, is not supported and may give unexpected results. Provide 'suffixes' as a tuple instead. In the future a 'TypeError' will be raised.` from #34810 so probably can't use `None` on it's own till 2.0

forget that, it already raises `TypeError: cannot unpack non-iterable NoneType object` so not sure why we get a `FutureWarning`

so why can't we use `None`?
> so why can't we use None?

because of this code
```
if not lsuffix and not rsuffix:
      raise ValueError(f"columns overlap but no suffix specified: {to_rename}")
```
> > so why can't we use None?
> 
> because of this code
> 
> ```
> if not lsuffix and not rsuffix:
>       raise ValueError(f"columns overlap but no suffix specified: {to_rename}")
> ```

isn't this the validation that we a proposing to remove? if the user doesn't specify `suffixes` the default is `(“_x”, “_y”)`
> isn't this the validation that we a proposing to remove?

I was reading your message the other way around, I thought you meant that accepting nulls isn't safe
I'll remove the validation, do you think I should also add the ability to pass callable as a suffix while I'm at it?
> do you think I should also add the ability to pass callable as a suffix while I'm at it?

could maybe do that as a follow-up, keep this one simple for now?
@simonjayhawkins fixed :)
Thanks @ericman93 for working on this. We have a legacy PR which should have done something similar #25243 and the PR and the associated issue was closed as won't fix #24782. There is also much discussion in #24819 that is related.

Can you review those and ensure that we are not doing anything here that previously was rejected. I think we may need to expand the release note into a dedicated "notable bug fix".

I think https://github.com/pandas-dev/pandas/pull/25243#issuecomment-474191032 can be now be disregarded since this PR is about not mangling non-string column labels, which was not discussed previously.

What I think what may not have been covered here fully yet, is how to accommodate users that merge DataFrames (without pre-checks) and expect it to raise instead of creating duplicate column labels.

also from https://github.com/pandas-dev/pandas/pull/24819#discussion_r253306708 since we are now reconsidering the behavior, cc @pandas-dev/pandas-core (to include participants in previous discussions)
After reading all issues and PRs, I don't think that there is something that was rejected before in my PR
Actually, the writer of https://github.com/pandas-dev/pandas/pull/24819 mentioned that he will open a PR for `suffixes=None` after your comment about allowing it, but I don't see he did

> What I think what may not have been covered here fully yet, is how to accommodate users that merge DataFrames (without pre-checks) and expect it to raise instead of creating duplicate column labels.

In my case, for example, the duplicated columns always have the same values, so I take the first one
But IMO pandas should allow duplicated columns, and it's on the developer to decide what to do in this situation
@jreback its all green
but I want to see what @simonjayhawkins has to say about the default value of the suffixes in `join`
@jreback do we still want to wait for @simonjayhawkins response or should we push it?