Rewrite the parsing engine for speed (3.7x rows, 10.6x with the new iter_terms) - #113
Merged
Conversation
Closes the coverage gaps the whole-branch review found: extension data files were pinned on a single configuration and build_archive's extension branch was never exercised, and random-access coverage was incidental to iteration (today DwCAReader.next() is implemented as get_row_by_position, which the upcoming streaming rewrite changes). Also fixes comparing an unlinked CoreRow, which started raising AttributeError once rows became hashable, and tightens the benchmark harness so it works on archives of any size.
DataFileDescriptor had no __eq__, so it compared by object identity. Row equality embeds the descriptor, which meant rows read from two DwCAReader instances over the same archive never compared equal even when their data, raw_fields, id, position and rowtype were identical. Also fixes comparing a CoreRow to an ExtensionRow, or either to a non-row object, which raised AttributeError: __key is name-mangled, so other.__key() resolves to other._CoreRow__key(), which those objects do not have.
Fix benchmarks/bench_reader.py, which Task 4 broke: it read the lazy _line_offsets attribute directly instead of going through the _get_line_offsets() accessor that builds it. Re-measure the phase's before/after numbers back to back in one sitting (machine variance between sessions has been observed near 40 percent) and record all four measurements in benchmarks/README.md. Update the CHANGES.txt speedup claim to the measured figure, and drop the stale TODO comment on CSVDataFile.
Group the 17 accumulated v0.17.0 entries into New/Performance/Fixed/Changed/ Documentation runs, matching the file's existing flat-list-with-inline-label style rather than inventing a subheading convention it never used. Fix the iter_terms() speedup claim to the measured figures (2.9x vs the row API, 10x vs the pre-rewrite implementation) instead of an unmeasured "twice as fast", and drop the close()-after-iteration entry's directory-only qualifier, which does not hold: the same fix also applies to zip/tgz archives.
…eanups - FieldPlan.term_getter() iterated its terms argument three times, so a one-shot iterable (e.g. a generator) silently produced empty tuples on every row after the first pass. Normalize to a list once up front. - CHANGES.txt: remove an unreleased-section entry describing a bug that never affected a released version (already correct on 0.16.4), and reword another entry that mischaracterized an intra-branch regression as a disagreement between iteration and random access, when the real released-version-relative change is that both now work correctly. - doc/index.rst: update the claimed minimum Python version from 3.5 to 3.8, matching setup.py's python_requires. - Tighten iter_terms() return type hints to Iterator[Tuple[str, ...]] in dwca/read.py and dwca/files.py, and add a trailing newline to requirements-dev.txt.
Verified against 0.16.4: a field containing the line terminator was read as truncated by both iteration and random access, and the exception surfaced on a later, unrelated row rather than on the affected one.
…oreid TODOs - DwCAReader: document the skip_metadata constructor parameter, including that it leaves metadata as None without affecting source_metadata. - CSVDataFile.next(): add the docstring, matching DwCAReader.next(). - DwCAReader.rows: replace the ordering TODO with the guarantee, now decided, tested and documented elsewhere (tutorial.rst, test_iterate_order, test_rows_property). - CSVDataFile.get_all_rows_by_coreid: replace both TODOs with a description of its behavior on extension vs core files, pinned by test_coreid_index.
actions/cache v2 is now hard-failed by GitHub, and checkout and setup-python were both on v2 as well. setup-python v5 caches pip itself, so the manual cache step goes away entirely, and with it the deprecated ::set-output call it needed. The old cache key hashed requirements.txt, which this project does not have, so it never varied with dependencies; the new one hashes requirements-dev.txt and setup.py.
Archives routinely declare linesTerminatedBy="\n" while the data file itself has CRLF line endings - git checks text files out that way on Windows, so an archive committed to a repository hits this without anyone choosing it. The csv module the streaming engine replaced dropped the stray CR for us. Splitting on the separator does not, so the last field of every row kept it. Both the streaming and the random-access paths now strip it, which restores what the old engine returned and keeps the two paths in agreement. No changelog entry: this repairs a regression introduced and fixed within this unreleased branch, so no released version ever behaved this way.
pandas publishes no PyPy wheels, so pip built numpy from source and its C++ failed to compile on the macOS runner, taking the whole job down at install time. pandas has been an optional dependency of this library for a long time - pd_read() raises a clear ImportError without it, and v0.16.4 fixed a regression that made it mandatory - but nothing in CI ever verified that claim, because the test module imported pandas unconditionally and failed at collection. The import is now guarded and the nine tests that genuinely exercise pd_read() skip themselves when pandas is absent. test_pd_read_pandas_unavailable still runs, since it is the one that checks the no-pandas behavior. PyPy therefore gives the optional-dependency path its only CI coverage instead of failing to install.
The previous commit appended requirements-ci.txt to a file whose last line had no newline, merging the two into ".tmp/requirements-ci.txt" and silently un-ignoring the .tmp/ directory the test suite creates.
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.
Rewrites the DwC-A parsing engine for speed, behind a characterization test suite built first.
Motivation: on a real import pipeline, parsing was about 70% of total time.
Results
Measured back to back on one machine, 400k rows, reading 14 of 50 columns:
for row in dwca)iter_terms()Opening an archive is now effectively free: the line offset index is built on first
random access rather than eagerly for every data file.
benchmarks/holds the harness and the recorded before/after. Absolute timings varyby up to ~40% between sessions on the same code, so only same-run ratios are meaningful;
the README says so explicitly.
What was actually slow
Not the CSV looping, and not the seek-based random access. Iterating an archive
instantiated a fresh
csv.readerfor every line and rebuilt the term-to-valuemapping per row. Together that was 78% of the profile. A control experiment (streaming
the file into the old
Rowclass) gave no speedup at all, which is what pointed at thereal cause.
How it works now
FieldPlan).physical lines, so a quoted field containing the line terminator no longer
desynchronises it.
DwCAReader.__iter__returns a fresh generator, so iterations are independent.iter_terms()yields a tuple per row for chosen terms, skipping theRowobject and its dict.
"id"and"coreid"request the key columns, using the samenames
headers()andpd_read()already use.Behavior changes reviewers should know about
All are documented in CHANGES.txt. The engine ones were each pinned by a
characterization test that was deliberately inverted when the fix landed, so every one
is a visible diff rather than a silent change:
ignoreHeaderLinesabove 1 skipped only one line when iterating aCSVDataFile,leaking header rows into
coreid_indexandorphaned_extension_rows.fieldsEnclosedBycharacter had thatcharacter stripped.
get_corerow_by_id()inside a loop never terminated.
paths, with following rows misaligned.
DwCAReader.next()now keeps an iterator independent of anyforloop, and starts anew pass once exhausted. Iterate over the reader instead.
Also fixed, independent of the engine:
headersdropped the column at index 0 formetafile-less archives;
hash()raisedTypeErroron every row; rows from two readersover the same archive never compared equal; comparing a
CoreRowto anExtensionRowraised
AttributeError.Packaging
dwca.star_recordimportedtyping_extensions, whichsetup.pynever declared, so apip-installed package raised
ImportError. It now usestyping.Literal, which removesthe dependency and raises the floor to Python 3.8. The CI matrix drops 3.7 in the same
commit, and
python_requiresplus the classifiers now match it.Testing
108 tests to 221. The suite was built before the rewrite specifically so the engine
could be replaced with evidence rather than hope, covering axes the existing fixtures
never touched: encodings, line terminators, quoting, header counts, ragged and blank
rows, extension files, and iteration semantics.
Two defects were found by fuzzing rather than by tests, because no sample archive in the
repo contains a newline inside a quoted field:
CoreRow.extensionscouldreturn silently truncated data. A 400-archive differential fuzz (iteration vs random
access vs a
csv.readerground truth) now passes 400/400.csv.readerover 2000 generatedarchives at five chunk sizes before being written into the code.
Not included
v0.17.0 (unreleased)- cutting the release is the maintainer's call.pd_read()still does not forwardencodingorquotechar, so a non-UTF-8 data fileworks through the row API but fails through pandas. Pre-existing, documented in the
pandas tutorial, and a behavior change worth deciding separately.
scoped out; it needs lazy per-member extraction to keep
absolute_temporary_path()working.