Skip to content

Add escapeChar support to TextLoader and added benchmark for TextLoader #5147

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
harishsk merged 12 commits into from
May 21, 2020
Merged

Add escapeChar support to TextLoader and added benchmark for TextLoader #5147

Show file tree
Hide file tree
Changes from 11 commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
ae99160
Added support for escapeChar to escape quotes inside quoted fields
antoniovs1029 May 20, 2020
22434c1
Fixed bug in logic when field is quoted and empty
antoniovs1029 May 20, 2020
862de8b
Added serialization logic
antoniovs1029 May 20, 2020
caa4dc8
Added tests including backcompat test
antoniovs1029 May 20, 2020
cd26dfb
Updated manifest
antoniovs1029 May 20, 2020
32fffe2
Fix whitespace
antoniovs1029 May 21, 2020
b2d5025
Throw exceptions if escapeChar is also used as separator
antoniovs1029 May 21, 2020
5484d11
Fixed small bug on multireader
antoniovs1029 May 21, 2020
fa16dd8
Updated manifest
antoniovs1029 May 21, 2020
20e07f4
Fixed mistake on deserialization logic
antoniovs1029 May 21, 2020
e331ebe
Added Benchmark test
antoniovs1029 May 21, 2020
098a56e
Updated benchmark
antoniovs1029 May 21, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
35 changes: 31 additions & 4 deletions src/Microsoft.ML.Data/DataLoadSave/Text/TextLoader.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -521,6 +521,12 @@ public class Options
[Argument(ArgumentType.AtMostOnce, HelpText = "Maximum number of rows to produce", ShortName = "rows", Hide = true)]
public long? MaxRows;

/// <summary>
/// Character to use to escape quotes inside quoted fields. It can't be a character used as separator.
/// </summary>
[Argument(ArgumentType.AtMostOnce, HelpText = "Character to use to escape quotes inside quoted fields. It can't be a character used as separator.", ShortName = "escapechar")]
public char EscapeChar = Defaults.EscapeChar;

Comment on lines +524 to +529

@antoniovs1029 antoniovs1029 May 20, 2020 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(pinned to a line of code to allow for threaded conversations)

