Properly determine CSV delimiter

[richmahn]

Fixes #16558

First thing this changes is that if a Filename is passed in the RenderContext object, the extension is used:

.tsv => tab
.psv => pipe (|)

If no filename is given, or the extension is the generic ".csv", we need a way to guess the delimiter that will most likely result in the CSV table will be generated. If we try all the allowed delimiters on the first 10 lines (or less if there are less than that), the one we pick should actually work with the CSV Reader and not fail (otherwise just default to comma).

So this does away with the "best score" algorithm in favor of just getting at most 10 lines, emptying any cell values that are quoted (as they may have new lines in the text and we want the first 10 rows, not just lines, of the CSV), and trying the CSV text with each delimiter to see if rendering the CSV rows passes or not. If not, go onto the next delimiter.

If two or more delimiters actually pass, the new algorithm will pick the one that results in the most columns. Many delimiters will pass, as using tab with

a,b
c,d

will of course pass, but only have 1 column. The comma delimiter results in two columns, so it is used.

[wxiaoguang]

We may need some tests to cover guessDelimiter and determineDelimiter

[richmahn]

We may need some tests to cover guessDelimiter and determineDelimiter

@wxiaoguang I added tons of tests. Not sure what you are saying. You have to expand that diff since it is too large! ;)

[richmahn]

@wxiaoguang Since I guess people aren't seeing the test file, csv_test.go, I shorted one test that actually deals with this issue at and where the "best score" algorithm would have failed to get the tab delimiter, but my guessDelimiter() function gets it right. It's case 12 at the bottom. Now the diff shows since it didn't actually need that much text.

https://github.com/go-gitea/gitea/pull/17459/files#diff-de6131d15bff9ea571b236e6b2d9702949f1fd3b3af4648e762e39851f90f201R352

[wxiaoguang]

Oh yes, impressive! Now the only problem is the CI failure 😂

[richmahn]

@wxiaoguang I actually knew a spelling error would get me. Forgot to run that test locally. Ok, fixed.

[codecov-commenter]

Codecov Report

Merging #17459 (88a2642) into main (63c0dc8) will increase coverage by 0.03%.
The diff coverage is 92.45%.

@@            Coverage Diff             @@
##             main   #17459      +/-   ##
==========================================
+ Coverage   45.47%   45.51%   +0.03%     
==========================================
  Files         791      791              
  Lines       88702    88717      +15     
==========================================
+ Hits        40341    40380      +39     
+ Misses      41849    41828      -21     
+ Partials     6512     6509       -3     

Impacted Files	Coverage Δ
routers/web/repo/compare.go	43.52% <0.00%> (ø)
modules/csv/csv.go	100.00% <100.00%> (+22.00%)	⬆️
modules/markup/csv/csv.go	33.78% <100.00%> (ø)
models/unit.go	41.09% <0.00%> (-2.74%)	⬇️
models/issue_comment.go	51.89% <0.00%> (+0.29%)	⬆️
modules/queue/queue_bytefifo.go	59.88% <0.00%> (+0.59%)	⬆️
models/repo_list.go	79.21% <0.00%> (+0.78%)	⬆️
modules/queue/workerpool.go	51.90% <0.00%> (+3.81%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 63c0dc8...88a2642. Read the comment docs.

[delvh]

Do we count file size in decimal or binary units?
Because in binary, this number should be a bit larger (10192, or 2^13, if I'm not mistaken).
I know that oftentimes, compilers prefer powers of two as those can be optimized better. I guess that should be the case here as well?

[richmahn]

This isn't something I added, as it was always a sample size of 10,000. So yeah, it is using 'k' here in the comments that were there before I worked on this as decimal. It's just an arbitrary sample size. I made it a constant so I can check if we may have a truncated line when getting 10 lines from that sample text.

See: https://github.com/go-gitea/gitea/blob/main/modules/csv/csv.go#L34

[delvh]

It's just an arbitrary sample size

Yes. And I'm arguing to set it to a value that the compiler will be able to optimize better.

One small correction, though: Apparently, I thought that 2^(11+1) would be 5096, instead of 4096.
So, what I am asking for is to at best use either 1<<13 (8192) or 1<<14 (16384) bytes.
Which do you prefer?
Or do you prefer the 10000, which is slightly easier to read, but will be worse in runtime performance?

[wxiaoguang]

It doesn't matter whatever we choose, 1e4, 16384, 8192, all work, you won't feel any difference.

We are not writing an OS or a memory allocation library, we won't benefit from the page-aligned memory size.

We do not need to struggle on such trivial details.

[delvh]

So if I see that correctly, adding tabs between the separator (i.e. comma) and a quoted value, we will have an incorrectly guessed delimiter (tab)? I don't think we've seen the last of this issue with this approach.

[richmahn]

@delvh Do people add tabs after a separator? I know many people find a comma delimited csv to be easier to read with a space after the comma or semicolon:

col1, col2, col3
a, b, c

But I can't image in doing

col1, 	col2, 	col3
a, 	b, 	c

Are you suggesting to allow [\s]* instead of just a space so it gets all white space? Yet then I wonder if that could mess up when there are two tabs because no value for that cell

col1 	col2 	col3
a 	 	c

Maybe

`([,@\t;|]{0,1})\s*?(?:"[^"]*")+([,@\t;|]{0,1})`

as the regex?

[richmahn]

Just realized, the above regex I suggested would now fail on this, guessing comma instead of tab, even though the first row uses a tab:

col1	col2
he said,	"""hello"""
she said,	"""goodbye"""

Here the comma is part of the text. I think if this is the case, we should just use the guestDelimiter() function and not try to do this before/after quote as it might guess wrong. If we still use it, we should probably try it on the first row to make sure we get more than two columns returned, or maybe in guessDelimiter() we call guessFromBeforeAfterQuotes and we try that on the 10 lines first, rather than trying all delimiters and if it passes, just return it?

[richmahn]

I'm voting it is just best to go through the list of the delimiters and find what works for the sample 1 - 10 lines, removing all quoted strings so we have no values containing new lines. If we do this "find delimiter before/after quote" we still have to check if it works with the sample text.