Document duplicate header behavior

[SethFalco]

A while back we discussed duplicate header behavior, and in the comments thought we should define what happens when one tries to get from a CSVRecord/Map<String, String> when this occurs.

Relevant comments:

• CSV-264: Added DuplicateHeaderMode for flexibility with header strictness. #114 (comment)
• CSV-264: Added DuplicateHeaderMode for flexibility with header strictness. #114 (comment)

I reviewed the code and manually tested to discover the current behavior:

• Adds two test cases that reinforce the current behavior.
• Updates the docstrings of CSVRecord#get and CSVRecord#toMap to state how this is handled.

---

It may be that instead of this, we may want to instead define a different behavior before we add documentation and tests to cover this case.

I'm honestly not sure what the expected behavior would be for this. I'm tempted to say that it should throw an exception in this instance. 🤔

I'm not aware of the use-case for having duplicate headings in the first place, but I assume there is a reason for it. But I definitely don't see why one would use methods like #get or #toMap in cases that they know they have duplicate headers.

Maybe if the first and last index of the given header name differs, throw an exception?

[codecov-commenter]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 97.87%. Comparing base (420cd15) to head (7f33501).
Report is 491 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff            @@
##             master     #309   +/-   ##
=========================================
  Coverage     97.87%   97.87%           
  Complexity      549      549           
=========================================
  Files            11       11           
  Lines          1178     1178           
  Branches        204      204           
=========================================
  Hits           1153     1153           
  Misses           13       13           
  Partials         12       12           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[kinow]

Hmmm, tricky. I think documenting the behavior sounds about right for now.

My go-to tool for parsing CSV ATM is Python. The stdlib csv module has the same behavior documented here, displaying the last occurrence.

>>> import csv
>>> 
>>> with open('/tmp/test.csv') as f:
...   reader = csv.reader(f, delimiter=',')
...   for row in reader:
...     print(', '.join(row))
... 
A, A, B, B
1, 2, 5, 6
>>>
>>> with open('/tmp/test.csv') as f:
...   reader = csv.DictReader(f)
...   dict_from_csv = dict(list(reader)[0])
...   print(', '.join(list(dict_from_csv.keys())))
... 
A, B
>>>
>>> dict_from_csv
{'A': '2', 'B': '6'}

But whenever I need some serious CSV processing, then I use Pandas. With Pandas it automatically deduplicates the column names. Maybe that's a feature that we could have in Commons CSV too?

>>> import pandas as pd
>>> 
>>> df = pd.read_csv('/tmp/test.csv')
>>> 
>>> ','.join(list(df.columns))
'A,A.1,B,B.1'
>>> 
>>> df['A']
0    1
Name: A, dtype: int64
>>> df['A.1']
0    2
Name: A.1, dtype: int64
>>> 

Cheers,
-Bruno

[SethFalco]

I'm up for anything. I'll still give my thoughts on the methods above, though.

The stdlib csv module has the same behavior documented here, displaying the last occurrence.

I think just doing the last occurrence is very likely to lead to scenarios where:

1. User implements something with this library
2. Runs into unexpected results due to duplicate header names
3. Code in a workaround to handle this situation as they feel best fits in with their application, i.e.:
   • Disallow duplicate header names
   • Use a different method to iterate through columns
   • Deduplicate the headings themselves before providing it to Commons CSV.

I think throwing an exception makes more sense, so developers can skip to step 3. 🤔

With Pandas it automatically deduplicates the column names

This crossed my mind, but I was hesitant to modify data passed to the lib, at least not implicitly.

I'm also wary that this may screw up existing projects that depend on allowing/disallowing duplicates. i.e. want to allow duplicates and handle things through indexes / iteration, so this didn't cause a problem for them and want to preserve header names, and so don't need the headers deduplicated.

If we want this in the library, maybe one of the following ideas would work, though?

HeaderStrategy Interface

Contains two functions:

• #normalizeHeaders(headings) - With given heading, output a list that fits with whatever the strategy is going for.
• #get(record, header) - Fetch value(s) based on given column name.

#normalizeHeaders function would be used during parsing. Implementations may choose not to do any normalization if they don't need headings renamed.

This would allow things like deduplicating headings. I think it's best to separate normalization from fetching as implementations like this may want to display a warning in #get if the user tries to get one of the original header names. (i.e. A,A → A.1,A.2 but user does #get("A"). People may be confused why it didn't work when it was indeed one of the original headings.)

The #get method would be used to get the value from the record. A strategy might be the:

• last occurrence
• the last occurrence with a non-empty value
• all values joined with a comma (,) or semicolon (;)

We can define one based on what we think should be default behavior. This would reduce maintenance burden if users have different ideas on how this should be handled so we don't have to support all use-cases ourselves. Then users can override it with whatever they want.

Repurpose DuplicateHeaderMode

We could introduce modes like Deduplicate. Unlike above, this doesn't let users implement their own strategies, they'd be stuck with our enum, which also means we shoulder the responsibility to implement whatever use-cases others have.

[garydgregory]

Hi All,

I'll start a thread on the ML for #309 (comment) and #309 (review)