`fread` fill=true and sep= provided could still read as 1-column by mattdowle · Pull Request #2755 · Rdatatable/data.table

mattdowle · 2018-04-17T00:55:12Z

The root cause was the fallback to single-column input that happened in all cases.

reorganized sep/quoteRule detection into different sections depending on whether fill=true/false and whether ncol==1 or >1. Different rules in different situations should be easier now. The numLines and numFields VLAs are no longer needed and there isn't a second step to find the first line again (to avoid inconsistencies there).
added warning when healing quoteRule is selected in jump 0 sample. (There was already a warning when healing quoteRule is chosen out-of-sample.) Any invalid files that are auto-healed should produce warning.
fill=true now uses the longest line from the jump 0 sample (which will most often be the column names on row 1), so long as the first row has more than one field with that sep/quoteRule. As before, no auto-skip when fill=true.

codecov-io · 2018-04-17T01:06:14Z

Codecov Report

Merging #2755 into master will increase coverage by 0.01%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #2755      +/-   ##
==========================================
+ Coverage   93.45%   93.46%   +0.01%     
==========================================
  Files          61       61              
  Lines       12323    12358      +35     
==========================================
+ Hits        11516    11551      +35     
  Misses        807      807

Impacted Files	Coverage Δ
R/test.data.table.R	`100% <100%> (ø)`	⬆️
src/fread.c	`98.03% <100%> (+0.05%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9d0de24...7704ec6. Read the comment docs.

st-pasha

It seems that now quote rules 2 and 3 are considered "improper" , and a warning will be issued whenever one of these rules is employed.

This is something we should be more upfront with in PR description, and in the Changelog. Note that this could be a breaking change for people who run fread under "warnings are errors" mode. Though I agree that QR=2 feels kinda like a bandaid, I'm not so sure about QR=3. After all, if a user chooses an uncommon sep (e.g. \x01), then it may be perfectly normal not to quote any fields regardless of whether they contain any quote characters or not.

Also I couldn't understand what does "self healing quote rule" mean, which was mentioned several times in comments.

Overall, I liked the reorg of the sep-detection logic: having fill=True case considered separately makes things less convoluted and more robust.

st-pasha · 2018-04-17T17:10:52Z

@@ -3959,11 +3959,14 @@ test(1215,
   data.table(N_ID=175931L, VISIT_DATE="2013-03-08T23:40:30", REQ_URL='http://aaa.com/rest/api2.do?api=getSetMobileSession&data={"imei":"60893ZTE-CN13cd","appkey":"android_client","content":"Z0JiRA0qPFtWM3BYVltmcx5MWF9ZS0YLdW1ydXoqPycuJS8idXdlY3R0TGBtU', REQType=2L)
 )
 test(1216.1, fread('A,B,C\n1.2,Foo"Bar,"a"b\"c"d"\nfo"o,bar,"b,az""\n'),


This looks confusing: do you mean to have
'A,B,C\n1.2,Foo"Bar,"a"b"c"d"\nfo"o,bar,"b,az""\n'
or
'A,B,C\n1.2,Foo"Bar,"a"b\\"c"d"\nfo"o,bar,"b,az""\n' ?

Good point. I'm not sure what is meant by that extra \ in that test before the \"c. It's superfluous since opening the string ' automatically escapes any " inside the string. I checked that as follows :

> identical('A,B,C\n1.2,Foo"Bar,"a"b"c"d"\nfo"o,bar,"b,az""\n', + 'A,B,C\n1.2,Foo"Bar,"a"b\"c"d"\nfo"o,bar,"b,az""\n') [1] TRUE

So out of the two choices you present, the first one is what it does. Maybe the second one is what was intended.

I'll add them all in, with a comment alongside.

mattdowle · 2018-04-17T22:38:04Z

It seems that now quote rules 2 and 3 are considered "improper" , and a warning will be issued whenever one of these rules is employed.

Yes. Other csv readers halt with 'file invalid' message. We should at least have a warning.

This is something we should be more upfront with in PR description.

It is mentioned in this PR's description, point 2. Feel free to edit better wording in there as I'm not sure what you mean.

and in the Changelog

Good point. Will add something to NEWS.

Also I couldn't understand what does "self healing quote rule" mean, which was mentioned several times in comments.

"Self-healing" is meant to convey "automatic-fixing". The idea is that quote rules 2 and 3 deal with invalid files. Without them, the file would fail to read due to the otherwise invalid quote.

Note that this could be a breaking change for people who run fread under "warnings are errors" mode.

Yes and that's better behaviour. Such users will want to know that fread thinks their file is invalid and an attempt to fix it has been made by data.table. They will likely want to only use fully correct and fully valid formats, so they want fread to warn about any little thing that might be wrong.

Note there is no way to control quote rule currently. Due in part not being sure what its parameter values should take, since the quote rule is still bedding down. It could be "auto.warn" (default), "auto.silent", or a specific rule by number or name. Currently user can turn off the new warning with suppressWarnings() but that also turns off all warnings from that fread call too. I suggest I create a new feature request for quoteRule= to be implemented in future, otherwise we'll never get anything out the door. [Done - #2768]

After all, if a user chooses an uncommon sep (e.g. \x01), then it may be perfectly normal not to quote any fields regardless of whether they contain any quote characters or not.

Yes and that's why quote rule 3 exists. But that is unusual and should be warned about because it's more likely something is wrong in a regular comma-separated-file. If file is using unusual sep for that reason without any quoted fields, but where the field does contain quotes, the warning now suggests to set quote="". This turns off the warning and reads the file cleanly and correctly. Readers of the code containing that fread call would then see immediately that an unquoted file was being read. (I previously suggested leaving this to later, but as it turned out, quote="" already worked -- test 1906 added).

…r quote='' added for no warning when unusual sep used without quoting

mattdowle · 2018-04-18T23:47:52Z

@st-pasha Have addressed your comments and this PR is complete I hope. Please re-review.

mattdowle added 7 commits April 16, 2018 11:45

Interim

7c0476a

Interim

9260600

Interim

849ad82

Interim

0f1625d

Interim

6dd29fe

Interim

da176f7

Tidy

43c21a0

coverage

f6212fb

mattdowle added this to the v1.10.6 milestone Apr 17, 2018

mattdowle requested a review from st-pasha April 17, 2018 01:13

mattdowle changed the title ~~Specifying sep could be ignored~~ fread fill=true and sep= provided could still read as 1-column Apr 17, 2018

Merge branch 'master' into fread_sep_ignored

193ddd2

st-pasha reviewed Apr 17, 2018

View reviewed changes

mattdowle mentioned this pull request Apr 17, 2018

Remove branch CI and keep just pr CI #2743

Closed

mattdowle added 2 commits April 18, 2018 13:51

Merge branch 'master' into fread_sep_ignored

ad40a40

Pasha's comments addressed: NEWS item, superfluous escape and test fo…

7704ec6

…r quote='' added for no warning when unusual sep used without quoting

mattdowle mentioned this pull request Apr 18, 2018

Add quoteRule= to fread #2768

Open

st-pasha approved these changes Apr 19, 2018

View reviewed changes

mattdowle merged commit c44aeb3 into master Apr 19, 2018

mattdowle deleted the fread_sep_ignored branch April 19, 2018 23:30

Navigation Menu

Repository navigation

`fread` fill=true and sep= provided could still read as 1-column - #2755

`fread` fill=true and sep= provided could still read as 1-column#2755
mattdowle merged 11 commits into
masterfrom
fread_sep_ignored

mattdowle commented Apr 17, 2018 •

edited

Loading

Uh oh!

codecov-io commented Apr 17, 2018 •

edited

Loading

Uh oh!

st-pasha left a comment

Uh oh!

st-pasha Apr 17, 2018

Uh oh!

mattdowle Apr 17, 2018 •

edited

Loading

Uh oh!

mattdowle commented Apr 17, 2018 •

edited

Loading

Uh oh!

mattdowle commented Apr 18, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

mattdowle commented Apr 17, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov-io commented Apr 17, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

st-pasha left a comment

Choose a reason for hiding this comment

Uh oh!

st-pasha Apr 17, 2018

Choose a reason for hiding this comment

Uh oh!

mattdowle Apr 17, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mattdowle commented Apr 17, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mattdowle commented Apr 18, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

mattdowle commented Apr 17, 2018 •

edited

Loading

codecov-io commented Apr 17, 2018 •

edited

Loading

mattdowle Apr 17, 2018 •

edited

Loading

mattdowle commented Apr 17, 2018 •

edited

Loading