fix TextLoader bug when there's newline between quotes #4584

LittleLittleCloud · 2019-12-17T23:53:57Z

A quick workaround for this Issue
#4460
perhaps also this one
#4555

JakeRadMSFT · 2019-12-17T23:55:56Z

src/Microsoft.ML.Data/Utilities/StreamUtils.cs

+
+        public static string ReadEntry(this TextReader sr)
+        {
+            string strReturn = string.Empty;


string entry

JakeRadMSFT · 2019-12-17T23:56:27Z

src/Microsoft.ML.Data/Utilities/StreamUtils.cs

+            // And get more lines until the number of quotes is even
+            while (strReturn.GetNumberOf("\"").IsOdd())
+            {
+                string strNow = sr.ReadLine();


string line

JakeRadMSFT · 2019-12-17T23:57:59Z

src/Microsoft.ML.Data/Utilities/StreamUtils.cs

+            }
+        }
+
+        public static int GetNumberOf(this string s, string strSearchString)


I would just leave this as a static method instead of an extension method. It's easier to read that way.

I would agree, especially with completion for unimported extension methods coming from Roslyn. Specialized extension methods for common types will be uncomfortably noisy during development.

JakeRadMSFT · 2019-12-17T23:59:00Z

src/Microsoft.ML.Data/Utilities/StreamUtils.cs

+
+        public static int GetNumberOf(this string s, string strSearchString)
+        {
+            return s.Length - s.Replace(strSearchString, string.Empty).Length;


Interesting approach :)

Actually there's a bug.... It should divided by the length of strSearchString to get the correct result. also if I do that, it won't work on string.Empty (div by 0)... so hard to write the correct code!

justinormont · 2019-12-20T06:44:34Z

src/Microsoft.ML.Data/Utilities/StreamUtils.cs

+            entry += sr.ReadLine();
+
+            // And get more lines until the number of quotes is even
+            while (GetNumberOf(entry, "\"") % 2 != 0 )


Need to first check that that quoting is enabled, and need to check for escaped quotes. May need a flag/field to define the escaping type, defaulting to allow either none or both.

Some tests:

True,"asdf,123 is perfectly ok without quoting enabled

True,"12\" in a foot",123 has an escaped quote w/ slash escaping

True,"12"" in a foot",123 has an escaped quote w/ double-double quotes (Excel style)

True, "asdf,123 is ok, if following Excel style as it ignores quoted fields with a space before

Also need to not append to lines at the start of the file which begin with // or #:

if (entry[0] == '#' || entry[0] == '/' && entry[1] == '/') { ... }

So try to repeat the logic here

