readLines with fread by dselivanov · Pull Request #1518 · Rdatatable/data.table

dselivanov · 2016-02-04T16:54:20Z

It is tricky to use fread as replacement for readLines for reading non-tabular raw text data. In fact I realised that I actually can do that only after creating this PR.
Consider following example where I want to read text without splitting it into data.frame/data.table

txt <- fread('a,b\n ab,cd,ce\n abcdef\n hjkli \n')
# Error in fread("a,b\n ab,cd,ce\n abcdef\n hjkli \n") : 
#  Expected sep (',') but new line, EOF (or other non printing character) ends field 0 when detecting types (   first):  abcdef
#In addition: Warning message:
#In fread("a,b\n ab,cd,ce\n abcdef\n hjkli \n") :
#  Starting data input on line 2 and discarding line 1 because it has too few or too many items to be column names or data: a,b

Fortunately (and I realised that only after browsing fread.c source code) we can pass sep = "\n".

txt <- fread('a,b\n ab,cd,ce\n abcdef\n hjkli \n', sep = '\n')
#        a,b
#1: ab,cd,ce
#2:   abcdef
#3:    hjkli

Note, that we received "hjkli", not " hjkli ", because strip.white = TRUE by default. This also can be misleading.

And the last - most painful. It is not possible to do trick above on windows, where EOL = '\r\n':

fread("a,b\n ab,cd,ce\n abcdef\n hjkli \n", sep = "\r\n")
#Error in fread("a,b\n ab,cd,ce\n abcdef\n hjkli \n", sep = "\r\n") : 
#  'sep' must be 'auto' or a single character

arunsrinivasan · 2016-02-04T22:36:11Z

Hm, Hash Sum mismatch error in travis-ci.. strange.

Looks nice, could you please remove irrelevant (spacing) lines from the PR so that the actual changes are easily visible to have a look at? Thanks.

dselivanov · 2016-02-05T06:53:52Z

see updated PR. Do you mind if I will open separate PR for syntax/readability refinements?

arunsrinivasan · 2016-02-08T18:18:41Z

@dselivanov I've not yet had time to look at the PR, sorry.

On syntax/readability, I'd suggest starting a new thread/issue so as to develop/write data.table package coding style (has been on my internal to-do for quite some time now :-( ).

MichaelChirico · 2016-02-10T02:43:35Z

this is cool. i'd been using sep = "^" for this but always anxious that there will be a ^ hidden in my file one time. good to have a robust alternative.

dselivanov · 2016-02-10T12:19:53Z

@arunsrinivasan see #1532.

arunsrinivasan · 2016-03-04T18:18:47Z

@dselivanov just had a look. Nice! Could you explain in words about sep = NULL in fread.Rd? Seems to be the only thing that remains.

MichaelChirico · 2016-03-04T21:04:23Z

Could also be useful to have some benchmarks to compare performance vis-a-vis readLines... just not sure what the proper medium for putting those benchmarks is. On the wiki somewhere? FAQ?

dselivanov · 2016-03-09T08:41:03Z

@arunsrinivasan sure, will update PR soon. Another question - in case of sep = NULL should we still return data.table/data.frame with single column or just plain character?

jangorecki · 2016-03-09T10:30:09Z

@dselivanov fread has data.table argument which could be used in that context, when data.table=FALSE then a vector could be returned.

arunsrinivasan · 2016-03-09T19:12:06Z

@dselivanov same behaviour as always. Default is data.table. data.table=FALSE returns data.frame (1-col).

On second thought, sep=NULL is quite confusing. Would it be possible to have the same functionality with sep="\n" instead? The logic to detect if eol is \r\n or \n is already there.. so sep="\n" could just use it?

Also, sep="\n" could also be added as the last separator in the auto detection phase.

dselivanov · 2016-03-09T19:24:50Z

@arunsrinivasan sure, we can check for "\n". For me sep = NULL looks very natural:

sep = NULL - do not split/parse
sep = NA_character_ or sep = "auto" - don't know separator, try to guess about it.
sep = any_single_character - know separtor in advance, don't look for it.

Another option is to introduce new function, say, fread_lines which will return plain character and will have only a very few arguments.

arunsrinivasan · 2016-03-09T19:38:39Z

sep="\n" looks natural to me. Why does sep=NULL look more natural to you? Not sure what you mean by split/parse.. it still has to take care of embedded \ns, isn't it?

arunsrinivasan · 2016-03-09T19:40:51Z

Oh I see, sep=NULL reads the entire line as such (is your point). Hm, makes sense.

arunsrinivasan · 2016-03-14T06:52:37Z

I'm not yet sure about this. Will come back later.

