[CSV Sniffer] Tweaking header detection #10714
Merged
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
Collaborator
Author
|
Edit: I changed a lot of tests to remove now unnecessary |
Mytherin
reviewed
Feb 23, 2024
Collaborator
Mytherin
left a comment
There was a problem hiding this comment.
Thanks for the PR! Looks good - some minor comments:
| # conversion is skipped if we don't read the value - so even with the incorrect type specified this still works | ||
| query I | ||
| SELECT l_returnflag FROM read_csv('test/sql/copy/csv/data/real/lineitem_sample.csv', delim='|', columns={'l_orderkey': 'INTEGER','l_partkey': 'INTEGER','l_suppkey': 'INTEGER','l_linenumber': 'INTEGER','l_quantity': 'INTEGER','l_extendedprice': 'DECIMAL(15,2)','l_discount': 'DECIMAL(15,2)','l_tax': 'DECIMAL(15,2)','l_returnflag': 'VARCHAR','l_linestatus': 'VARCHAR','l_shipdate': 'DATE','l_commitdate': 'DATE','l_receiptdate': 'DATE','l_shipinstruct': 'DATE','l_shipmode': 'VARCHAR','l_comment': 'VARCHAR'}); | ||
| SELECT l_returnflag FROM read_csv('test/sql/copy/csv/data/real/lineitem_sample.csv', delim='|', columns={'l_orderkey': 'INTEGER','l_partkey': 'INTEGER','l_suppkey': 'INTEGER','l_linenumber': 'INTEGER','l_quantity': 'INTEGER','l_extendedprice': 'DECIMAL(15,2)','l_discount': 'DECIMAL(15,2)','l_tax': 'DECIMAL(15,2)','l_returnflag': 'VARCHAR','l_linestatus': 'VARCHAR','l_shipdate': 'DATE','l_commitdate': 'DATE','l_receiptdate': 'DATE','l_shipinstruct': 'DATE','l_shipmode': 'VARCHAR','l_comment': 'VARCHAR'}, header = 0); |
Collaborator
There was a problem hiding this comment.
Is this necessary here - shouldn't DetectHeaderWithSetColumn figure this one out?
Collaborator
Author
There was a problem hiding this comment.
Not really. The problem with these tests is that the types of the columns are set incorrectly, which causes the conversion test to be skipped if we don't read the value. However, this will lead to inconsistency in the row because the cast will fail, and it breaks the header detection.
Collaborator
|
Thanks! |
github-actions bot
pushed a commit
to duckdb/duckdb-r
that referenced
this pull request
Mar 15, 2024
Merge pull request duckdb/duckdb#10714 from pdet/header_default_true Merge pull request duckdb/duckdb#10840 from ahuarte47/main_add-version-parts
2 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
This change affects how header detection works for CSV files. In the previous algorithm, we preferred false negatives over false positives, leading us to miss headers in many different CSV files. For example
The issue here is that most sane CSV files actually do have a header. I then changed the algorithm to always detect these cases correctly.
This change increases our accuracy in many of our tests. I believe that the only situation where our header detection will fail is when we have an all-varchar CSV file where the first row is not a header. For example:
Because all columns of the CSV File are varchar, we will wrongfully detect
Pedro;~29as the header.cc: @tdoehmen