BUG, ENH: Add support for parsing duplicate columns
State: closed

Introduces `mappings` and `reverse_map` attributes to the parser in `pandas.io.parsers` that allow it to differentiate between duplicate columns that may be present in a file.

Closes #7160.
Closes #9424.


Labels:
name: Bug
name: IO CSV
description: read_csv, to_csv

Comments:
you missed my point. don't use a dictionary. use a list for the data and the column names then you don't need all of these crazy gymnastics.

I did try using lists.  I could not get it to work.  The current mechanism is very reliant on using the names and labels in the `dict` object to generate the data and create the `DataFrame` at the end, which two `list` objects is not very _efficient_ or _good_ at doing.  Gymnastics, maybe, but it does manage to resolve the duplicate column names in a parsing system that is entirely unique-column oriented.

@gfyoung you have added a lot of code which is not idomatic, is quite complicated. Refacting should reduce the code not increase it dramatically.

@jreback :  A closer inspection of my changes will indicate that I did not make any major changes, except for moving two functions into a class and adding some references to `mapping` and `reverse_map` to enable **unique** references to column names, even if there are duplicates.

Also, to reiterate what I said above, two `list` objects **did not work** for me.  Not only could I not get it to pass all of the parsing tests, but it is **slow** as you now have to use `list` indexing instead of `dict` look-ups to access and manipulate labels.  You also have to spend time to re-arrange the lists at the end to properly pass it into the `DataFrame`, since AFAICT it only accepts rows, not columns when passing in `list` of `list`.  So yes, there may be some "gymnastics" to get it to work.  However, my changes maintain the overall structure of the current parsing system that, separate from the duplicate columns issue I think works fairly well and efficiently.

In short, it would be more useful to receive specific comments as to what I can do to simplify the code and make it more idiomatic instead of blanket comments like ones previous.

I would do it this way.

```
In [15]: arr = [np.array([1,2,3]),np.array([1.0,1.1,2.0]),np.array(['foo','bar','az']),[4,5,6]]

In [16]: pd.concat(map(Series,arr),axis=1,copy=False)
Out[16]: 
   0    1    2  3
0  1  1.0  foo  4
1  2  1.1  bar  5
2  3  2.0   az  6

In [17]: pd.concat(map(Series,arr),axis=1).dtypes
Out[17]: 
0      int64
1    float64
2     object
3      int64
dtype: object
```

then if you need to assign different names

```
In [23]: result.columns = list('AAAA')

In [24]: result
Out[24]: 
   A    A    A  A
0  1  1.0  foo  4
1  2  1.1  bar  5
2  3  2.0   az  6
```

This might need some profiling, not for speed (as it conceptually does the same think as construction), but for interim memory considerations.

How would this set up handle `parse_dates` for example, where you could be dealing with either `str` names or numerical indices?

@gfyoung pls try to avoid moving around blocks of code for cleaning (unless its germane), rather do that separately. Its very hard to see what you are actually changing.

This is completely orthogonal to date parsing.

1) Noted about the block code moving (to do it in a separate commit).  I moved it because I wanted to use `mapping` and `reverse_map` in the code, and I saw no reason why I should be passing in additional parameters (they are private functions after all).  It also nicely simplifies the signature.

2) I disagree about the orthogonality.  What happens if `parse_dates` needs to use a column whose name happens to be a duplicate?

@gfyoung you are missing the point.

it is orthogonal to construction of the final product. yes of course it needs to deal with names, but these are defined in a list-like structure that it can easily access. The point is there is somewhat clean separation now:
- tokenize (maybe include usecols)
- dtype inference
- possible column combination (e.g. from parse_dates)
- construction

name assignment doesn't happen till the final step (though it can be modified in column combinations)

don't try to link things that are not necessary

As evidenced by the current code, even under the assumption of unique columns, keeping track of the naming changes is not "simple" to do and is quite intertwined with what happens during the processing so that the final construction step is correct.  In your comment, abstracting away the naming works that goes on under "name assignment doesn't happen till the final step (though it can be modified in column combinations)" is misleading IMO.

## [Current coverage](https://codecov.io/gh/pydata/pandas/pull/12935?src=pr) is **83.84%**