read line
if 'line' starts with '#' or starts with '\' => return 'line' # comments
if quote is disabled: => use the existing logic in TextLoader
else
if ( 'line' has only odd# real quote ) => read next 'line' until sum of real quote is even and return 'line' # real quote means " and it's not one of [ "", \" "]
else return 'line'

And BTW it seems the existing enable quote logic doesn't check if it's real quote or not. ( for example in lines: "blablabla""blablabla" will cause an error in FetchNextField function, but in Excel it's legal) Do we have a plan for adding support for that?

justinormont · 2019-12-20T06:45:23Z

src/Microsoft.ML.Data/Utilities/StreamUtils.cs

+
+        public static int GetNumberOf(string s, string strSearchString)
+        {
+            if(strSearchString.Length == 0 || s.Length == 0)


Suggested change

if(strSearchString.Length == 0 || s.Length == 0)

if (strSearchString.Length == 0 || s.Length == 0)

justinormont · 2019-12-20T06:49:56Z

src/Microsoft.ML.Data/Utilities/StreamUtils.cs

+            {
+                return 0;
+            }
+            return (s.Length - s.Replace(strSearchString, string.Empty).Length) / strSearchString.Length;


String.Replace() is not efficient as it reallocates.

justinormont · 2019-12-20T06:51:18Z

src/Microsoft.ML.Data/DataLoadSave/Text/TextLoaderCursor.cs

@@ -514,7 +514,7 @@ private void ThreadProc()
                                    if (_abort)
                                        return;

-                                    text = rdr.ReadLine();
+                                    text = rdr.ReadEntry();


Should check if moving to multi-line reading will interfere with multi-threaded reading.

How to check that, could you expand a bit?

justinormont · 2019-12-20T06:51:44Z

src/Microsoft.ML.Data/DataLoadSave/Text/TextLoaderCursor.cs

@@ -487,7 +487,7 @@ private void ThreadProc()
                                    // REVIEW: Avoid allocating a string for every line. This would probably require
                                    // introducing a CharSpan type (similar to ReadOnlyMemory but based on char[] or StringBuilder)
                                    // and implementing all the necessary conversion functionality on it. See task 3871.
-                                    text = rdr.ReadLine();
+                                    text = rdr.ReadEntry();


Recommend a flag for multi-line support.

I think it can share the same flag with EnableQuote, If we allow quote, then multi-line support is necessary to make sure TextLoader function well

justinormont · 2019-12-20T06:53:11Z

src/Microsoft.ML.Data/Utilities/StreamUtils.cs

+            while (GetNumberOf(entry, "\"") % 2 != 0 )
+            {
+                string line = sr.ReadLine();
+                entry += line;


Likely want to begin a string builder instead of a +=.

You'll want to push thru a newline into the string builder. ReadLine() doesn't include the \n or \r or \r\n [ref].

Otherwise:

a,b,"c my missing space",e,f

Would be read as:

a,b,"c my missingspace",e,f

Instead of:

a,b,"c my missing\nspace",e,f

It likely is not important, but it may be better to match the \n or \r or \r\n of the last line read. This would only be important if the user read the file w/ TextLoader but fed the model with an IEnumerable.

fix textloader bug on quotes

e03b0e4

JakeRadMSFT reviewed Dec 17, 2019

View reviewed changes

LittleLittleCloud changed the title ~~fix textloader bug on quotes~~ fix TextLoader bug when there's newline between quotes Dec 18, 2019

LittleLittleCloud marked this pull request as ready for review December 18, 2019 00:14

LittleLittleCloud requested a review from a team as a code owner December 18, 2019 00:14

LittleLittleCloud added 2 commits December 19, 2019 10:30

use static method instead of extension method

3ddb3b0

better name

9b20ead

justinormont reviewed Dec 20, 2019

View reviewed changes

LittleLittleCloud mentioned this pull request Dec 23, 2019

TSV Dataset with commas doesn't work dotnet/machinelearning-modelbuilder#327

Closed

Merge branch 'master' into u/xiaoyun/fixTextLoaderBug

34d9efa

antoniovs1029 mentioned this pull request May 14, 2020

Enable TextLoader to accept new lines in quoted fields #5125

Merged

harishsk closed this May 29, 2020

ghost locked as resolved and limited conversation to collaborators Mar 19, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix TextLoader bug when there's newline between quotes #4584

fix TextLoader bug when there's newline between quotes #4584

LittleLittleCloud commented Dec 17, 2019 •

edited

Loading

JakeRadMSFT Dec 17, 2019

JakeRadMSFT Dec 17, 2019

JakeRadMSFT Dec 17, 2019

sharwell Dec 18, 2019

JakeRadMSFT Dec 17, 2019

LittleLittleCloud Dec 18, 2019

justinormont Dec 20, 2019 •

edited

Loading

justinormont Dec 20, 2019 •

edited

Loading

LittleLittleCloud Dec 20, 2019 •

edited

Loading

justinormont Dec 20, 2019

justinormont Dec 20, 2019

justinormont Dec 20, 2019

LittleLittleCloud Dec 20, 2019

justinormont Dec 20, 2019

LittleLittleCloud Dec 20, 2019 •

edited

Loading

justinormont Dec 20, 2019

justinormont Dec 20, 2019

	if(strSearchString.Length == 0 \|\| s.Length == 0)
	if (strSearchString.Length == 0 \|\| s.Length == 0)

fix TextLoader bug when there's newline between quotes #4584

fix TextLoader bug when there's newline between quotes #4584

Conversation

LittleLittleCloud commented Dec 17, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

justinormont Dec 20, 2019 • edited Loading

Choose a reason for hiding this comment

justinormont Dec 20, 2019 • edited Loading

Choose a reason for hiding this comment

LittleLittleCloud Dec 20, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LittleLittleCloud Dec 20, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LittleLittleCloud commented Dec 17, 2019 •

edited

Loading

justinormont Dec 20, 2019 •

edited

Loading

justinormont Dec 20, 2019 •

edited

Loading

LittleLittleCloud Dec 20, 2019 •

edited

Loading

LittleLittleCloud Dec 20, 2019 •

edited

Loading