Throw Warning When Reading File with Blank Lines

[Asa-Henry]

Adds a check to detect when blank lines are present in the file passed to fread, but the user did not specify to skip blank lines. This change throws a warning to let the user know they may want to consider setting the blank.lines.skip to TRUE. Closes #3339

[jangorecki]

PR could have meaningful title

[tdhock]

please add a test case

[aitap]

This is a good start.

Can you run R CMD build . in the Git checkout directory and then R CMD check data.table_1.18.99.tar.gz? You'll need a working compiler for this (Rtools on Windows, highly specific compiler versions and libraries on macOS, feel free to ask otherwise) and all of data.table's dependencies installed (which you can do using something like source('.ci/ci.R'); dcf.dependencies(which = 'all') |> install.packages(); this is needed only once). Generally, data.table is required to pass the check with nothing worse than a few NOTEs. (There are some shorthands for this process that need a little more familiarity with R. You can learn about them if you want to by reading .dev/cc.R. See also ?data.table::test.data.table.)

You can see that the tests don't pass. I think that tests 1578.1, 1578.2, 1578.6, 1883 will need to be updated for the new warning, possibly 1867.07 and 1867.11 too. Unfortunately, other tests show a regression:

• tests 1840.2, 1867.05, 1867.06, 1867.09, 2251.07, 2251.09 produce the "blank line" warning despite there are no blank lines in the input:

  write('a,b\n ab,cd,ce\nabc,def \n hj,kli  ', f<-tempfile())
  fread(f)

  fread("A\nCol1,Col2\n1,3,5\n2,4,6\n")
  fread("Some header\ninfo\nCol1,Col2,Col3\n1,3,5\n2,4,6\n")
  fread("Heading text\nA,B,C,D,E\n1,3,5\n2,4,6\n")

  lines = c(
  "12223, University",
  "12227, bridge, Sky",
  "12828, Sunset",
  "13801, Ground",
  "14853, Tranceamerica",
  "14854, San Francisco",
  "15595, shibuya, Shrine",
  "16126, fog, San Francisco",
  "16520, California, ocean, summer, golden gate, beach, San Francisco",
  "")
  text = paste(lines, collapse="\n")
  fread(text)
  text = paste(lines[c(1:5, 9L, 6:8, 10L)], collapse="\n")
  fread(text)

• test 1856.3 warns about the blank lines despite they don't pose a real problem:

  fread("A,B\n\n\n")

Any ideas how the spurious warnings can be fixed? If you need to debug data.table::fread while it runs, you can run R -d $your_debugger or call Sys.getpid() and attach to the running process.

[Asa-Henry]

Thanks for this comprehensive response @aitap! This made it easier to understand what went wrong with my changes!

In response to your question: it appears I introduced confusion between a file with intermittent blank lines and a file which has the header and the data separated by a blank line. For issues such as 1840.2, looks like I over looked how spaces come into play when parsing...

I picked up on that topSkip, prevStart, and topStart are used to find chunks of lines, but topSkip was greater than zero for the example case presented in the issue #3339, but I don't know enough about freadMain to know what all goes into dealing with blank lines. Is there any insight for how freadMain handles files or maybe that I'm working in the wrong place?

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 99.04%. Comparing base (3a45206) to head (e70f5da).

Additional details and impacted files

@@           Coverage Diff           @@
##           master    #7707   +/-   ##
=======================================
  Coverage   99.04%   99.04%           
=======================================
  Files          87       87           
  Lines       17123    17041   -82     
=======================================
- Hits        16959    16878   -81     
+ Misses        164      163    -1     

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

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[Asa-Henry]

What is the problem with the 'atime performance tests'? It's saying that fix-3339 is an invalid reference: I wondering if I made the pull request from the wrong place?

[MichaelChirico]

What is the problem with the 'atime performance tests'? It's saying that fix-3339 is an invalid reference: I wondering if I made the pull request from the wrong place?

That CI doesn't work on PRs from forks -- you can ignore it

[Asa-Henry]

May I get some help writing a test? I'm trying to make a test using the input shown in the initial issue #3339. The following is what I have:

input = "x y\n\n1 a\n\n2 b\n\n3 c"
test(
  1578.10,
  fread(input),
  data.table(V1=3, V2=c),
  warning="The rows in this file appear to be separated by blank lines. This resulted in most rows being skipped. If this was not the intended outcome, please consider setting 'blank.lines.skip' to TRUE."
)

But then test 1578.1 fails. If I don't add my test, then all tests pass.

[aitap]

Congratulations on figuring this out! (Sorry I didn't find the time to help.)

Your test should work after you replace data.table(V1=3, V2=c) (which results in V2 being a list containing the function base::c) with data.table(V1=3L, V2="c") (R also distinguishes integers and double precision floats). This should also help increase coverage.

[aitap]

Oh, right. Test numbers are ordinary doubles, so test() cannot distinguish between 1578.1 that it had already seen and the new 1578.10. On the other hand, test numbers must be strictly monotonically rising, so the test fails before running any code ("Test id 1578.1 is not in increasing order"). The minimal change that would fix that would be to renumber the new test 1578.91.

[Asa-Henry]

I might need some more support writing an issue or two for my pull request. When testing locally, is there a file where more information about what went wrong with particular issue is stored?

[aitap]

A failed test tests/*.R leaves a file named data.table.Rcheck/tests/*.Rout.fail. In your case the difference boils down to the extra newline. If you capture the warning condition using withCallingHandlers() or tryCatch(), you can see that it doesn't end with a newline; R adds a newline by itself when it prints the warning.

[Asa-Henry]

Thanks for pointing out the newline @aitap! Looks like that was the last base to be covered! Is it okay that the issue is '1578.91'? I noticed there is a scheme where 0s are added as needed to accommodate tests which have many significant figures.

I believe the last thing is to add to NEWS.md?

[aitap]

Yes, a NEWS.md entry is in order. You can also renumber the tests to go from .01 to .10 instead of .1 to .91 if you'd like to. The former option is more aesthetically pleasing (people reading the code will be happier) and the latter gives a smaller diff (less potential for conflicts with other PRs, easier to git bisect).