> Merging [#12935](https://codecov.io/gh/pydata/pandas/pull/12935?src=pr) into [master](https://codecov.io/gh/pydata/pandas/branch/master?src=pr) will decrease coverage by **0.32%**

``` diff
@@             master   #12935   diff @@
========================================
  Files           138      136     -2   
  Lines         50536    49671   -865   
  Methods           0        0          
  Branches          0        0          
========================================
- Hits          42541    41647   -894   
- Misses         7995     8024    +29   
  Partials          0        0          
```

> Powered by [Codecov](https://codecov.io?src=pr). Last updated by [afde718...78f69c6](https://codecov.io/gh/pydata/pandas/compare/afde7187e22b2013147d0a15911f6ec72e056a43...78f69c6445609e82607ac5ed12759402b8400f23)

@jreback : I have hit a brick wall in terms of the two lists idea,  and worse, I also found that my original changes that didn't handle the empty solution very well (at least in its current form).  Alas...

Re-reading the two issues again, it appears that mangling the duplicate column names with (`mangle_dupe_cols=True`) is acceptable, so I decided to just mangle the names (if passed as the `names` parameter) by default, which was very simple to do.

@gfyoung so your soln is to mange, form the DataFrame, then unmangle?  (if the option is not set)

No unmangling at the end.  The behaviour is exactly as it is when you do this:

``` python
>>> data = 'a,a,b\n1,2,3'  # this already happens
>>> df = read_csv(StringIO(data))
>>> df
  a a.1  b
0 1   2  3
```

except now it will happen as well when you do this:

``` python
>>> data = '1,2,3'  # this behaviour will now match that above
>>> df = read_csv(StringIO(data), names=['a', 'a', 'b'])
>>> df
  a a.1  b
0 1   2  3
```

From what I am reading from the issues, this solution appears to be acceptable.  I've hit a brick wall with regards to un-mangling, as handling the `names` changes in a duplicate-friendly manner as I've mentioned before is not at all straightforward to do AFAICT.

@gfyoung you need to unmangle at the end. The user requested certain names. No reason not to provide them (and its trivial to do this).

What trivial solution did you have in mind?  And I don't consider `df.columns = columns` valid unless you are planning to somehow manipulate `columns` throughout the entire data conversion process as per the current process (or by some other means).

you can simple unmangle at the end. You already _know_ exactly what the names are at the end.

Not quite in the manner I think you are describing it IINM:

``` python
>>> data = '2016-01-01',9:00,5'
>>> read_csv(StringIO(data), names=['day', 'time', 'c'],
             parse_dates={'datetime': ['day', 'time']})
             datetime c
0 2016-01-01 09:00:00 5
```

This is only a "simple" example.  I can keep making things more complicated because I know that with unique names, they will be handled just fine with the current system.

@gfyoung I took a look again. This looks pretty reasonable actually.

pls test with both `mangle_dupe_names=True` (default) and `False` for all added tests.

Further make sure that we have all 3 examples in #7160 

@jreback : The duplicate column handling only works when `mangle_dupe_cols=True`.  Part of my change was to warn users in the documentation about the current dangers of specifying `mangle_dupe_cols=False` because of this issue of data overwriting in the dictionaries.

To allow `mangle_dupe_cols=False` to work with duplicate columns will probably require a significant refactoring of the existing code.  Such a refactoring is probably best saved for later.  In the meantime, at least we can say that we do support duplicate column names unlike before, when we really couldn't.

First example from #7160 already covered in `pandas/test/frame/test_constructors.py`.  The last two examples are covered in my added test.

@jreback : Travis is giving the green light.  Ready to merge if there are no further issues or concerns.

@jreback : Made of the documentation changes as requested.  Ready to merge if there is nothing else.

@gfyoung i think this is all ok, even though it iS a small/slight API change (IOW if you pass duplicate names you now will get mangeled columns).

So I think that if `names` has duplicates and you are mangling (which is the default). A warning might be useful, maybe something like `DuplicateWarning`? 

@jorisvandenbossche thoughts?

@jreback : I'm on the fence for that, since we already mangle by default (with no warning) when the file header contains duplicate names, but we can see what @jorisvandenbossche says regarding that.

Question (apart from the (difficulty of) the implementation): when a user provides explicit `names`, what would we ideally do with duplicates in that list? 
As it is explicitly provided by the user, I would say that we ideally leave it intact, and by default do not mangle them.

However, that is not what this PR is doing. I understand that it is a compromise to mangle the column names, as actually supporting parsing correctly with duplicate column names is not easy to implement? However, the question is, if that would be implemented, would we change the behaviour back (not mangling by default when `names` are provided)? If yes, that seems like a change in behaviour back and forth, and I don't know if that would be a good idea.

Further, I would rather raise a warning when the parsing is actually doing something wrong (wrong data is returned, so in case of `mangle_dupe_cols=False` and duplicate column names), however this could maybe also be a `NotImplementedError`

Also note that this PR does not really add support for parsing duplicate columns, it AFAIU only applies `mangle_dupe_cols=True` to provided `names` as well (in addition to parsed names in the data). Ending up with actual duplicate column names is still not possible (`mangle_dupe_cols=False` is still buggy). So I don't think it fully closes #9424

@jorisvandenbossche :

1) Ideal behaviour is to leave the names intact.  However, as previous discussions have illustrated, that is likely going to take a lot of effort to do and is best saved for a major release, not a minor one.

2) `mangle_dupe_cols=True` by default, so in fact we _should_ be mangling `names` IMO by default if there are duplicates, which is what we already do with file headers that contain duplicates, so this behaviour that I introduce in my PR is here to stay I think.