MichaelChirico · 2016-03-31T14:14:29Z

Could you file an issue for this? I'd like to refer to an issue number rather than a PR number.

dselivanov · 2016-03-31T14:28:01Z

@MichaelChirico feel free to file issue yourself if you really need it.

dselivanov · 2016-05-12T20:10:21Z

Can I do anything else to improve this PR, so it could be merged?

arunsrinivasan · 2016-05-13T06:55:15Z

@dselivanov sorry for the delay. I've given it some thought. I still find sep="\n" to be more natural. Of course this implicitly assumes "\r\n" if necessary. Would it be possible to make this change?

To me, sep=NULL seems appropriate for the behaviour where the entire file is read in as a character vector of length=1, which is not a particularly useful feature, but just mentioning it to explain what behaviour comes to my mind when I think of sep=NULL.

dselivanov · 2016-05-13T07:53:14Z

@arunsrinivasan ok agree with your point about sep=NULL. I will add sep="\n" in documentation. Internally it will still work with sep=NULL (so auto detection of \r\n will work). Will this be appropriate?

arunsrinivasan · 2016-05-13T08:04:32Z

Okay. As long as sep=NULL errors (current behaviour), it's fine.

dselivanov · 2016-05-23T09:20:56Z

See updated PR. Sorry for long update, totally forgot about it...

jangorecki · 2016-05-23T09:56:51Z

ifelse could be more useful for vectorized input, here strip.white = sep!="\n" or strip.white = !identical(sep,"\n") should be sufficient, isn't it? the latter one if we allow 0 length sep argument.

good point!

jangorecki · 2016-07-21T18:44:33Z

does it wait for anything? assuming feedback was well addressed we could have this feature as it is pretty useful. @mattdowle @arunsrinivasan
edit: will launch check build after rebase due to change in dependencies (chron)

dselivanov · 2016-07-21T18:54:23Z

I only kindly ask to merge it before 1.9.8 CRAN release :-).

21 июля 2016 г. 9:44 PM пользователь "Jan Gorecki" notifications@github.com
написал:

does it wait for anything? assuming feedback was well addressed we could
have this feature as it is pretty useful. @mattdowle
https://github.com/mattdowle @arunsrinivasan
https://github.com/arunsrinivasan

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#1518 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AE4u3cWggBU9O8HF-rrvkkYa5BRkszUoks5qX74YgaJpZM4HTjIL
.

arunsrinivasan · 2016-07-21T18:57:40Z

@dselivanov if you could rebase and resolve the conflicts, I'll take a quick look and merge it in.

jangorecki · 2016-07-21T19:15:24Z

BTW. IMO sep="\n"is not a perfect option. If that feature is about to be used as readLines then there is just a single field in each line, so no separation really. \n refers to new line, which isn't another field, but another row. sep should refers to separation between fields in a row.

arunsrinivasan · 2016-07-21T19:20:26Z

@jangorecki EOF is always an implicit separator.

jangorecki · 2016-07-21T19:24:46Z

Makes more sense, so it is consistent to how strsplit would handle that. Looks good

dselivanov · 2016-07-21T19:33:58Z

I will try to resolve conflicts and update PR.

dselivanov · 2016-09-15T07:20:06Z

@arunsrinivasan rebased now.

jangorecki · 2016-09-15T11:52:47Z

test numbers can be adjusted to avoid duplicated id of tests

Nice catch. I have amended PR.

jangorecki · 2016-09-15T11:54:17Z

it is more readable now 👍

jangorecki · 2016-09-15T11:56:14Z

ideally, this could have one extra space after if

Merge remote-tracking branch 'upstream/master' into fread_lines # Conflicts: # R/fread.R # inst/tests/tests.Rraw

dselivanov · 2016-09-25T07:31:23Z

merge upstream to my PR branch.
@jangorecki I replaced your recent addition withfile argument to file=NULL. It is not a good practice to have arguments without default value after arguments with default values.

codecov-io · 2016-09-25T07:52:07Z

Current coverage is 90.66% (diff: 100%)

Merging #1518 into master will increase coverage by 0.03%

@@             master      #1518   diff @@
==========================================
  Files            58         58          
  Lines         10697      10702     +5   
  Methods           0          0          
  Messages          0          0          
  Branches          0          0          
==========================================
+ Hits           9695       9703     +8   
+ Misses         1002        999     -3   
  Partials          0          0

Powered by Codecov. Last update 5959d67...42a02a9

dselivanov · 2016-10-02T06:40:59Z

@arunsrinivasan @jangorecki anything else I can do?

arunsrinivasan · 2016-10-06T22:10:18Z