So currently this PR uses the escapeChar to escape quote chars (") inside quoted fields; which I think should be enough for the main ask made by Justin of letting users escape with \". Still, there is room for expanding this feature in 2 directions:

  1. Allow escapeChar to also escape quote chars inside non-quoted fields. The RFC 4180 would actually consider invalid any quote char (") outside of quoted fields. Still, ML.NET has always allowed unescaped quotes inside non-quoted fields... so if a field contained something like 1,these are 2 quotes "" and this is one " quote,true it would actually read the second field as these are 2 quotes "" and this is one " quote (i.e., without throwing or logging errors, and without escaping the first ""). So if we allow escapeChar to escape quotes even outside quoted fields, it will break previous behavior of ML.NET (of not escaping double quotes) and it would actually be unnecessary (since escaping them was never necessary).

  2. Allow escapeChar to also escape separators (a.k.a delimitation characters) (outside and/or inside quoted fields). Justin mentioned on his ask that other libraries allow escaping separators, but actually it seems that Databricks only allows escaping quotes, whereas Pandas' documentation is more ambiguous, and it says escapechar is used to "escape other characters"... so I'd still need to actually test if pandas allow escaping separators. Justin has also shared with me this paper where (among many other things) they used escapechar to escape both separators, quotes and the escapeChar itself... but I haven't read it fully yet, and I'm not sure if they conclude anything specifically on the usefulness of escaping separators. Even more, I'm not sure if @justinormont considers this to be a must. Also, it would be ambiguous what to do if the escapeChar = " (default) and we encounter the common case of ... "this is a field",5 ...... should ", be interpreted as an escaped ,? I guess that if escapeChar = " then it should never escape separators... this tricky case might require more changes to the code, making it somewhat bigger and perhaps somewhat less efficient.

My opinion is we shouldn't allow those two cases, and that we should stick with only having escapeChar escape quotes inside quoted fields, as adding the other cases might require more code to maintain and have some impact on efficiency. But I'm eager to hear opinions from others. @harishsk @justinormont #Resolved

@harishsk harishsk May 20, 2020

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree with the above assessment.

In reply to: 427946570 [](ancestors = 427946570)

@justinormont justinormont May 21, 2020 •
edited by antoniovs1029
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd recommend covering the first case. Though, as is, this is a large improvement to the code base.

One possible way to handle it is to only accept quotes when pared w/ a separator:

  • Open quote with one of
    • Preceded w/ a separator: a,"open...close",c
      (with whitespace optional a, "open...close", c)
    • At the start of a line: "open...close",b,c
  • Close quote with one of
    • Followed by a separator: a,"open...close",c
      (no white space allowed)
    • At the end of a line: a,b,"open...close"
bool isOpenQuote, isCloseQuote, escapedChar, isInsideQuotedRegion, fieldClosed;
for (i = 0; i < str.length; i++)
{
  isOpenQuote = !isInsideQuotedRegion && !escapedChar && (
      i == 0 && str[i] == quoteChar
      || i >= 1 && str[i] == quoteChar && str[i-1] == sepChar
      || i >= 2 && str[i] == quoteChar && str[i-2] == sepChar && IsWhiteSpace(str[i-1])
    );

  if (isOpenQuote) { isInsideQuotedRegion = true; }

  isCloseQuote = isInsideQuotedRegion && !isOpenQuote && !escapedChar && (
      str[i] == quoteChar && str[i+1] == sepChar &&  i <= str.length - 2
      || str[i] == quoteChar && i == str.length - 1
    ); 

  if (isCloseQuote) { isInsideQuotedRegion = false; }

  fieldClosed = !isInsideQuotedRegion && (!escapedChar && str[i] == sepChar || i ==  str.length - 1);

  /* if fieldClosed or escapedChar, append to string builder, checking for isOpenQuote/isCloseQuote to skip the start/current char */

  escapedChar = !escapedChar && str[i] == escapeChar && IsEscapable(str[i+1]) && i <= str.length - 2;
}

Along with handling the cases of a,12",b, this " quote was escaped, and 1,these are 2 quotes "" and this is one " quote,true, this also catches the weird though common enough case of unescaped quotes a,"open...unescaped"...close",c.

It wouldn't handle 1,"abc",def" as the middle quote would still need to be properly escaped. #Resolved

@antoniovs1029 antoniovs1029 May 21, 2020 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for taking the time to review this, @justinormont .

In general it would look to me that your logic is correct, and it might be a good (if not the best) to actually fully accept the first case I mentioned above. Although problems would arise if we also wanted to support all of the other current ML.NET behavior... for example, that we currently ignore all spaces before the opening quote and after the closing quote, whereas your logic only accepts 1 space before and none after (and this characteristic is what your logic rely on for distinguishing between a "real" closing quote that's supposed to be used to close the field, or a simple quote that just happens to be inside a field... so we would either have to choose between your logic or ML.NET current behavior). Also, with your logic I'm not sure if the whole QuotingError mechanism in ML.NET (where quoting errors are logged and empty fields are returned if they appear) would make sense.

In any case, as I mention below in another comment the code of the Parser already avoids as much as possible doing comparisons and calling functions. So it might not be good to add all of the checks that your logic needs to apply to every character of the input string. Because of this, I wouldn't recommend going on with your logic.

Specially, since all the valid cases (according to the RFC) are already accepted, and your logic seems to only be accepting some unescaped quotes (for which we would typically simply have a QuotingError), and being able to escape "escapable characters" inside or outside of quoted fields (which might or might not be the desired behavior of any given user, if we hard code which are these escapable characters).

Without your logic, particularly for case 1 you mentioned might be important, the behavior on this PR is to leave unquoted fields untouched... so "1,this is an \"unquoted\" field,true" will read the second field as this is an \"unquoted\" field (i.e. it won't escape the \" as your logic does, but it won't throw errors or anything else, it will simply not remove the \).

So I don't think it's worth it to add more logic to this PR to support escaping in case 1. If that's ok with you. #Resolved

@justinormont justinormont May 21, 2020 •
edited by antoniovs1029
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good. Thanks for thinking through the impact. #Resolved

/// <summary>
/// Checks that all column specifications are valid (that is, ranges are disjoint and have min&lt;=max).
/// </summary>
Expand All @@ -538,6 +544,7 @@ internal static class Defaults
internal const bool HasHeader = false;
internal const bool TrimWhitespace = false;
internal const bool ReadMultilines = false;
internal const char EscapeChar = '"';
}

/// <summary>
Expand Down Expand Up @@ -702,11 +709,11 @@ public Bindings(TextLoader parent, Column[] cols, IMultiStreamSource headerFile,
ch.Assert(0 <= inputSize & inputSize < SrcLim);
List<ReadOnlyMemory<char>> lines = null;
if (headerFile != null)
Cursor.GetSomeLines(headerFile, 1, parent.ReadMultilines, parent._separators, ref lines);
Cursor.GetSomeLines(headerFile, 1, parent.ReadMultilines, parent._separators, parent._escapeChar, ref lines);
if (needInputSize && inputSize == 0)
Cursor.GetSomeLines(dataSample, 100, parent.ReadMultilines, parent._separators, ref lines);
Cursor.GetSomeLines(dataSample, 100, parent.ReadMultilines, parent._separators, parent._escapeChar, ref lines);
else if (headerFile == null && parent.HasHeader)
Cursor.GetSomeLines(dataSample, 1, parent.ReadMultilines, parent._separators, ref lines);
Cursor.GetSomeLines(dataSample, 1, parent.ReadMultilines, parent._separators, parent._escapeChar, ref lines);

if (needInputSize && inputSize == 0)
{
Expand Down Expand Up @@ -1063,7 +1070,8 @@ private static VersionInfo GetVersionInfo()
// verWrittenCur: 0x00010009, // Introduced _flags
//verWrittenCur: 0x0001000A, // Added ForceVector in Range
//verWrittenCur: 0x0001000B, // Header now retained if used and present
verWrittenCur: 0x0001000C, // Removed Min and Contiguous from KeyType, and added ReadMultilines flag to OptionFlags
//verWrittenCur: 0x0001000C, // Removed Min and Contiguous from KeyType, and added ReadMultilines flag to OptionFlags
verWrittenCur: 0x0001000D, // Added escapeChar option
verReadableCur: 0x0001000A,
verWeCanReadBack: 0x00010009,
loaderSignature: LoaderSignature,
Expand All @@ -1090,6 +1098,7 @@ private enum OptionFlags : uint

private readonly bool _useThreads;
private readonly OptionFlags _flags;
private readonly char _escapeChar;
private readonly long _maxRows;
// Input size is zero for unknown - determined by the data (including sparse rows).
private readonly int _inputSize;
Expand Down Expand Up @@ -1210,6 +1219,10 @@ internal TextLoader(IHostEnvironment env, Options options = null, IMultiStreamSo
}
}

_escapeChar = options.EscapeChar;
if(_separators.Contains(_escapeChar))
throw _host.ExceptUserArg(nameof(Options.EscapeChar), "EscapeChar '{0}' can't be used both as EscapeChar and separator", _escapeChar);

_bindings = new Bindings(this, cols, headerFile, dataSample);
_parser = new Parser(this);
}
Expand Down Expand Up @@ -1373,6 +1386,7 @@ private TextLoader(IHost host, ModelLoadContext ctx)
// int: inputSize: 0 for determined from data
// int: number of separators
// char[]: separators
// char: escapeChar
// bindings
int cbFloat = ctx.Reader.ReadInt32();
host.CheckDecode(cbFloat == sizeof(float));
Expand All @@ -1397,6 +1411,17 @@ private TextLoader(IHost host, ModelLoadContext ctx)
if (_separators.Contains(':'))
host.CheckDecode((_flags & OptionFlags.AllowSparse) == 0);

if (ctx.Header.ModelVerWritten >= 0x0001000D)
{
_escapeChar = ctx.Reader.ReadChar();
}
else
{
_escapeChar = Defaults.EscapeChar;
}

host.CheckDecode(!_separators.Contains(_escapeChar));

_bindings = new Bindings(ctx, this);
_parser = new Parser(this);
}
Expand Down Expand Up @@ -1437,6 +1462,7 @@ void ICanSaveModel.Save(ModelSaveContext ctx)
// int: inputSize: 0 for determined from data
// int: number of separators
// char[]: separators
// char: escapeChar
// bindings
ctx.Writer.Write(sizeof(float));
ctx.Writer.Write(_maxRows);
Expand All @@ -1445,6 +1471,7 @@ void ICanSaveModel.Save(ModelSaveContext ctx)
_host.Assert(0 <= _inputSize && _inputSize < SrcLim);
ctx.Writer.Write(_inputSize);
ctx.Writer.WriteCharArray(_separators);
ctx.Writer.Write(_escapeChar);

_bindings.Save(ctx);
}
Expand Down
110 changes: 75 additions & 35 deletions src/Microsoft.ML.Data/DataLoadSave/Text/TextLoaderCursor.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -146,7 +146,7 @@ public static DataViewRowCursor Create(TextLoader parent, IMultiStreamSource fil
SetupCursor(parent, active, 0, out srcNeeded, out cthd);
Contracts.Assert(cthd > 0);

var reader = new LineReader(files, BatchSize, 100, parent.HasHeader, parent.ReadMultilines, parent._separators, parent._maxRows, 1);
var reader = new LineReader(files, BatchSize, 100, parent.HasHeader, parent.ReadMultilines, parent._separators, parent._escapeChar, parent._maxRows, 1);
var stats = new ParseStats(parent._host, 1);
return new Cursor(parent, stats, active, reader, srcNeeded, cthd);
}
Expand All @@ -163,7 +163,7 @@ public static DataViewRowCursor[] CreateSet(TextLoader parent, IMultiStreamSourc
SetupCursor(parent, active, n, out srcNeeded, out cthd);
Contracts.Assert(cthd > 0);

var reader = new LineReader(files, BatchSize, 100, parent.HasHeader, parent.ReadMultilines, parent._separators, parent._maxRows, cthd);
var reader = new LineReader(files, BatchSize, 100, parent.HasHeader, parent.ReadMultilines, parent._separators, parent._escapeChar, parent._maxRows, cthd);
var stats = new ParseStats(parent._host, cthd);
if (cthd <= 1)
return new DataViewRowCursor[1] { new Cursor(parent, stats, active, reader, srcNeeded, 1) };
Expand Down Expand Up @@ -205,7 +205,7 @@ public override ValueGetter<DataViewRowId> GetIdGetter()
};
}