3) Raising an error when the parser is doing something wrong seems inconsistent with what we do for other features that are not supported.  I guess what we could do is check whether or not `mangle_dupe_cols` is `False` and raise a `ValueError` saying that such an option is not supported because of the data overwriting.  How does that sound?

4) Actually, I believe it does close fully #9424.  The user writes in the issue that mangled columns are considered acceptable, so therefore, his issue is now resolved.  I can open a separate issue to add support for `mangle_dupe_cols=False`.

> 1) Ideal behaviour is to leave the names intact. However, as previous discussions have illustrated, that is likely going to take a lot of effort to do and is best saved for a major release, not a minor one.

I agree with that, but changing the behaviour in a minor release, to change it potentially again in a following major release does also not seem like a good idea. Question is how likely this 'potentially' is.

> 2) mangle_dupe_cols=True by default, so in fact we should be mangling names IMO by default if there are duplicates, which is what we already do with file headers that contain duplicates, so this behaviour that I introduce in my PR is here to stay I think.

This contradicts with what you say in point 1) I think. Any case, I think the "we should be mangling names by default" is stated too strong IMO, as this is not documented is not the current behaviour, so we can make a decision what we think is best.

Regarding 3), that is what I meant. 

Regarding 4), doesn't really matter if it closes it fully or not, opening a new issue for the `mangle_dupe_cols=False` buggy behaviour is perfectly fine as well.

@gfyoung Regarding the actual buggy behaviour when having duplicate column names (which would take more work to refactor), I was thinking of a possible hack as workaround, but don't know the code well enough to see if it makes sense or not:
- in `_maybe_dedup_names`, you can easily store which columns have been renamed (eg in a mapping like `{'colA.1': 'colA'}`)
- we could internally mangle _always_ the duplicate names (to always avoid the overwriting bug)
- in the end (before returning) we could rename the columns based on the saved mapping above if `mangle_dupe_cols=False` and leave them mangled when it is True

That is maybe not the nicest way, but seems not much more additional work in this PR?

@jorisvandenbossche : 

1) and 2) I don't quite follow.  `mangle_dupe_cols` is clearly documented as being defaulted as `True` as of `0.18.1`, so the decision has already been made for us.  My changes make this option extend to the case when `names` contains duplicates, not just when the file header contains duplicates.  These changes would only affect the case when `mangle_dupe_cols=True`, whereas a major release change would involve refactoring the code so that `mangle_dupe_cols=False` becomes viable.

3) Very well.  I will add `mangle_dupe_cols=False` as an unsupported option.  Also would be good to get feedback on this well from @jreback on this.