add max buffer sizes and if should process bad field flag #2188
Conversation
|
A similar feature was added in #1278 (version 30) as |
That is completely different since it lets skip too long rows/fields. MaxFieldSize would throw whole parsing if set. BTW: Tomorrow I will push improvements to that pr's branch. |
|
I've just added mentioned improvement. |
| @@ -28,6 +28,8 @@ public record CsvConfiguration : IReaderConfiguration, IWriterConfiguration | |||
| /// <inheritdoc/> | |||
| public virtual BadDataFound BadDataFound { get; set; } = ConfigurationFunctions.BadDataFound; | |||
|
|
|||
| public virtual bool ProcessBadDataFields { get; set; } = true; | |||
There was a problem hiding this comment.
All the other properties (not only properties) have /// <inheritdoc/> above them, you forgot to add it above this line
| @@ -281,15 +369,56 @@ public async Task<bool> ReadAsync() | |||
| } | |||
| } | |||
|
|
|||
| private async Task<bool> FillBufferAsync() | |||
There was a problem hiding this comment.
- You have fully repeated the FillBuffer method's code, it's not good. Don't you think it would be better to call FillBufferAsync().Result from the FillBuffer method?
- The method's purpose is not clear, because of a lot of global fields. I don't think it's a good idea to refactor this class now, so add a comment line above both FillBuffer and FillBufferAsync
- It's a little unclear what is going on in lines 386 to 407, maybe we could call an additional method with a good name?
| @@ -356,34 +494,41 @@ private ReadLineResult ReadLine(ref char c, ref char cPrev) | |||
| } | |||
|
|
|||
| var isFirstCharOfRow = rowStartPosition == bufferPosition - 1; | |||
| if (isFirstCharOfRow && (allowComments && c == comment || ignoreBlankLines && ((c == '\r' || c == '\n') && !isNewLineSet || c == newLineFirstChar && isNewLineSet))) | |||
|
|
|||
| if (isFirstCharOfRow && | |||
There was a problem hiding this comment.
Too many conditions for one if statement, hard to understand, maybe we can create several bool variables, so we get something like this -
bool4 = bool7 || bool8;
bool3 = (bool2 && bool3) || (bool4 && bool5);
if (isFirstCharOfRow && bool3)
{}
| } | ||
|
|
||
| if (buffer[newStart] != quote || buffer[newStart + newLength - 1] != quote || newLength == 1 && buffer[newStart] == quote) | ||
| if (buffer[newStart] != quote || |
There was a problem hiding this comment.
Maybe we could move these conditions to an additional method and call it, I think we can make the purpose of these conditions more clear
|
@nicemanman thanks for review but you mostly reviewed things which are not related to this pull request. Please create new pull request regarding changes you propose which are not related to this branch. |
Okay, I will, thanks |
It is possible that buffers and field buffers can grow with no limits what leads to big arrays allocations and large memory consumption, what finally leads to OutOfMemory exception. This PR introduces properties to limit max sizes of those buffers. And beside that ProcessBadDataFields flag which makes 'bad field' not processed. Since default is true, current behaviour would not be changed. Otherwise that field will be set to array of empty char because in such flag setting we are not interested in processing bad field. In some cases it can improve performance and memory consumption.
WARNING: Limiting max processed field buffer size can lead to field value truncation (if that buffer is too small to fit in field). But it works well with ShouldSkipRecord configured to skip too long fields.