Sorry, took a break. Seems fine but might be hard for this release as Matt seems to have almost wrapped up checking against dependent packages. Tagging @mattdowle just in case.

dselivanov · 2017-09-24T18:47:25Z

I don't think this can be easily merged - logic of fread moved forward a lot - closing.

dselivanov force-pushed the fread_lines branch from 249b89c to cae607e Compare February 4, 2016 17:52

dselivanov force-pushed the fread_lines branch 3 times, most recently from a50445e to 23fbe0f Compare February 5, 2016 06:51

arunsrinivasan added enhancement fread labels Mar 4, 2016

arunsrinivasan added this to the v1.9.8 milestone Mar 4, 2016

arunsrinivasan removed this from the v1.9.8 milestone Mar 14, 2016

MichaelChirico mentioned this pull request Mar 31, 2016

FR: Robust option for fread not to split lines #1616

Closed

dselivanov mentioned this pull request Apr 5, 2016

Document that fread can readLines with sep='@' (non present character) #1627

Closed

dselivanov force-pushed the fread_lines branch from 23fbe0f to 6a5d2a9 Compare May 23, 2016 09:16

jangorecki reviewed May 23, 2016
View reviewed changes

dselivanov force-pushed the fread_lines branch from 6a5d2a9 to 4965e05 Compare May 23, 2016 10:12

dselivanov force-pushed the fread_lines branch from 4965e05 to d52ea15 Compare September 15, 2016 07:00

jangorecki reviewed Sep 15, 2016

View reviewed changes

readLines with fread

5eae076

dselivanov force-pushed the fread_lines branch from d52ea15 to 5eae076 Compare September 15, 2016 12:39

merge upstream

42a02a9

Merge remote-tracking branch 'upstream/master' into fread_lines # Conflicts: # R/fread.R # inst/tests/tests.Rraw

st-pasha mentioned this pull request Jul 6, 2017

Master task for fread bugs / proposals #2247

Closed

dselivanov closed this Sep 24, 2017

mattdowle added a commit that referenced this pull request Oct 30, 2017

Added tests for sep=NULL and sep=, #1616 and #1518

3f1df75

Conversation

dselivanov commented Feb 4, 2016

Uh oh!

arunsrinivasan commented Feb 4, 2016

Uh oh!

dselivanov commented Feb 5, 2016

Uh oh!

arunsrinivasan commented Feb 8, 2016

Uh oh!

MichaelChirico commented Feb 10, 2016

Uh oh!

dselivanov commented Feb 10, 2016

Uh oh!

arunsrinivasan commented Mar 4, 2016

Uh oh!

MichaelChirico commented Mar 4, 2016

Uh oh!

dselivanov commented Mar 9, 2016

Uh oh!

jangorecki commented Mar 9, 2016

Uh oh!

arunsrinivasan commented Mar 9, 2016

Uh oh!

dselivanov commented Mar 9, 2016

Uh oh!

arunsrinivasan commented Mar 9, 2016

Uh oh!

arunsrinivasan commented Mar 9, 2016

Uh oh!

arunsrinivasan commented Mar 14, 2016

Uh oh!

MichaelChirico commented Mar 31, 2016

Uh oh!

dselivanov commented Mar 31, 2016

Uh oh!

dselivanov commented May 12, 2016

Uh oh!

arunsrinivasan commented May 13, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dselivanov commented May 13, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

arunsrinivasan commented May 13, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dselivanov commented May 23, 2016

Uh oh!

jangorecki May 23, 2016

Choose a reason for hiding this comment

Uh oh!

dselivanov May 23, 2016

Choose a reason for hiding this comment

Uh oh!

jangorecki commented Jul 21, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dselivanov commented Jul 21, 2016

Uh oh!

arunsrinivasan commented Jul 21, 2016

Uh oh!

jangorecki commented Jul 21, 2016

Uh oh!

arunsrinivasan commented Jul 21, 2016

Uh oh!

jangorecki commented Jul 21, 2016

Uh oh!

dselivanov commented Jul 21, 2016

Uh oh!

dselivanov commented Sep 15, 2016

Uh oh!

jangorecki Sep 15, 2016

Choose a reason for hiding this comment

Uh oh!

dselivanov Sep 15, 2016

Choose a reason for hiding this comment

Uh oh!

jangorecki Sep 15, 2016

Choose a reason for hiding this comment

Uh oh!

arunsrinivasan commented May 13, 2016 •

edited

Loading

dselivanov commented May 13, 2016 •

edited

Loading

arunsrinivasan commented May 13, 2016 •

edited

Loading

jangorecki commented Jul 21, 2016 •

edited

Loading

dselivanov commented Sep 25, 2016 •

edited

Loading