[ENG-1809] Improve .csv dialect sniffing #362
Open
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
cslzchen force-pushed the feature/improve-csv-sniffing branch from 3131b7e to 641367a Compare cslzchen changed the title cslzchen force-pushed the feature/improve-csv-sniffing branch 3 times, most recently from 1de56f0 to 60521e9 Compare cslzchen changed the title 
CSV sniffer used to sniff a fixed size of 2048 Bytes of any given file. However, this led to wrong guessed delimiter if the first row is larger than 2048 Bytes. The issue is fixed and accuracy is also improved by allowing the sniffer to adaptively sniff the 1st full row as long as it is within the max render file size.
* Updated tabular rendered tests * Wrote tests for new utility methods added to `stdlib_tools`
cslzchen force-pushed the feature/improve-csv-sniffing branch from 3ffbffa to 9e8b9fc Compare cslzchen commented Jun 5, 2020
Comment on lines +22 to +24
| # Prepare the first row for sniffing | ||
| data = fp.read(INIT_SNIFF_SIZE) | ||
| data = _trim_or_append_data(fp, data, INIT_SNIFF_SIZE, 0) |
There was a problem hiding this comment.
The initial solution (see below) didn't work 100% since for some file, sniffing the full file ended up in wrong delimiter while sniffing only the first row worked as expected. This is why even when the sniffer can sniff the full file, MFR only provides it with the first row.
... data = fp.read(TABULAR_INIT_SNIFF_SIZE) if len(data) == TABULAR_INIT_SNIFF_SIZE: data = _trim_or_append_data(fp, data, TABULAR_INIT_SNIFF_SIZE, 0) ...| :param text: the current text chunk to check the new line character | ||
| :param read_size: the last read size when `fp.read()` is called | ||
| :param size_to_sniff: the accumulated size fo the text to sniff | ||
| :param max_render_size: the max file size for render |
There was a problem hiding this comment.
Instead of using MAX_FILE_SIZE directly, the max_render_size= provides an option for unit tests to use a much smaller size.
Comment on lines +167 to +171
| index = text.find('\r\n') | ||
| if index == -1: | ||
| index = text.find('\n') | ||
| if index == -1: | ||
| index = text.find('\r') |
There was a problem hiding this comment.
I am not sure why .rfind() was used in the initial solution. One guess is that it only reads a fixed size for sniff so it wants to include as many rows as possible. However, this is no longer the case in my implementation where only the first row is sniffed.
index = text.rfind('\r\n') if index == -1: index = text.rfind('\n') if index == -1: index = text.rfind('\r') Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Add this suggestion to a batch that can be applied as a single commit. This suggestion is invalid because no changes were made to the code. Suggestions cannot be applied while the pull request is closed. Suggestions cannot be applied while viewing a subset of changes. Only one suggestion per line can be applied in a batch. Add this suggestion to a batch that can be applied as a single commit. Applying suggestions on deleted lines is not supported. You must change the existing code in this line in order to create a valid suggestion. Outdated suggestions cannot be applied. This suggestion has been applied or marked resolved. Suggestions cannot be applied from pending reviews. Suggestions cannot be applied on multi-line comments. Suggestions cannot be applied while the pull request is queued to merge. Suggestion cannot be applied right now. Please check back later.
Ticket
https://openscience.atlassian.net/browse/ENG-1809
Purpose
Improve .csv dialect sniffing.
Changes
Provide a full row of data to
csv.Sniffer().sniff()so it can effectively detect the correct dialect. MFR reads a small amount of data from the file and recursively read more until either the following happens.TabularRenderError. However, this is more like a failsafe since oversized file should never reach the sniffer in the first place.Updated existing and added new unit tests
Side effects
Memory usage for sniffing depends on the size of the first row of the file. In the worst case, it could sniff at most 10MB. Please note that we don't have enough statistical data on how many bytes the first row of a
.csvfile takes on average. However, files larger than 10MB have already failed the size check by the renderer before the sniffing starts.Given that for CSV, we needs to read the full file into memory for rendering anyway and the partial sniff data is always deleted after use, I don't think it will be a problem.
QA Notes
TBD
Deployment Notes
N / A