Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixed CSV read failure involving blank lines in RFC4180 mode. #2434

Merged
merged 2 commits into from
Oct 24, 2023

Conversation

strRM
Copy link
Contributor

@strRM strRM commented Oct 23, 2023

We noticed that blank lines in quoted columns lead to parse failures in RFC4180 mode. The fix is fairly straightforward and just requires checking for blank lines and just reading the next one.

I fixed some problems with load11 because were not testing what we wanted to test and did not use rfc4180 mode.
I also added a couple of additional tests.

strRM added 2 commits October 23, 2023 12:27
There was a problem handling blank lines in a quoted
CSV column when reading RFC4180 mode.
Instead of continuing to read more lines, it dropped out
assuming there was at least 1 character. Now, if the
line we read contains 0 characters, it immediately goes
back to read more lines.

I did not notice this before, because the test did not
use rfc4180 csv files for input or output.
This adds a test to make sure we can process
quotes within a quoted column. Quotes in quotes
need to be double-quoted.
@codecov
Copy link

codecov bot commented Oct 23, 2023

Codecov Report

Merging #2434 (b65d87a) into master (2914f7b) will increase coverage by 0.19%.
Report is 17 commits behind head on master.
The diff coverage is 72.70%.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2434      +/-   ##
==========================================
+ Coverage   77.75%   77.94%   +0.19%     
==========================================
  Files         474      479       +5     
  Lines       31164    31437     +273     
==========================================
+ Hits        24232    24505     +273     
  Misses       6932     6932              
Files Coverage Δ
src/FunctorOps.cpp 60.81% <ø> (ø)
src/ast/IterationCounter.cpp 100.00% <100.00%> (ø)
src/ast/QualifiedName.h 94.11% <ø> (ø)
src/ast/Relation.h 100.00% <100.00%> (ø)
src/ast/Term.h 75.00% <100.00%> (-15.00%) ⬇️
src/ast/analysis/PrecedenceGraph.cpp 42.85% <100.00%> (+7.72%) ⬆️
src/ast/analysis/typesystem/Type.h 50.00% <ø> (ø)
...ast/analysis/typesystem/TypeConstrainsAnalysis.cpp 99.39% <100.00%> (+0.01%) ⬆️
...c/ast/analysis/typesystem/TypeConstrainsAnalysis.h 100.00% <ø> (ø)
src/ast/transform/ComponentInstantiation.cpp 92.33% <100.00%> (+0.16%) ⬆️
... and 38 more

... and 8 files with indirect coverage changes

Copy link
Member

@quentin quentin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the fix

@quentin quentin merged commit 7353751 into souffle-lang:master Oct 24, 2023
30 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants