rows_from_file() ignores ignore_extras= and extras_key= for anything that isn't Format.CSV
Maintainers usually reply within 3 days
Assessment
- Difficulty
- 2/5
- Estimated time
- 1-3 hours
- Newbie friendliness
- 88/100
Research direction
Start in sqlite_utils/utils.py at rows_from_file() and inspect tests/test_rows_from_file.py, especially the existing CSV test at line 44. Run the rows_from_file tests, then cover the explicitly mentioned TSV and auto-detected paths while preserving the default RowError behavior. Done means ignore_extras and extras_key work consistently for those paths without changing the documented default.
Written by the indexing model from the issue text.
Description
rows_from_file() ignores ignore_extras= and extras_key= for anything that isn't explicitly format=Format.CSV.
The docstring says:
If a CSV or TSV file includes rows with more fields than are declared in the header a
sqlite_utils.utils.RowErrorexception will be raised when you loop over the generator.You can instead ignore the extra data by passing
ignore_extras=True.Or pass
extras_key="rest"to put those additional values in a list in a key calledrest.
In practice those two options only work for format=Format.CSV. For TSV — which the docstring names explicitly — and for the auto-detected path (format omitted) they are silently discarded, and the caller's own explicit request is contradicted by a RowError.
Both branches that recurse into the CSV branch call rows_from_file() without forwarding the two arguments, so the inner call uses its own defaults (ignore_extras=False, extras_key=None) and raises before the outer _extra_key_strategy() wrapper — which is dead code — ever sees the row:
>>> from io import BytesIO
>>> from sqlite_utils.utils import rows_from_file, Format
>>> rows, _ = rows_from_file(
... BytesIO(b"id\tname\r\n1\tCleo\toops"),
... format=Format.TSV,
... ignore_extras=True,
... )
>>> list(rows)
Traceback (most recent call last):
...
sqlite_utils.utils.RowError: Row {'id': '1', 'name': 'Cleo'} contained these extra values: ['oops']
The same happens with extras_key="_rest":
>>> rows, _ = rows_from_file(
... BytesIO(b"id\tname\r\n1\tCleo\toops"),
... format=Format.TSV,
... extras_key="_rest",
... )
>>> list(rows)
Traceback (most recent call last):
...
sqlite_utils.utils.RowError: Row {'id': '1', 'name': 'Cleo'} contained these extra values: ['oops']
With no options passed the behaviour is correct and should not change — RowError is still raised, as documented.
git log -S"_extra_key_strategy" points at d379f43 ("rows_from_file(... ignore_extras: bool, restkey: str), refs #440"), which introduced the strategy and both branches in one commit. The recursion has never forwarded the arguments, so this looks like an oversight in the original commit rather than a decision.
The existing test hardcoded format=Format.CSV (tests/test_rows_from_file.py:44), which is why it was never caught.
Proposed fix
Forward ignore_extras= and extras_key= to the inner call in both branches, and drop the now-redundant outer _extra_key_strategy wrapper so the strategy is applied once, on the shared CSV path that already implements it.
Note on scope
I would fix the auto-detect branch in the same commit, since it is the same root cause (same dropped-argument recursion, same dead wrapper) and would otherwise be a one-line follow-up. It interacts with #891: once the sniffer picks the right delimiter for a ragged file, the auto-detected path will honour ignore_extras too.
I did not add an auto-detect regression test, because csv.Sniffer returns the wrong delimiter on a small ragged input and pinning today's sniffing behaviour would bake in the bug #891 is about. The TSV test passes an explicit format, so it does not depend on the sniffer.
- Dominant language
- Python
- Stars
- 2.2k
- Forks
- 215
- PR merge metrics
- No merged PRs in 30d
Getting set up
- No Dockerfile or Docker Compose file
- No pull request template
- Read the contributing guide
First steps
- Read the whole issue, then the project's contributing guide.
- Comment on the issue to say you are picking it up — it saves two people doing the same work.
- Fork the repository and make your change on a branch.
- Open a pull request that references the issue number.
More from simonw/sqlite-utils
-
Difficulty 2/5 1-3 hours Newbie friendliness 84/100
simonw/sqlite-utils#928 ·
Maintainers usually reply within 3 days
-
Tests use CliRunner.isolated_filesystem(), which is deprecated and will be removed in Click 9.0Possibly taken @devpryan7792 claimed this 11 days ago. Open
Difficulty 2/5 1-3 hours Newbie friendliness 78/100
simonw/sqlite-utils#916 ·
Maintainers usually reply within 3 days
-
rows_from_file() splits a single-column CSV file on a letter, or raises csv.ErrorPossibly taken @L4XB claimed this 19 days ago. Open
Difficulty 2/5 1-3 hours Newbie friendliness 84/100
simonw/sqlite-utils#907 ·
Maintainers usually reply within 3 days
-
enable_fts() exhausts iterator columns before populating the indexPossibly taken @syahra712 claimed this 25 days ago. Open
Difficulty 2/5 1-3 hours Newbie friendliness 83/100
simonw/sqlite-utils#902 ·
Maintainers usually reply within 3 days
-
Raw numpy scalar values get silently stored as corrupted BLOBs instead of being convertedPossibly taken @TanbirRamim claimed this 43 days ago. Open
Difficulty 2/5 1-3 hours Newbie friendliness 75/100
simonw/sqlite-utils#876 ·
Maintainers usually reply within 3 days
All issues in simonw/sqlite-utils
Similar issues
-
community enhancement good first issue
Difficulty 2/5 1-3 hours Newbie friendliness 86/100
Maintainers usually reply within 1 day
-
Difficulty 2/5 1-3 hours Newbie friendliness 77/100
Hochfrequenz/sapgui.mcp#1039 ·
Maintainers usually reply within 1 day
-
Difficulty 2/5 1-3 hours Newbie friendliness 88/100
didi/helixharness#18 ·
-
Difficulty 2/5 1-3 hours Newbie friendliness 75/100
Maintainers usually reply within 1 day
-
Difficulty 2/5 1-3 hours Newbie friendliness 68/100
dataforgoodfr/democracy-watch#46 · 1 reaction ·