Repository navigation
Fix https://github.com/dotnet/corefxlab/issues/2906 - #2907
5 commits merged into
Conversation
| using (var streamReader = new StreamReader(csvStream, encoding ?? Encoding.UTF8, detectEncodingFromByteOrderMarks: true, DefaultStreamReaderBufferSize, leaveOpen: true)) | ||
| { | ||
| string line = null; | ||
| if (dataTypes == null) |
There was a problem hiding this comment.
Do you remember why we were doing this in the first place? Or was this just a mistake?
There was a problem hiding this comment.
In the case of dataTypes being passed in, should we stop processing after the header row?
In reply to: 416981821 [](ancestors = 416981821)
There was a problem hiding this comment.
Yea I was wondering about it too. I think I was using dataTypes inside as part of "guessing" the types at one point. Either that or it was a mistake made from trying to optimize the csv read.
There was a problem hiding this comment.
Good idea. I fixed that, and found some other corner cases and added unit tests for them.
| if ((numberOfRowsToRead == -1) || rowline < numberOfRowsToRead) | ||
| { | ||
| if ((numberOfRowsToRead == -1) || rowline < numberOfRowsToRead) | ||
| if (linesForGuessType.Count < guessRows || guessRows == 0) |
There was a problem hiding this comment.
Isn't the right check here:
if (linesForGuessType.Count < guessRows || (header && rowline == 0))
?
If guessRows == 0 and !header, then there is no reason to get here, right?
There was a problem hiding this comment.
Yup, your idea is better. The only other thing I had to do was to change(line 248)
if (rowline == guessRows)
{
break;
}to
if (rowline == guessRows || guessRows == 0)
{
break;
}Otherwise we'd end up reading the whole stream when guessRows = 0 unnecessarily.
eerhardt
left a comment
eerhardt
left a comment
There was a problem hiding this comment.
Just one minor question on a check. Other than that, this looks good.
|
Hello @pgovind! Because this pull request has the p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (
|
We were dropping the column names when
DataTypeswas being passed in toLoadCsv.I suggest looking at the diff with whitespace turned off