You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
When attempting to import either of the .csv files from GitHubs' recent "Open Source Survey 2017" data release, they both fail with an error message for the first line:
That error message is from negative_incidents.csv, but survey_data.csv (much larger file) exhibits the same problem. Looking through the data, there's no obvious line of double the correct length. The error message itself says there's double the number of fields, which indicates some kind of problem with the line ending not being recognised.
The data itself doesn't seem to have a problem though. The EOL character is 0D (carriage return):
Haven't yet looked at the code, but I'm guessing we're assuming Windows style newlines. If that's the case, we should probably update the import dialog to accept platform-dependent newlines. Either automatically, or ask like we do in the export dialog. Kind of thinking we should be able to auto-detect them though. 😄
Useful extra information
I'm opening this issue because:
DB4S has a bug
I'm using DB4S on:
Mac OS: (10.11.6)
I'm using DB4S version:
3.9.1
Other: Nightly build from a few days ago (2017-05-2*)
Wow, what an issue 😆 I'm wondering what the chances of this happening are. And what a hell to debug.
So it turns out this is not caused by the 0D. We actually support this already. You can tell by the fact that the preview in the Import CSV dialog is working correctly. Instead the issue is caused by the fact that we're not reading the CSV file entirely at once but in smaller chunks. We then loop through each chunk byte by byte and parse it. Now, if we hit a \r character we do a lookahead check if the next byte is a \n character. The problem arises when the \r character is right at the end of the current buffer, that means the following character isn't available yet. Essentially this would put the parser in a wrong state. This is by the way also the reason that smaller portions of the file (what I tried first) won't trigger the error. Anyway, the issue is now fixed by checking for this exact case and loading an extra byte into the buffer when needed 😄
@justinclift Can you double check it's working for you, too? 😄
Just tested on Fedora 25, and the imports worked. I'll test it on the OSX Desktop in a few minutes too just to make sure, as that's what the problem originally showed up on.
Activity
[-]Display failed row number when CSV import fails[/-][+]CSV import fails on OSX when EOL char is `0D`[/+]Fix CSV import when line breaks appear at the buffer boundary
MKleusberg commentedon Jun 29, 2017
Wow, what an issue 😆 I'm wondering what the chances of this happening are. And what a hell to debug.
So it turns out this is not caused by the
0D. We actually support this already. You can tell by the fact that the preview in the Import CSV dialog is working correctly. Instead the issue is caused by the fact that we're not reading the CSV file entirely at once but in smaller chunks. We then loop through each chunk byte by byte and parse it. Now, if we hit a \r character we do a lookahead check if the next byte is a \n character. The problem arises when the \r character is right at the end of the current buffer, that means the following character isn't available yet. Essentially this would put the parser in a wrong state. This is by the way also the reason that smaller portions of the file (what I tried first) won't trigger the error. Anyway, the issue is now fixed by checking for this exact case and loading an extra byte into the buffer when needed 😄@justinclift Can you double check it's working for you, too? 😄
justinclift commentedon Jun 30, 2017
Yep, will do shortly. 😄
justinclift commentedon Jun 30, 2017
Just tested on Fedora 25, and the imports worked. I'll test it on the OSX Desktop in a few minutes too just to make sure, as that's what the problem originally showed up on.
justinclift commentedon Jun 30, 2017
Awesome, yep this is definitely fixed now @MKleusberg. Tested it now on the OSX desktop... import went flawlessly. Thanks heaps. 😄