Progress indicator - #67
Conversation
kal
left a comment
kal
left a comment
There was a problem hiding this comment.
A couple of minor niggles, but otherwise looks fine.
| dc.getGlobalEnv().put(ConverterProcess.FILE_NAME, filename); | ||
| dc.getGlobalEnv().put(ConverterProcess.FILE_BASE_NAME, filebasename); | ||
| InputStream is = BOMInputStream.builder().setInputStream( new FileInputStream(dataFileF) ).get(); | ||
| int rowCount = LineCount.file(dataFileF) - 1; // discount header row |
There was a problem hiding this comment.
I wonder if it is safer to count the rows before opening the InputStream. I can see that you have two separate handles to the file here, but having the one for processing open while counting the rows just feels like asking for OS level malarky.
|
@simonoakesepimorphics - can you check why the build is stuck in in progress. I don't see it in the Actions tab at all so it might just be a GH issue. |
kal
left a comment
kal
left a comment
There was a problem hiding this comment.
With the clarification about the row count property, I now have a concern that we are confusing row and line counts in calculating progress, which may cause problems in CSVs with muti-line rows.
| process.setTemplate( template ); | ||
| process.setMessageReporter( reporter ); | ||
| process.setAllowNullRows( !args.isNullRowAborts() ); | ||
| process.setRowCount(rowCount); |
There was a problem hiding this comment.
This line sets the row count to the count of the number of lines in the file, not the number of rows and so contradicts what the documentation says. Given that you don't know how many multi-line rows you have without greater parsing up front, it feels like this should be setting a line count, not a row count?
There was a problem hiding this comment.
You're right. I couldn't find an efficient way to deal with this so I rewrote the feature to use the formula (total bytes in file - bytes in header) / (bytes in rows processed).
kal
left a comment
kal
left a comment
There was a problem hiding this comment.
The heuristic used for the bytes read is a bit suspect, and would definitely trip us up in non-European language content. A better solution would probably be to wrap the InputStream in a stream that can report position and expose that through the BindingEnv.
| String[] headers = getHeaders(); | ||
| totalBytes -= headers.length; // assume 1 byte for each column separator | ||
| for (String header: getHeaders()) { | ||
| totalBytes -= header.length(); |
There was a problem hiding this comment.
As a rough heuristic this is OK, but worth remembering that if a file is UTF-8 encoded, one character may encode to multiple bytes.
| /** | ||
| * Set the total number of rows (not lines) to be processed for the purpose of computing progress percentage. | ||
| */ | ||
| public void setRowCount(int rowCount) { |
There was a problem hiding this comment.
This is now redundant and can be removed.
There was a problem hiding this comment.
Done.
| CsvBindingEnv row = new CsvBindingEnv(); | ||
| for (int i = 0; i < rowLength; i++) { | ||
| String rowValue = rowValues[i]; | ||
| sourceBytes += rowValue.length(); |
There was a problem hiding this comment.
Again this heuristic will be thrown out by multi-byte UTF-8 characters.
There was a problem hiding this comment.
I wonder if a better solution would be to wrap the provided InputStream in an implementation that is capable of reporting the current position in the stream? There are a couple of options described in this SO answer - one using a custom decorator, the other suggesting the use of a Commons IO (though the latter references a deprecated class that has been deprecated in favour of BoundedInputStream)
There was a problem hiding this comment.
I love the idea but the CSV parser is free to read ahead in the input stream on its own whim so that byte count won't be reliable. I've improved the accuracy of the original method by counting actual bytes instead of chars.
There was a problem hiding this comment.
Is it possible to specify the encoding of the file when counting the bytes of the string. At the moment the code is using the default encoding of the system which may or may not be the same as the encoding of the file...
There was a problem hiding this comment.
There is a hardcoded assumption of UTF-8 elsewhere so I've made it more explicit.
2 participants
Fixes #28