Add a flag to allow the user to specify that if excess columns are fo…

[kev3978]

…und in any given row, they can be ignored (e.g. row is truncated)

[doyaaaaaken]

Hi, @kev3978. Thanks for your pull request!
What is the difference between this ignoreExcessCols option and the skipMissMatchedRow option?
It looks the same to me.

[kev3978]

If you have a csv:

A,B,C
D,E,F,G
H,I,J

Then ignoreExcess cols will result in 3 lines being read:
"A", "B", "C"
"D", "E", "F"
"H", "I", "J"

e.g. the "G" on line 2 will be ignored.

and skipMissMatchedRow will result in 2 lines being read:
"A", "B", "C"
"H", "I", "J"

[doyaaaaaken]

Thank you for your detailed explanation, but the ignoreExcess option is covered by the skipMissMatchedRow option.

How about making the skipMissMatchedRow option deprecated, and introducing a new option differentFieldLengthRow.

The new option can take four values.

1. ERROR (default value) : Throw exception. This behavior is the same as skipMissMatchedRow = false and canonical according to the RFC4180.
2. IGNORE: This behavior is the same as skipMissMatchedRow = true.
3. IGNORE_IF_EXCESS: This behavior is the same as the ignoreExcess option.
4. IGNORE_IF_SHORT: This behavior is opposite to the ignoreExcess option.

[kev3978]

Thank you for your detailed explanation, but the ignoreExcess option is covered by the skipMissMatchedRow option.

I don't think this is correct, for example, if you change the test I made in CsvReaderTest to use "skipMissMatchedRow" flag instead of "ignoreExcess" then the test will fail, since it will ignore the second row rather than cutting it down.

I do like your solution of packaging the behaviour into a single option, however I would suggest the following:

differentLengthFieldRow
ERROR - same as yours
IGNORE - same behaviour as skipMissMatchedRow
TRIM_EXCESS - same behaviour as my current setup in ignoreExcess

I don't think we can handle the situation with a csv like:
column1, column2, column3
A,B,C
D,E
H,I,J

If we try to do the "Ignore if short" behaviour, then we will need to place a null in the second row, to deal with the situation where we use readAllWithHeaderAsSequence

[doyaaaaaken]

I agree with you. 👍

• The IGNORE_IF_SHORT option value which I suggested is not necessary.
• The specification of the differentLengthFieldRowoption which you suggested sounds good.

If it's okay with you, I'd love to have your pull request modified.

[kev3978]

Hi @doyaaaaaken - absolutely, I will alter it later this week.

[kev3978]

@doyaaaaaken I've just thought, there could be a case where the user wants both the TRIM_EXCESS and IGNORE options, e.g. they want to trim the excess columns if possible, or ignore the row if there are fewer than expected. With that in mind, I have reverted back to my original solution and updated the README to reflect that change. I've also added another test case.

[doyaaaaaken]

@kev3978

I've just thought, there could be a case where the user wants both the TRIM_EXCESS and IGNORE options,

Thanks for the great point!

I have reverted back to my original solution and updated the README to reflect that change.

I would very much appreciate it, but I think we should follow a different way.
The ignoreExcessCols option is similar to the skipMissMatchedRow option, so it is confusing to users.

My suggestion is we introduce new insufficientFieldsRow and excessFieldsRow options, and make the skipMissMatchedRow option deprecated.
The insufficientFieldsRow option can take the value of ERROR or IGNORE.
The excessFieldsRow option can take the value of ERROR or IGNORE or TRIM.
These options are simple so the behavior can be understood easily.

[kev3978]

Sounds like a good solution, I will update the PR later this week. 👍

[kev3978]

@doyaaaaaken - I've updated the PR with those changes now.

[doyaaaaaken]

LGTM! Thanks a lot!

[doyaaaaaken]

Released on version 1.3.0 🚀