Repository navigation
Parse ARFF data records according to ARFF quoting rules - #532
Open
officialasishkumar wants to merge 1 commit into
Open
officialasishkumar wants to merge 1 commit into
officialasishkumar wants to merge 1 commit into
Conversation
The ARFF extractor split data records with str_getcsv() using a single
quote enclosure, which does not match how ARFF tokenizes values:
- Whitespace around values was retained, so " x" did not match the
nominal value "x" and " ?" was not recognized as missing (numeric
columns threw "Expected numeric value").
- Double-quoted values and attribute names, which Weka accepts and
tools such as R's write.arff produce, were not understood. Commas
and percent signs inside double quotes split the record or were
stripped as comments.
- Backslash escapes in quoted data values were left in place
('don\'t' became don\'t) even though the same token in an attribute
name was decoded to don't.
- A quoted '?' was treated as a missing value although only an
unquoted ? denotes a missing value in ARFF.
- An apostrophe inside an unquoted value (don't) was taken as the
start of a quoted string, causing following lines to be swallowed
until an "Unterminated quoted string" error.
Data records are now split by a dedicated values() method that ignores
whitespace around values, unquotes single- or double-quoted values
using the same rules as attribute names, and returns unquoted ? as
null so that only genuine missing values are substituted. Quoted
string parsing is shared between attribute names and data values via
a new quoted() helper, and a quote only opens a quoted string at the
start of a token in balanced(), stripComment() and values().
balanced() and stripComment() now skip over runs of ordinary
characters with strcspn() and values() has a fast path for records
without quotes, which makes extraction faster than before (about 5x
on unquoted records and 2x on quoted records in a 100k row benchmark)
while producing identical output for previously supported files.
Fixes RubixML#530
This branch has not been deployed
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
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.
Fixes #530
The ARFF extractor used
str_getcsv()with a single-quote enclosure to split data records. That doesn't follow ARFF's tokenizing rules, so valid files were imported incorrectly or rejected:1.5, xproduced" x", which doesn't match the nominal valuex, and" ?"wasn't treated as missing.write.arffproduce them.'don\'t'becamedon\'t), even though attribute names were already decoded correctly.'?'was treated as missing. In ARFF only an unquoted?means missing.don't, was taken as the start of a quoted string.Changes
values()method splits records on commas and ignores surrounding whitespace. It unquotes single- or double-quoted values and returns an unquoted?asnull, so only real missing values are replaced withNANor the placeholder.quoted()helper, so attribute names and data values follow the same rules (backslash escapes, doubled quote characters).opensQuote(): inbalanced(),stripComment()andvalues(), a quote character only opens a string at the start of a token. As before, a stray"in the middle of a value (e.g.5" tall) is a literal character.?.The existing
extractEscapedApostrophe*tests expected the backslash to be kept (don\'t). They now expectdon't, which matches how the same token is already decoded in attribute names.Performance
balanced()andstripComment()run on every line. They now usestrcspn()to skip runs of ordinary characters, andvalues()has a fast path for records without quotes. On a 100k-row file (best of 6 runs):For both benchmark files and
tests/test.arff, the output is identical tomaster.Tests
New tests cover whitespace around values, double-quoted values and attribute names (with commas,
%, apostrophes and doubled quotes inside), quoted versus unquoted?, and text after a closing quote, which raises an exception. The full suite, PHPStan and the coding style check pass.