public static void GetSomeLines(IMultiStreamSource source, int count, bool readMultilines, char[] separators, ref List<ReadOnlyMemory<char>> lines)
public static void GetSomeLines(IMultiStreamSource source, int count, bool readMultilines, char[] separators, char escapeChar, ref List<ReadOnlyMemory<char>> lines)
{
Contracts.AssertValue(source);
Contracts.Assert(count > 0);
Expand All @@ -215,7 +215,7 @@ public static void GetSomeLines(IMultiStreamSource source, int count, bool readM
count = 2;

LineBatch batch;
var reader = new LineReader(source, count, 1, false, readMultilines, separators, count, 1);
var reader = new LineReader(source, count, 1, false, readMultilines, separators, escapeChar, count, 1);
try
{
batch = reader.GetBatch();
Expand Down Expand Up @@ -404,6 +404,7 @@ private sealed class LineReader
private readonly bool _hasHeader;
private readonly bool _readMultilines;
private readonly char[] _separators;
private readonly char _escapeChar;
private readonly int _batchSize;
private readonly IMultiStreamSource _files;

Expand All @@ -413,7 +414,7 @@ private sealed class LineReader
private Task _thdRead;
private volatile bool _abort;

public LineReader(IMultiStreamSource files, int batchSize, int bufSize, bool hasHeader, bool readMultilines, char[] separators, long limit, int cref)
public LineReader(IMultiStreamSource files, int batchSize, int bufSize, bool hasHeader, bool readMultilines, char[] separators, char escapeChar, long limit, int cref)
{
// Note that files is allowed to be empty.
Contracts.AssertValue(files);
Expand All @@ -428,6 +429,7 @@ public LineReader(IMultiStreamSource files, int batchSize, int bufSize, bool has
_batchSize = batchSize;
_readMultilines = readMultilines;
_separators = separators;
_escapeChar = escapeChar;
_files = files;
_cref = cref;

Expand Down Expand Up @@ -474,15 +476,19 @@ private class MultiLineReader
private readonly char _sep0;
private readonly char[] _separators;
private readonly bool _sepsContainsSpace;
private readonly char _escapeChar;
private readonly bool _escapeCharIsDoubleQuote;
private readonly StringBuilder _sb;
private readonly TextReader _rdr;

public MultiLineReader(TextReader rdr, char[] separators)
public MultiLineReader(TextReader rdr, char[] separators, char escapeChar)
{
Contracts.AssertNonEmpty(separators);
_sep0 = separators[0];
_separators = separators;
_sepsContainsSpace = IsSep(' ');
_escapeChar = escapeChar;
_escapeCharIsDoubleQuote = (escapeChar == '"');
_sb = new StringBuilder();
_rdr = rdr;
}
Expand Down Expand Up @@ -569,52 +575,86 @@ private bool FieldIncludesNewLine(ref string line, ref int ichCur, int ichLim,
ichCur++;
}

if (ichCur >= ichLim) // if there were only leading spaces on the line
return startsInsideQuoted;

if(startsInsideQuoted || line[ichCur] == '"')
{
// Quoted Field Case

if (!startsInsideQuoted)
ichCur++;

for (; ; ichCur++)
if (_escapeCharIsDoubleQuote)
{

@harishsk harishsk May 21, 2020

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it possible to refactor these two if/else blocks into separate functions for the sake of readability? (Or even better merge them into a single function/codepath?

@antoniovs1029 antoniovs1029 May 21, 2020 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it is possible to have this refactored into two functions or even into one path.

I didn't do this because the original writers of the Parser seemed to be very worried about efficiency and the way they addressed this is by avoiding creating / calling functions, and using bools such as the one I've used here for _escapeCharIsDoubleQuote.

So for example, we have the following code written by them, which can be obviously be refactored into a function, and perhaps even simplified to avoid the first 2 if branches, but it's stated that they didn't do it because it was "performance critical"

machinelearning/src/Microsoft.ML.Data/DataLoadSave/Text/TextLoaderParser.cs

Lines 1208 to 1241 in c023271

// Please note that these branched tight loops are intended and performance critical.
if (_seps.Length == 1)
{
for (; ; ichCur++)
{
Contracts.Assert(ichCur <= ichLim);
if (ichCur >= ichLim)
break;
if (_sep0 == span[ichCur])
break;
}
}
else if (_seps.Length == 2)
{
for (; ; ichCur++)
{
Contracts.Assert(ichCur <= ichLim);
if (ichCur >= ichLim)
break;
if (_sep0 == span[ichCur] || _sep1 == span[ichCur])
break;
}
}
else
{
for (; ; ichCur++)
{
Contracts.Assert(ichCur <= ichLim);
if (ichCur >= ichLim)
break;
if (IsSep(span[ichCur]))
break;
}
}

Following that pattern I thought that the fastest way for the code I am writing to run is to branch earlier with _escapeCharIsDoubleQuote only requiring to check it once... because if I refactor it to call functions or even worse if I merge into a single path I'll have to continuously check if _escapeCharIsDoubleQuote for it to work correctly, then it would have to take more steps and probably have a toll on performance. Also, personally I think that a merged path would be harder to read.

Do you still want me to try to refactor it? If so, I can try to come up with a refactored version for us to compare.

In reply to: 428386984 [](ancestors = 428386984)

@harishsk harishsk May 21, 2020

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I couldn't find any benchmark tests for text loader in the code. Can you please look up and see if I have missed something?

In reply to: 428393614 [](ancestors = 428393614,428386984)

@antoniovs1029 antoniovs1029 May 21, 2020

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think there's any benchmark that only tests text loader. But I think it's a good idea to add one, so I'll do that.

In the meantime, I actually used the FeaturizeTextBench (which only loads a syntetic dataset and featurizes it) to run benchmarks on my previous readMultilines PR:
#5125 (comment)

As I mention there, I'm not sure how to interpret the results, so I will address this tomorrow on scrum.

In reply to: 428398494 [](ancestors = 428398494,428393614,428386984)

@antoniovs1029 antoniovs1029 May 21, 2020 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So I've added a new TextLoaderBench Benchmark test, based on TextFeaturizerBench and CacheDataViewBench. On my new benchmark I simply load a DataView using TextLoader from a syntetic file, get the getter of its first 20 columns and read its 3000 rows without saving them into memory.

The results are as follows. Notice that I still don't understand how to interpret them, as they're somewhat unexpected... it seems they indicate there's not much change across runs, but it might be just as well that perhaps my benchmark isn't conclusive on what it's measuring. I wouldn't know 😕

(notice that even when the mean is very similar across scenarios, the Error is consistently higher without my PRs)

Without this PR, and also without my readMultilines PR

|                Method |    Mean |   Error |   StdDev | Extra Metric |
|---------------------- |--------:|--------:|---------:|-------------:|
| TestTextLoaderGetters | 1.256 s | 3.273 s | 0.1794 s |            - |

With this PR, with readMultilines = false and escapeChar =" (i.e. with defaults)

|                Method |     Mean |    Error |   StdDev | Extra Metric |
|---------------------- |---------:|---------:|---------:|-------------:|
| TestTextLoaderGetters | 810.5 ms | 765.0 ms | 41.93 ms |            - |

With this PR, readMultilines = true, escapeChar = "

|                Method |    Mean |    Error |   StdDev | Extra Metric |
|---------------------- |--------:|---------:|---------:|-------------:|
| TestTextLoaderGetters | 1.089 s | 0.7992 s | 0.0438 s |            - |

With this PR, readMultilines = true, escapeChar = \

|                Method |     Mean |    Error |   StdDev | Extra Metric |
|---------------------- |---------:|---------:|---------:|-------------:|
| TestTextLoaderGetters | 857.9 ms | 696.7 ms | 38.19 ms |            - |

// * Legends *
  Mean         : Arithmetic mean of all measurements
  Error        : Half of 99.9% confidence interval
  StdDev       : Standard deviation of all measurements
  Extra Metric : Value of the provided extra metric
  1 s          : 1 Second (1 sec)

In reply to: 428493474 [](ancestors = 428493474,428398494,428393614,428386984)

if (ichCur >= ichLim)
// We've reached the end of the line without finding the closing quote,
// so next line will start on this quoted field
return true;

if (line[ichCur] == '"')
for (; ; ichCur++)
{
if (++ichCur >= ichLim)
// Last character in line was the closing quote of the field
return false;
if (ichCur >= ichLim)
// We've reached the end of the line without finding the closing quote,
// so next line will start on this quoted field
return true;

if (line[ichCur] == '"')
// 2 Double quotes means escaped quote
continue;
{
if (++ichCur >= ichLim)
// Last character in line was the closing quote of the field
return false;

// If it wasn't an escaped quote, then this is supposed to be
// the closing quote of the field, and there should only be spaces remaining
// until the next separator.
if (line[ichCur] == '"')
// 2 Double quotes means escaped quote
continue;

if (!_sepsContainsSpace)
{
// Ignore leading spaces
while (ichCur < ichLim && line[ichCur] == ' ')
ichCur++;
// If it wasn't an escaped quote, then this is supposed to be
// the closing quote of the field
break;
}
}
}
else
{
for (; ; ichCur++)
{
if (ichCur >= ichLim)
// We've reached the end of the line without finding the closing quote,
// so next line will start on this quoted field
return true;

// If there's anything else than spaces or the next separator,
// this will actually be a QuotingError on the parser, so we decide that this
// line contains a quoting error, and so it's not going to be considered a valid field
// and the rest of the line should be ignored.
if (ichCur >= ichLim || IsSep(line[ichCur]))
return false;
if (line[ichCur] == _escapeChar)
{
if (++ichCur >= ichLim)
// Last character in line was escapeChar
return true;

quotingError = true;
return false;
// Whatever char comes after an escapeChar is ignored
continue;
}
else if (line[ichCur] == '"')
{
// Since this wasn't an escaped quote, then this is supposed to be
// the closing quote of the field
break;
}
}
}

// After finding the closing quote of the field...
// There should only be empty spaces until the next separator
if (!_sepsContainsSpace)
{
// Ignore leading spaces
while (ichCur < ichLim && line[ichCur] == ' ')
ichCur++;
}

// If there's anything else than spaces or the next separator,
// this will actually be a QuotingError on the parser, so we decide that this
// line contains a quoting error, and so it's not going to be considered a valid field
// and the rest of the line should be ignored.
if (ichCur >= ichLim || IsSep(line[ichCur]))
return false;

quotingError = true;
return false;
}

// Unquoted field case.
Expand Down Expand Up @@ -655,7 +695,7 @@ private void ThreadProc()
string path = _files.GetPathOrNull(ifile);
using (var rdr = _files.OpenTextReader(ifile))
{
var multilineReader = new MultiLineReader(rdr, _separators);
var multilineReader = new MultiLineReader(rdr, _separators, _escapeChar);
string text;
long line = 0;
for (; ; )
Expand Down
Loading