From ddd46eeb9c642a3a42b6de380439a66deb7caf9b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Nicolas=20No=C3=A9?= Date: Tue, 28 Jul 2026 12:51:30 +0200 Subject: [PATCH 01/33] Add a synthetic archive builder for characterization tests --- dwca/test/archive_builder.py | 188 +++++++++++++++++++++++++++++ dwca/test/test_characterization.py | 41 +++++++ 2 files changed, 229 insertions(+) create mode 100644 dwca/test/archive_builder.py create mode 100644 dwca/test/test_characterization.py diff --git a/dwca/test/archive_builder.py b/dwca/test/archive_builder.py new file mode 100644 index 0000000..ac90927 --- /dev/null +++ b/dwca/test/archive_builder.py @@ -0,0 +1,188 @@ +"""Build synthetic Darwin Core Archives for tests. + +Produces a directory-based archive (DwCAReader reads directories directly, so nothing +needs zipping). Every parameter that the Metafile can express is explicit, which is what +lets the characterization tests cover encodings, terminators, quoting and header counts +without committing more binary sample files. +""" + +import os +import shutil +import tempfile +from xml.sax.saxutils import quoteattr + +TERM_PREFIX = "http://rs.tdwg.org/dwc/terms/" + + +def temp_archive_dir(test_case): + """Return a temporary directory that is removed when `test_case` finishes. + + Several tests in the suite count the entries in the system temporary directory, so + leaking one per test would eventually make them flaky. + """ + directory = tempfile.mkdtemp() + test_case.addCleanup(shutil.rmtree, directory, ignore_errors=True) + return directory + +METAFILE_TEMPLATE = """ + + occurrence.txt + +{fields} + +{extension} + +""" + +EXTENSION_TEMPLATE = """ + extension.txt + +{fields} + """ + + +def _escape(raw): + # The Metafile stores separators escaped, e.g. the tab character as the two + # characters backslash-t. Mirror what real archives contain. + escaped = ( + raw.replace("\\", "\\\\") + .replace("\t", "\\t") + .replace("\n", "\\n") + .replace("\r", "\\r") + ) + return quoteattr(escaped) + + +def _field_tags(terms, defaults, indent=" "): + lines = [] + for index, term in enumerate(terms): + default = (defaults or {}).get(term) + if default is None: + lines.append( + '{i}'.format(i=indent, n=index, t=term) + ) + else: + lines.append( + '{i}'.format( + i=indent, n=index, t=term, d=quoteattr(default) + ) + ) + for term, default in (defaults or {}).items(): + if term not in terms: + lines.append( + '{i}'.format( + i=indent, t=term, d=quoteattr(default) + ) + ) + return "\n".join(lines) + + +def _render_rows(rows, fields_terminated_by, lines_terminated_by, fields_enclosed_by): + out = [] + for row in rows: + if fields_enclosed_by: + cells = [ + fields_enclosed_by + + cell.replace(fields_enclosed_by, fields_enclosed_by * 2) + + fields_enclosed_by + for cell in row + ] + else: + cells = list(row) + out.append(fields_terminated_by.join(cells)) + return lines_terminated_by.join(out) + (lines_terminated_by if out else "") + + +def build_archive( + directory, + rows, + columns=None, + encoding="utf-8", + lines_terminated_by="\n", + fields_terminated_by="\t", + fields_enclosed_by="", + ignore_header_lines=0, + header_rows=(), + id_index=0, + terms=None, + defaults=None, + extension=None, + trailing_newline=True, + raw_payload=None, +): + """Write a Darwin Core Archive into `directory` and return its path. + + :param rows: list of lists of str, the data rows. + :param terms: list of full term URIs, one per column. Defaults to term0, term1, ... + :param defaults: dict term -> default value. A term not present in `terms` becomes a + default-only field (no index attribute), as the standard allows. + :param extension: list of rows for an extension data file, or None. Column 0 is the coreid. + :param raw_payload: bytes written verbatim as the core data file instead of `rows`. + Used to build inputs no well-formed writer would produce (undecodable bytes, ragged + rows, blank lines). + """ + if columns is None: + columns = max([len(r) for r in rows] + [1]) if rows else 1 + if terms is None: + terms = [TERM_PREFIX + "term" + str(i) for i in range(columns)] + + metafile = METAFILE_TEMPLATE.format( + encoding=quoteattr(encoding), + fields_terminated_by=_escape(fields_terminated_by), + lines_terminated_by=_escape(lines_terminated_by), + fields_enclosed_by=_escape(fields_enclosed_by), + ignore_header_lines=quoteattr(str(ignore_header_lines)), + id_index=id_index, + fields=_field_tags(terms, defaults), + extension=( + "" + if extension is None + else EXTENSION_TEMPLATE.format( + encoding=quoteattr(encoding), + fields_terminated_by=_escape(fields_terminated_by), + lines_terminated_by=_escape(lines_terminated_by), + fields_enclosed_by=_escape(fields_enclosed_by), + ignore_header_lines=quoteattr(str(ignore_header_lines)), + fields=_field_tags( + [TERM_PREFIX + "vernacularName"], None, indent=" " + ).replace('index="0"', 'index="1"'), + ) + ), + ) + + with open(os.path.join(directory, "meta.xml"), "w", encoding="utf-8") as f: + f.write(metafile) + + data_path = os.path.join(directory, "occurrence.txt") + if raw_payload is not None: + with open(data_path, "wb") as f: + f.write(raw_payload) + else: + payload = _render_rows( + list(header_rows) + list(rows), + fields_terminated_by, + lines_terminated_by, + fields_enclosed_by, + ) + if not trailing_newline and payload.endswith(lines_terminated_by): + payload = payload[: -len(lines_terminated_by)] + with open(data_path, "w", encoding=encoding, newline="") as f: + f.write(payload) + + if extension is not None: + payload = _render_rows( + list(header_rows) + list(extension), + fields_terminated_by, + lines_terminated_by, + fields_enclosed_by, + ) + with open( + os.path.join(directory, "extension.txt"), "w", encoding=encoding, newline="" + ) as f: + f.write(payload) + + return directory diff --git a/dwca/test/test_characterization.py b/dwca/test/test_characterization.py new file mode 100644 index 0000000..9f72a37 --- /dev/null +++ b/dwca/test/test_characterization.py @@ -0,0 +1,41 @@ +"""Characterization tests: these pin CURRENT behavior so a rewrite of the parsing +engine produces a visible diff rather than a silent change. + +Tests marked with a `# CHARACTERIZATION: wrong, see B` comment assert behavior we +know to be incorrect. Phase 1 changes them deliberately. +""" + +import unittest + +import pytest + +from dwca.exceptions import InvalidArchive +from dwca.read import DwCAReader + +from .archive_builder import build_archive, temp_archive_dir + +TERM0 = "http://rs.tdwg.org/dwc/terms/term0" +TERM1 = "http://rs.tdwg.org/dwc/terms/term1" + + +class TestArchiveBuilder(unittest.TestCase): + def test_builds_a_readable_archive(self): + path = build_archive( + temp_archive_dir(self), rows=[["1", "Borneo"], ["2", "Mumbai"]] + ) + + with DwCAReader(path) as dwca: + rows = list(dwca) + + assert 2 == len(rows) + assert "1" == rows[0].id + assert "Borneo" == rows[0].data[TERM1] + assert "Mumbai" == rows[1].data[TERM1] + + def test_raw_payload_is_written_verbatim(self): + path = build_archive( + temp_archive_dir(self), rows=[], columns=2, raw_payload=b"1\tBorneo\n" + ) + + with DwCAReader(path) as dwca: + assert ["Borneo"] == [row.data[TERM1] for row in dwca] From 6615b0ca5fdbd260ea5763e96377830748e321b2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Nicolas=20No=C3=A9?= Date: Tue, 28 Jul 2026 12:56:21 +0200 Subject: [PATCH 02/33] Characterize header lines, line terminators and encodings --- dwca/test/test_characterization.py | 163 +++++++++++++++++++++++++++++ 1 file changed, 163 insertions(+) diff --git a/dwca/test/test_characterization.py b/dwca/test/test_characterization.py index 9f72a37..323c79d 100644 --- a/dwca/test/test_characterization.py +++ b/dwca/test/test_characterization.py @@ -39,3 +39,166 @@ def test_raw_payload_is_written_verbatim(self): with DwCAReader(path) as dwca: assert ["Borneo"] == [row.data[TERM1] for row in dwca] + + +class TestHeaderLines(unittest.TestCase): + """ignoreHeaderLines, for BOTH access paths. + + The two paths disagree today: DwCAReader iteration goes through get_row_by_position() + (which offsets correctly), while CSVDataFile.__iter__ uses readlines(hint) where the + argument is a byte-size hint rather than a line count. coreid_index is built from the + second path, so it picks up leftover header lines. + """ + + def _archive(self, ignore_header_lines, header_rows): + return build_archive( + temp_archive_dir(self), + rows=[["1", "Borneo"], ["2", "Mumbai"]], + ignore_header_lines=ignore_header_lines, + header_rows=header_rows, + ) + + def test_no_header(self): + path = self._archive(0, []) + + with DwCAReader(path) as dwca: + assert ["1", "2"] == [row.id for row in dwca] + assert "1" == dwca.core_file.get_row_by_position(0).id + + def test_one_header(self): + path = self._archive(1, [["id", "locality"]]) + + with DwCAReader(path) as dwca: + assert ["1", "2"] == [row.id for row in dwca] + assert "1" == dwca.core_file.get_row_by_position(0).id + assert {"1": [0], "2": [1]} == { + k: list(v) for k, v in dwca.core_file.coreid_index.items() + } + + def test_two_headers_iteration_and_random_access_agree(self): + path = self._archive(2, [["idA", "locA"], ["idB", "locB"]]) + + with DwCAReader(path) as dwca: + assert ["1", "2"] == [row.id for row in dwca] + assert "1" == dwca.core_file.get_row_by_position(0).id + + # CHARACTERIZATION: wrong, see B1. readlines() takes a byte-size hint, not a + # line count, so the second header line leaks into the index as a data row. + assert {"idB": [0], "1": [1], "2": [2]} == { + k: list(v) for k, v in dwca.core_file.coreid_index.items() + } + + +class TestLineTerminators(unittest.TestCase): + def test_unix_terminator(self): + path = build_archive( + temp_archive_dir(self), + rows=[["1", "Borneo"], ["2", "Mumbai"]], + lines_terminated_by="\n", + ) + + with DwCAReader(path) as dwca: + assert ["Borneo", "Mumbai"] == [row.data[TERM1] for row in dwca] + + def test_dos_terminator(self): + path = build_archive( + temp_archive_dir(self), + rows=[["1", "Borneo"], ["2", "Mumbai"]], + lines_terminated_by="\r\n", + ) + + with DwCAReader(path) as dwca: + assert ["Borneo", "Mumbai"] == [row.data[TERM1] for row in dwca] + + def test_multichar_terminator_is_rejected_by_python_io(self): + path = build_archive( + temp_archive_dir(self), + rows=[["1", "Borneo"]], + lines_terminated_by="@@\n", + ) + + # io.open() only accepts None, '', '\n', '\r' and '\r\n' as newline, so an archive + # declaring anything else cannot be opened at all. Pinned so a rewrite that moves + # off the newline= parameter has to decide what to do about it. + with pytest.raises(ValueError): + DwCAReader(path) + + def test_unicode_next_line_does_not_split_a_row(self): + """U+0085 must not be treated as a line break (issue #20). + + str.splitlines() splits on U+0085 but io line iteration does not, so any rewrite + using splitlines() would silently double the row count here. + """ + path = build_archive( + temp_archive_dir(self), + rows=[], + columns=2, + raw_payload=b"1\tbefore\xc2\x85after\n", # U+0085 encoded as UTF-8 + ) + + with DwCAReader(path) as dwca: + rows = list(dwca) + + assert 1 == len(rows) + assert "before\u0085after" == rows[0].data[TERM1] + + +class TestEncodings(unittest.TestCase): + def test_windows1252_data_file(self): + path = build_archive( + temp_archive_dir(self), + rows=[["1", "caf\xe9"], ["2", "na\xefve"]], + encoding="windows-1252", + ) + + with DwCAReader(path) as dwca: + assert ["caf\xe9", "na\xefve"] == [row.data[TERM1] for row in dwca] + + def test_latin1_data_file(self): + path = build_archive( + temp_archive_dir(self), rows=[["1", "caf\xe9"]], encoding="latin-1" + ) + + with DwCAReader(path) as dwca: + assert ["caf\xe9"] == [row.data[TERM1] for row in dwca] + + def test_utf8_bom_leaks_into_the_first_field(self): + path = build_archive( + temp_archive_dir(self), + rows=[], + columns=2, + raw_payload=b"\xef\xbb\xbf1\tBorneo\n", # UTF-8 byte order mark + ) + + with DwCAReader(path) as dwca: + rows = list(dwca) + + # CHARACTERIZATION: the encoding is "utf-8", not "utf-8-sig", so the byte order + # mark becomes part of the id. A rewrite adding BOM handling changes this + # deliberately. + assert "\ufeff1" == rows[0].id + + def test_undecodable_byte_desynchronises_random_access(self): + path = build_archive( + temp_archive_dir(self), + rows=[], + columns=2, + raw_payload=b"1\tcaf\xe9\n2\tMumbai\n3\tBorneo\n", + ) + + with DwCAReader(path) as dwca: + # Row 0 is fine: the offset index has not drifted yet. + assert "caf\ufffd" == dwca.core_file.get_row_by_position(0).data[TERM1] + + # CHARACTERIZATION: wrong, see B2. errors="replace" turns the undecodable byte + # into U+FFFD, which re-encodes to three bytes instead of one, so every offset + # after it is two bytes too large. The seek lands mid-row and the truncated row + # no longer has enough columns. + with pytest.raises(InvalidArchive): + dwca.core_file.get_row_by_position(1) + + # CHARACTERIZATION: wrong, see B2. Iteration is broken too, because today it is + # implemented as repeated get_row_by_position() calls. After Phase 1 this reads + # ["caf\ufffd", "Mumbai", "Borneo"], which is the whole point of the fix. + with pytest.raises(InvalidArchive): + list(dwca) From 3995f37edc8c4b936bbb2c3946f262fe357b7cf2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Nicolas=20No=C3=A9?= Date: Tue, 28 Jul 2026 13:01:32 +0200 Subject: [PATCH 03/33] Characterize quoting, degenerate rows and iteration semantics --- dwca/test/test_characterization.py | 162 +++++++++++++++++++++++++++++ 1 file changed, 162 insertions(+) diff --git a/dwca/test/test_characterization.py b/dwca/test/test_characterization.py index 323c79d..3e59865 100644 --- a/dwca/test/test_characterization.py +++ b/dwca/test/test_characterization.py @@ -202,3 +202,165 @@ def test_undecodable_byte_desynchronises_random_access(self): # ["caf\ufffd", "Mumbai", "Borneo"], which is the whole point of the fix. with pytest.raises(InvalidArchive): list(dwca) + + +class TestQuoting(unittest.TestCase): + def _read_localities(self, payload): + path = build_archive( + temp_archive_dir(self), + rows=[], + columns=2, + fields_terminated_by=",", + fields_enclosed_by='"', + raw_payload=payload, + ) + + with DwCAReader(path) as dwca: + return [row.data[TERM1] for row in dwca] + + def test_delimiter_inside_a_quoted_field(self): + """Regression guard for the v0.11.0 fix. Any hand-rolled parser breaks this.""" + assert ["plain, with comma"] == self._read_localities( + b'"1","plain, with comma"\n' + ) + + def test_quote_in_the_middle_of_content(self): + assert ['say "hi" there'] == self._read_localities(b'"1","say ""hi"" there"\n') + + def test_quote_at_the_edge_of_content_is_eaten(self): + # CHARACTERIZATION: wrong, see B3. csv parses this correctly to 'say "hi"', then + # the trailing .strip(fields_enclosed_by) removes the legitimate closing quote. + assert ['say "hi'] == self._read_localities(b'"1","say ""hi"""\n') + + # Same at the start of the field. + assert ['hi" she said'] == self._read_localities(b'"1","""hi"" she said"\n') + + def test_quote_characters_are_kept_when_the_archive_declares_no_enclosure(self): + path = build_archive( + temp_archive_dir(self), + rows=[], + columns=2, + raw_payload=b'1\t"betta" splendens\n', + ) + + with DwCAReader(path) as dwca: + assert '"betta" splendens' == list(dwca)[0].data[TERM1] + + +class TestDegenerateRows(unittest.TestCase): + def test_row_with_fewer_columns_than_declared(self): + path = build_archive( + temp_archive_dir(self), rows=[], columns=3, raw_payload=b"1\tBorneo\n" + ) + + with DwCAReader(path) as dwca: + with pytest.raises(InvalidArchive): + list(dwca) + + def test_row_with_more_columns_than_declared(self): + path = build_archive( + temp_archive_dir(self), + rows=[], + columns=2, + raw_payload=b"1\tBorneo\textra\n", + ) + + with DwCAReader(path) as dwca: + rows = list(dwca) + + # Extra columns are ignored by data but kept in raw_fields. + assert "Borneo" == rows[0].data[TERM1] + assert ["1", "Borneo", "extra"] == rows[0].raw_fields + + def test_blank_line_in_the_middle(self): + path = build_archive( + temp_archive_dir(self), + rows=[], + columns=2, + raw_payload=b"1\tBorneo\n\n2\tMumbai\n", + ) + + with DwCAReader(path) as dwca: + with pytest.raises(InvalidArchive): + list(dwca) + + def test_no_trailing_newline_at_eof(self): + path = build_archive( + temp_archive_dir(self), + rows=[], + columns=2, + raw_payload=b"1\tBorneo\n2\tMumbai", + ) + + with DwCAReader(path) as dwca: + assert ["Borneo", "Mumbai"] == [row.data[TERM1] for row in dwca] + + def test_empty_core_file(self): + path = build_archive( + temp_archive_dir(self), rows=[], columns=2, raw_payload=b"" + ) + + with DwCAReader(path) as dwca: + assert [] == list(dwca) + assert [] == dwca.rows + + def test_header_only_core_file(self): + path = build_archive( + temp_archive_dir(self), + rows=[], + columns=2, + ignore_header_lines=1, + raw_payload=b"id\tlocality\n", + ) + + with DwCAReader(path) as dwca: + assert [] == list(dwca) + + +class TestIterationSemantics(unittest.TestCase): + def _archive(self): + return build_archive( + temp_archive_dir(self), + rows=[["1", "Borneo"], ["2", "Mumbai"], ["3", "Paris"]], + ) + + def test_sequential_re_iteration(self): + with DwCAReader(self._archive()) as dwca: + assert 3 == len(list(dwca)) + assert 3 == len(list(dwca)) + + def test_nested_iteration(self): + with DwCAReader(self._archive()) as dwca: + pairs = [] + for outer in dwca: + for inner in dwca: + pairs.append((outer.id, inner.id)) + if len(pairs) > 20: + break + + # CHARACTERIZATION: wrong, see B4. DwCAReader is its own iterator with one shared + # pointer, so the inner loop consumes it and the outer loop ends after one pass. + assert 3 == len(pairs) + + def test_lookup_inside_a_loop(self): + seen = [] + with DwCAReader(self._archive()) as dwca: + for row in dwca: + seen.append(row.id) + dwca.get_corerow_by_id("2") + if len(seen) > 8: + break + + # CHARACTERIZATION: wrong, see B4. get_corerow_by_id() resets the shared pointer, + # so this never terminates. Without the break it would loop forever. + assert len(seen) > 3 + + def test_random_access_interleaved_with_iteration_is_safe(self): + with DwCAReader(self._archive()) as dwca: + seen = [] + for row in dwca: + seen.append(row.id) + # This path is offset-based, so it does not disturb iteration. + assert "1" == dwca.core_file.get_row_by_position(0).id + + assert ["1", "2", "3"] == seen From da6011a864c41df5405124d1d6cf7cdbcde03839 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Nicolas=20No=C3=A9?= Date: Tue, 28 Jul 2026 13:07:48 +0200 Subject: [PATCH 04/33] Fix headers dropping the column at index 0 --- CHANGES.txt | 6 ++++++ dwca/descriptors.py | 6 +++--- dwca/test/test_descriptors.py | 10 ++++++++++ dwca/test/test_dwcareader.py | 10 ++++++++-- 4 files changed, 27 insertions(+), 5 deletions(-) diff --git a/CHANGES.txt b/CHANGES.txt index 2f8749e..7ccde18 100644 --- a/CHANGES.txt +++ b/CHANGES.txt @@ -1,3 +1,9 @@ +0.17.0 (unreleased) +------------------- + +- Fixed: DataFileDescriptor.headers dropped the column at index 0 for archives without a + metafile, which also made pd_read() promote that column to the DataFrame index. + v0.16.4 (2024-10-18) -------------------- diff --git a/dwca/descriptors.py b/dwca/descriptors.py index 5623669..491f632 100644 --- a/dwca/descriptors.py +++ b/dwca/descriptors.py @@ -229,9 +229,9 @@ def headers(self) -> List[str]: columns = {} for f in self.fields: - if f[ - "index" - ]: # Some (default values for example) don't have a corresponding col. + # Some fields (those carrying only a default value) have no column. Note the + # explicit None test: index 0 is a valid column and must not be dropped. + if f["index"] is not None: columns[f["index"]] = f["term"] # In addition to DwC terms, we may also have id (Core) or core_id (Extensions) columns diff --git a/dwca/test/test_descriptors.py b/dwca/test/test_descriptors.py index a16e702..62df868 100644 --- a/dwca/test/test_descriptors.py +++ b/dwca/test/test_descriptors.py @@ -594,3 +594,13 @@ def test_exposes_metadata_filename(self): descriptor = dwca.descriptor assert descriptor.metadata_filename == "eml.xml" + + +class TestHeadersIndexZero(unittest.TestCase): + def test_column_at_index_zero_is_not_dropped(self): + """A metafile-less archive has no id_index, so column 0 comes only from fields.""" + with DwCAReader(sample_data_path("dwca-simple-csv.zip")) as dwca: + descriptor = dwca.core_file.file_descriptor + + assert len(descriptor.fields) == len(descriptor.headers) + assert "gbifid" == descriptor.headers[0] diff --git a/dwca/test/test_dwcareader.py b/dwca/test/test_dwcareader.py index 8484670..c295a1b 100644 --- a/dwca/test/test_dwcareader.py +++ b/dwca/test/test_dwcareader.py @@ -124,10 +124,16 @@ def test_pd_read_utf8_eol_ignored(self): def test_pd_read_simple_csv(self): with DwCAReader(sample_data_path("dwca-simple-csv.zip")) as dwca: df = dwca.pd_read("0008333-160118175350007.csv") - # Ensure we get the correct number of rows + # Ensure we get the correct number of rows and columns assert 3 == df.shape[0] - # Ensure we can access arbitrary data + assert 42 == df.shape[1] + # This archive has no metafile, so the first column (gbifid) has no id_index to + # rely on. It must come from the headers list as a regular column, not be silently + # promoted to the DataFrame index by pandas because of a missing header name. + assert "gbifid" in df.columns + assert df.index.name is None + # Ensure we can access arbitrary data assert df["decimallatitude"].values.tolist()[1] == -31.98333 From 700e9433416a30a20f966e99588c9ce9d1bc70c6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Nicolas=20No=C3=A9?= Date: Tue, 28 Jul 2026 13:15:51 +0200 Subject: [PATCH 05/33] Fix hash() raising TypeError on rows --- CHANGES.txt | 2 ++ dwca/rows.py | 7 +++++-- dwca/test/test_rows.py | 31 +++++++++++++++++++++++++++++++ 3 files changed, 38 insertions(+), 2 deletions(-) diff --git a/CHANGES.txt b/CHANGES.txt index 7ccde18..91cdaac 100644 --- a/CHANGES.txt +++ b/CHANGES.txt @@ -3,6 +3,8 @@ - Fixed: DataFileDescriptor.headers dropped the column at index 0 for archives without a metafile, which also made pd_read() promote that column to the DataFrame index. +- Fixed: hash() on a CoreRow or an ExtensionRow raised TypeError. Rows have been documented + as hashable since 0.3.3 but never were. v0.16.4 (2024-10-18) -------------------- diff --git a/dwca/rows.py b/dwca/rows.py index 5b7a4b0..fef8427 100644 --- a/dwca/rows.py +++ b/dwca/rows.py @@ -201,7 +201,10 @@ def __ne__(self, other): return not self.__eq__(other) def __hash__(self): - return hash(self.__key()) + # __key() embeds the data dict, the raw field list and the lazily-loaded extensions, + # none of which are hashable. Equal rows still hash equally because this is a subset + # of the equality key. + return hash((self.descriptor, self.id, self.rowtype, self.position)) class ExtensionRow(Row): @@ -241,7 +244,7 @@ def __ne__(self, other): return not self.__eq__(other) def __hash__(self): - return hash(self.__key()) + return hash((self.descriptor, self.core_id, self.rowtype, self.position)) def csv_line_to_fields(csv_line, line_ending, field_ending, fields_enclosed_by): diff --git a/dwca/test/test_rows.py b/dwca/test/test_rows.py index 543ff61..2716185 100644 --- a/dwca/test/test_rows.py +++ b/dwca/test/test_rows.py @@ -47,3 +47,34 @@ def test_position(self): assert 0 == vernacular_first_line.position assert 1 == vernacular_second_line.position assert 2 == vernacular_third_line.position + + +class TestRowHashing(unittest.TestCase): + def test_core_rows_are_hashable(self): + """CHANGES.txt has claimed rows are hashable since 0.3.3, but __key() embedded a + dict and a list, so hash() raised TypeError for every row.""" + with DwCAReader(sample_data_path("dwca-2extensions.zip")) as dwca: + rows = dwca.rows + + assert len(set(rows)) == len(rows) + + def test_extension_rows_are_hashable(self): + with DwCAReader(sample_data_path("dwca-2extensions.zip")) as dwca: + extension_rows = dwca.rows[0].extensions + + assert len(set(extension_rows)) == len(extension_rows) + + def test_equal_rows_hash_equally(self): + # Uses two independently-fetched (but equal) CoreRow instances from the *same* reader, + # rather than from two separate readers. DataFileDescriptor has no __eq__ of its own, so + # instances from two separate readers never compare equal (identity-based comparison) - + # that is a pre-existing bug unrelated to hashing, out of scope for this fix. Within a + # single reader, descriptor is the same shared instance, so this still genuinely exercises + # "two distinct objects that compare equal must hash equal". + with DwCAReader(sample_data_path("dwca-2extensions.zip")) as dwca: + one = dwca.rows[0] + two = dwca.rows[0] + + assert one is not two + assert one == two + assert hash(one) == hash(two) From 771593134599074cc4ff804790065c3519783e51 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Nicolas=20No=C3=A9?= Date: Tue, 28 Jul 2026 13:24:34 +0200 Subject: [PATCH 06/33] Make tests assert behavior rather than implementation details --- dwca/test/test_datafile.py | 6 +++--- dwca/test/test_dwcareader.py | 21 ++++++++++++--------- 2 files changed, 15 insertions(+), 12 deletions(-) diff --git a/dwca/test/test_datafile.py b/dwca/test/test_datafile.py index 5fb9a3d..2808ed1 100644 --- a/dwca/test/test_datafile.py +++ b/dwca/test/test_datafile.py @@ -35,6 +35,9 @@ def test_coreid_index(self): description_txt = extension_files[0] vernacular_txt = extension_files[1] + # coreid_index values are array("L") rather than lists. This is documented in the + # property docstring and DwCAReader.orphaned_extension_rows() relies on it via + # .tolist(), so it is part of the contract, not an implementation detail. expected_core = { "1": array("L", [0]), "2": array("L", [1]), @@ -49,9 +52,6 @@ def test_coreid_index(self): expected_description = {"1": array("L", [0, 1]), "4": array("L", [2])} assert description_txt.coreid_index == expected_description - with pytest.raises(AttributeError): - dwca.corefile.coreid_index - def test_file_descriptor_attribute(self): """The instance of DataFileDescriptor passed to the constructor is available in .file_descriptor""" diff --git a/dwca/test/test_dwcareader.py b/dwca/test/test_dwcareader.py index c295a1b..f5a55b5 100644 --- a/dwca/test/test_dwcareader.py +++ b/dwca/test/test_dwcareader.py @@ -263,11 +263,15 @@ def test_descriptor_references_non_existent_data_field(self): pass def test_custom_tempdir(self): - tmp_dir = os.path.abspath(".tmp") - with DwCAReader( - sample_data_path("dwca-simple-test-archive.zip"), tmp_dir=tmp_dir - ) as dwca: - assert dwca.absolute_temporary_path("occurrence.txt").startswith(tmp_dir) + previous_tempdir = tempfile.tempdir + try: + tmp_dir = os.path.abspath(".tmp") + with DwCAReader( + sample_data_path("dwca-simple-test-archive.zip"), tmp_dir=tmp_dir + ) as dwca: + assert dwca.absolute_temporary_path("occurrence.txt").startswith(tmp_dir) + finally: + tempfile.tempdir = previous_tempdir def test_use_extensions(self): """Ensure the .use_extensions attribute of DwCAReader works as intended.""" @@ -539,10 +543,9 @@ def test_row_human_representation(self): assert "Row id:" in l_repr assert "Reference extension rows: No" in l_repr assert "Reference source metadata: No" in l_repr - assert ( - "http://rs.tdwg.org/dwc/terms/scientificName': 'tetraodon fluviatilis'" - in l_repr - ) + # Assert the value reaches the representation, not how Python formats a dict. + assert "tetraodon fluviatilis" in l_repr + assert "tetraodon fluviatilis" == l.data[qn("scientificName")] with DwCAReader(sample_data_path("dwca-star-test-archive.zip")) as star_dwca: l = star_dwca.rows[0] From b92f701eb87398dd3b0a74347325189154e56914 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Nicolas=20No=C3=A9?= Date: Tue, 28 Jul 2026 13:35:01 +0200 Subject: [PATCH 07/33] Add a benchmark harness and retire minibench --- benchmarks/README.md | 54 +++++++++++++++++++++++ benchmarks/bench_reader.py | 78 ++++++++++++++++++++++++++++++++++ benchmarks/generate_archive.py | 77 +++++++++++++++++++++++++++++++++ dwca/files.py | 4 +- dwca/minibench.py | 61 -------------------------- 5 files changed, 210 insertions(+), 64 deletions(-) create mode 100644 benchmarks/README.md create mode 100644 benchmarks/bench_reader.py create mode 100644 benchmarks/generate_archive.py delete mode 100644 dwca/minibench.py diff --git a/benchmarks/README.md b/benchmarks/README.md new file mode 100644 index 0000000..2b8fb4c --- /dev/null +++ b/benchmarks/README.md @@ -0,0 +1,54 @@ +# Benchmarks + +Not part of the test suite. Run manually before and after a change that claims a speedup. + + PYTHONPATH=. .venv/bin/python benchmarks/generate_archive.py /tmp/dwca-bench 400000 + PYTHONPATH=. .venv/bin/python benchmarks/bench_reader.py /tmp/dwca-bench + +`generate_archive.py` writes a GBIF-shaped archive: 50 columns, tab separated, no field +enclosure, no header line. 400000 rows is roughly 250MB. + +`PYTHONPATH=.` is required because the scripts are run directly (not via `python -m`), +so the repository root is not otherwise on `sys.path` and `import dwca` fails. + +## Baseline + +Measured on: + +- Commit: `771593134599074cc4ff804790065c3519783e51` (branch `parsing-performance`) +- Python: 3.12.0 (CPython) +- Machine: MacBook Pro (Mac14,5, Apple Silicon, arm64), macOS 26.5 + +Numbers are only comparable within one machine. Command: + + PYTHONPATH=. .venv/bin/python benchmarks/generate_archive.py /tmp/dwca-bench 400000 + PYTHONPATH=. .venv/bin/python benchmarks/bench_reader.py /tmp/dwca-bench + +Generator output: + + wrote 400000 rows, 50 columns, 248MB to /tmp/dwca-bench + +Benchmark output (second of two consecutive runs; both runs agreed within about 5%): + + archive: /tmp/dwca-bench + open archive 0.24s n=occurrence.txt peak=71MB + iterate, no field access 9.81s n=400000 peak=71MB + iterate + read 14 terms 10.32s n=400000 peak=71MB + random access, ~14k seeks 0.59s n=14286 peak=71MB + +First run, for comparison (same archive, same process type, run immediately before the one +above): + + archive: /tmp/dwca-bench + open archive 0.46s n=occurrence.txt peak=70MB + iterate, no field access 10.61s n=400000 peak=70MB + iterate + read 14 terms 10.25s n=400000 peak=70MB + random access, ~14k seeks 0.61s n=14286 peak=70MB + +The "no field access" and "read 14 terms" timings are close to each other in both runs. +This is expected, not a bug: `CoreRow.data` is fully materialized (all columns split and +decoded) when the row is constructed during iteration, so the benchmark's extra `.get()` +calls on an already-built dict add only marginal cost on top of the row-parsing work that +both variants pay. The gap between the two iterate variants is a better indicator of +"reading fields" cost added ON TOP of parsing than a full picture of parsing cost itself, +which the "no field access" line represents. diff --git a/benchmarks/bench_reader.py b/benchmarks/bench_reader.py new file mode 100644 index 0000000..fa0fb39 --- /dev/null +++ b/benchmarks/bench_reader.py @@ -0,0 +1,78 @@ +"""Time the documented read paths against a generated archive. + +Usage: + python benchmarks/generate_archive.py /tmp/dwca-bench 400000 + python benchmarks/bench_reader.py /tmp/dwca-bench + +Record the output before and after a change: this is the evidence for any speedup claim. +""" + +import resource +import sys +import time + +from dwca.read import DwCAReader + +# A subset a real consumer reads, rather than every column. +WANTED = [ + "http://rs.tdwg.org/dwc/terms/" + name + for name in ( + "occurrenceID scientificName basisOfRecord kingdom family genus country " + "locality decimalLatitude decimalLongitude year month day recordedBy" + ).split() +] + + +def peak_memory_mb(): + usage = resource.getrusage(resource.RUSAGE_SELF).ru_maxrss + # Linux reports kilobytes, macOS reports bytes. + if sys.platform == "darwin": + usage = usage / 1024 + return usage / 1024 + + +def timed(label, function): + started = time.perf_counter() + count = function() + elapsed = time.perf_counter() - started + print( + " {label:44s} {elapsed:7.2f}s n={count} peak={memory:.0f}MB".format( + label=label, elapsed=elapsed, count=count, memory=peak_memory_mb() + ) + ) + + +def main(archive_path): + def open_and_close(): + reader = DwCAReader(archive_path, skip_metadata=True) + location = reader.core_file_location + reader.close() + return location + + def iterate(read_fields): + count = 0 + with DwCAReader(archive_path, skip_metadata=True) as reader: + for row in reader: + if read_fields: + data = row.data + for term in WANTED: + data.get(term) + count += 1 + return count + + def random_access(): + with DwCAReader(archive_path, skip_metadata=True) as reader: + data_file = reader.core_file + return sum( + 1 for i in range(0, 100000, 7) if data_file.get_row_by_position(i) + ) + + print("archive:", archive_path) + timed("open archive", open_and_close) + timed("iterate, no field access", lambda: iterate(False)) + timed("iterate + read 14 terms", lambda: iterate(True)) + timed("random access, ~14k seeks", random_access) + + +if __name__ == "__main__": + main(sys.argv[1]) diff --git a/benchmarks/generate_archive.py b/benchmarks/generate_archive.py new file mode 100644 index 0000000..0fb175b --- /dev/null +++ b/benchmarks/generate_archive.py @@ -0,0 +1,77 @@ +"""Generate a large, GBIF-shaped Darwin Core Archive for benchmarking. + +Usage: + python benchmarks/generate_archive.py /tmp/bench-archive 400000 +""" + +import os +import sys + +TERMS = ( + ["http://rs.tdwg.org/dwc/terms/occurrenceID"] + + ["http://rs.gbif.org/terms/1.0/gbifID"] + + [ + "http://rs.tdwg.org/dwc/terms/" + name + for name in ( + "datasetName basisOfRecord scientificName scientificNameAuthorship taxonID " + "kingdom phylum class order family genus specificEpithet taxonRank " + "countryCode country stateProvince county locality municipality " + "decimalLatitude decimalLongitude coordinateUncertaintyInMeters " + "verbatimLatitude verbatimLongitude minimumElevationInMeters " + "maximumElevationInMeters minimumDepthInMeters maximumDepthInMeters " + "year month day eventDate dateIdentified identifiedBy recordedBy " + "individualCount occurrenceStatus identificationVerificationStatus " + "institutionCode collectionCode catalogNumber datasetID references " + "continent habitat sex lifeStage establishmentMeans" + ).split() + ] +) + +METAFILE = """ + + occurrence.txt + +{fields} + + +""" + +SAMPLE = [ + "PRESERVED_SPECIMEN", "Chelonodon fluviatilis (Hamilton, 1822)", "Animalia", + "Chordata", "Actinopterygii", "Tetraodontiformes", "Tetraodontidae", "Tetraodon", + "India", "Andaman and Nicobar Islands", "Port Blair", "11.6234", "92.7265", + "1930", "4", "1", "Misra, K. S.; Rao, H. Srinivasa", "CAS", "SU (ICH)", +] + + +def main(directory, row_count): + os.makedirs(directory, exist_ok=True) + + fields = "\n".join( + ' '.format(i=i, t=term) + for i, term in enumerate(TERMS) + ) + with open(os.path.join(directory, "meta.xml"), "w", encoding="utf-8") as f: + f.write(METAFILE.format(fields=fields)) + + column_count = len(TERMS) + with open( + os.path.join(directory, "occurrence.txt"), "w", encoding="utf-8", newline="" + ) as f: + for i in range(row_count): + cells = [str(600000000 + i), str(900000000 + i)] + while len(cells) < column_count: + cells.append(SAMPLE[len(cells) % len(SAMPLE)]) + f.write("\t".join(cells[:column_count]) + "\n") + + size = os.path.getsize(os.path.join(directory, "occurrence.txt")) + print( + "wrote {n} rows, {c} columns, {mb:.0f}MB to {d}".format( + n=row_count, c=column_count, mb=size / (1024 * 1024), d=directory + ) + ) + + +if __name__ == "__main__": + main(sys.argv[1], int(sys.argv[2])) diff --git a/dwca/files.py b/dwca/files.py index e91266c..f6dc388 100644 --- a/dwca/files.py +++ b/dwca/files.py @@ -168,9 +168,7 @@ def _get_all_line_offsets(f: IO, encoding: str) -> array: # We use an array of Longs instead of a list to store the index. # It's much more memory efficient, and a few tests w/ 1-4Gb uncompressed archives - # didn't show any significant slowdown. - # - # See mini-benchmark in minibench.py + # didn't show any significant slowdown (see benchmarks/ for current measurements). line_offsets = array("L") offset = 0 for line in f: diff --git a/dwca/minibench.py b/dwca/minibench.py deleted file mode 100644 index 81d3bf2..0000000 --- a/dwca/minibench.py +++ /dev/null @@ -1,61 +0,0 @@ -# Quick'n'dirty mini benchmark used to compare memory and time performance of -# array() vs standard list for the _line_offsets attribute of class CSVDataFile -# -# Early 2015 results: array is much more efficient in term of memory, and doesn't -# seem slower => array wins. -import resource - -from dwca.read import DwCAReader - - -def sizeof_fmt(num, suffix="B"): - for unit in ["", "Ki", "Mi", "Gi", "Ti", "Pi", "Ei", "Zi"]: - if abs(num) < 1024.0: - return "%3.1f%s%s" % (num, unit, suffix) - num /= 1024.0 - return "%.1f%s%s" % (num, "Yi", suffix) - - -def show_memory_usage(): - bytes = resource.getrusage(resource.RUSAGE_SELF).ru_maxrss - print(sizeof_fmt(bytes)) - - -# print "Time before: " -# print time.ctime() - -# print "Memory before:" -# show_memory_usage() - - -def test(): - with DwCAReader("dwca-florabank1-occurrences") as dwca: - # print "Time after open:" - # print time.ctime() - - # print "Memory after open:" - # show_memory_usage() - - i = 0 - for row in dwca: - # tmp = row.data[qn('locality')] - i = i + 1 - if i % 100000 == 0: - print("in loop mem: ") - show_memory_usage() - - # print "Time after loop" - # print time.ctime() - - # print "Memory after loop:" - # show_memory_usage() - - # print "Memory at the end:" - # show_memory_usage() - - -if __name__ == "__main__": - from timeit import Timer - - t = Timer("test()", "from __main__ import test") - print(t.timeit(number=3)) From 1344d1c68f6567fe48f633bd0c75fa49730594e0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Nicolas=20No=C3=A9?= Date: Tue, 28 Jul 2026 13:58:34 +0200 Subject: [PATCH 08/33] Characterize extension files and direct random access 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. --- CHANGES.txt | 7 +- benchmarks/README.md | 17 ++- benchmarks/bench_reader.py | 12 +- dwca/rows.py | 15 +- dwca/test/test_characterization.py | 226 ++++++++++++++++++++++++++++- dwca/test/test_rows.py | 15 ++ 6 files changed, 279 insertions(+), 13 deletions(-) diff --git a/CHANGES.txt b/CHANGES.txt index 91cdaac..8ab23ed 100644 --- a/CHANGES.txt +++ b/CHANGES.txt @@ -1,10 +1,13 @@ -0.17.0 (unreleased) -------------------- +v0.17.0 (unreleased) +-------------------- - Fixed: DataFileDescriptor.headers dropped the column at index 0 for archives without a metafile, which also made pd_read() promote that column to the DataFrame index. - Fixed: hash() on a CoreRow or an ExtensionRow raised TypeError. Rows have been documented as hashable since 0.3.3 but never were. +- Fixed: comparing or hashing a CoreRow obtained without linking to its DwCAReader (e.g. via + CSVDataFile.get_row_by_position() directly rather than by iterating the reader) raised + AttributeError instead of comparing successfully. v0.16.4 (2024-10-18) -------------------- diff --git a/benchmarks/README.md b/benchmarks/README.md index 2b8fb4c..4f2eb28 100644 --- a/benchmarks/README.md +++ b/benchmarks/README.md @@ -6,11 +6,26 @@ Not part of the test suite. Run manually before and after a change that claims a PYTHONPATH=. .venv/bin/python benchmarks/bench_reader.py /tmp/dwca-bench `generate_archive.py` writes a GBIF-shaped archive: 50 columns, tab separated, no field -enclosure, no header line. 400000 rows is roughly 250MB. +enclosure, no header line. 400000 rows is roughly 250MB. Because `fieldsEnclosedBy=""`, +only the unquoted parsing path (`csv.QUOTE_NONE`, see `csv_line_to_fields()` in +`dwca/rows.py`) is exercised by these benchmarks - the quoted-field path is not measured +here. `PYTHONPATH=.` is required because the scripts are run directly (not via `python -m`), so the repository root is not otherwise on `sys.path` and `import dwca` fails. +The `peak=` column comes from `resource.getrusage(...).ru_maxrss`, which is a process-wide +high-water mark, not a per-measurement figure: it never decreases within a run, so each +`timed()` line reports the peak RSS seen so far across the whole process, including +everything measured by earlier lines. Compare `peak=` across separate invocations of the +script, not across lines of the same run. + +The "open archive" timing is the only one that isolates archive-opening cost: "iterate, +..." and "random access, ..." both open a fresh `DwCAReader` (via `with DwCAReader(...)`) +inside the timed region, so their reported time includes opening the archive (parsing the +metafile and building the core file's line-offset index) on top of the operation the label +describes. + ## Baseline Measured on: diff --git a/benchmarks/bench_reader.py b/benchmarks/bench_reader.py index fa0fb39..11bbbd3 100644 --- a/benchmarks/bench_reader.py +++ b/benchmarks/bench_reader.py @@ -63,15 +63,23 @@ def iterate(read_fields): def random_access(): with DwCAReader(archive_path, skip_metadata=True) as reader: data_file = reader.core_file + # No public API exposes the row count. _line_offsets is already built when the + # CSVDataFile is opened (that's the whole point of the index), so reading its + # length here is free and lets the range below scale with the actual archive + # instead of hardcoding 100000 (which raises IndexError on smaller archives). + row_count = len(data_file._line_offsets) - data_file.lines_to_ignore + upper_bound = min(row_count, 100000) return sum( - 1 for i in range(0, 100000, 7) if data_file.get_row_by_position(i) + 1 + for i in range(0, upper_bound, 7) + if data_file.get_row_by_position(i) ) print("archive:", archive_path) timed("open archive", open_and_close) timed("iterate, no field access", lambda: iterate(False)) timed("iterate + read 14 terms", lambda: iterate(True)) - timed("random access, ~14k seeks", random_access) + timed("random access, every 7th row up to 100k", random_access) if __name__ == "__main__": diff --git a/dwca/rows.py b/dwca/rows.py index fef8427..5c7ceff 100644 --- a/dwca/rows.py +++ b/dwca/rows.py @@ -183,12 +183,23 @@ def extensions(self): # Should these 3 be factorized ? How ? Mixin ? Parent class ? def __key(self): """Return a tuple representing the row. Common ground between equality and hash.""" + # A CoreRow obtained without going through DwCAReader iteration (e.g. + # CSVDataFile.get_row_by_position() called directly) never had link_extension_files() + # / link_source_metadata() called on it, so self.extensions and self.source_metadata + # don't exist. Fall back to None for those rows instead of letting the attribute + # access raise AttributeError. This doesn't change equality for linked rows (both + # attributes are always present after DwCAReader.next() links them). + extensions = self.extensions if hasattr(self, "extension_data_files") else None + source_metadata = ( + self.source_metadata if hasattr(self, "source_metadata") else None + ) + return ( self.descriptor, self.id, self.data, - self.extensions, - self.source_metadata, + extensions, + source_metadata, self.rowtype, self.raw_fields, self.position, diff --git a/dwca/test/test_characterization.py b/dwca/test/test_characterization.py index 3e59865..901dd1a 100644 --- a/dwca/test/test_characterization.py +++ b/dwca/test/test_characterization.py @@ -6,6 +6,7 @@ """ import unittest +from array import array import pytest @@ -13,9 +14,10 @@ from dwca.read import DwCAReader from .archive_builder import build_archive, temp_archive_dir +from .helpers import sample_data_path -TERM0 = "http://rs.tdwg.org/dwc/terms/term0" TERM1 = "http://rs.tdwg.org/dwc/terms/term1" +VERNACULAR_TERM = "http://rs.tdwg.org/dwc/terms/vernacularName" class TestArchiveBuilder(unittest.TestCase): @@ -173,9 +175,9 @@ def test_utf8_bom_leaks_into_the_first_field(self): with DwCAReader(path) as dwca: rows = list(dwca) - # CHARACTERIZATION: the encoding is "utf-8", not "utf-8-sig", so the byte order - # mark becomes part of the id. A rewrite adding BOM handling changes this - # deliberately. + # Surprising but not a known bug: the encoding is "utf-8", not "utf-8-sig", so the + # byte order mark becomes part of the id instead of being stripped. A rewrite adding + # BOM handling would change this deliberately. assert "\ufeff1" == rows[0].id def test_undecodable_byte_desynchronises_random_access(self): @@ -352,8 +354,11 @@ def test_lookup_inside_a_loop(self): break # CHARACTERIZATION: wrong, see B4. get_corerow_by_id() resets the shared pointer, - # so this never terminates. Without the break it would loop forever. - assert len(seen) > 3 + # so this never terminates. Without the break it would loop forever. The exact value + # (rather than a looser bound) makes a future fix's effect on this test unambiguous: + # get_corerow_by_id("2") always rewinds to id "2", so after the first row (id "1"), + # every subsequent row is "3" until the len(seen) > 8 guard fires. + assert 9 == len(seen) def test_random_access_interleaved_with_iteration_is_safe(self): with DwCAReader(self._archive()) as dwca: @@ -364,3 +369,212 @@ def test_random_access_interleaved_with_iteration_is_safe(self): assert "1" == dwca.core_file.get_row_by_position(0).id assert ["1", "2", "3"] == seen + + +class TestExtensionFiles(unittest.TestCase): + """extension= was never passed to build_archive by any test, so that whole branch of the + builder was dead code, and every extension-bearing behavior below ran only on the three + bundled sample archives, which all happen to share one configuration (utf-8, tab, no + enclosure, ignoreHeaderLines="1"). CSVDataFile.coreid_index is built through + CSVDataFile.__iter__ (the readlines(byte-hint) header bug, see TestHeaderLines above and + B1), and get_all_rows_by_coreid() feeds those positions into get_row_by_position(), which + re-applies lines_to_ignore - exactly the seam the B1 bug lives in, and it was unpinned for + extensions until now. + """ + + def _archive(self, **kwargs): + return build_archive( + temp_archive_dir(self), + rows=[["1", "Borneo"], ["2", "Mumbai"]], + extension=[["1", "elephant"], ["1", "tiger"], ["2", "monkey"]], + **kwargs, + ) + + def test_coreid_index(self): + path = self._archive() + + with DwCAReader(path) as dwca: + index = dwca.extension_files[0].coreid_index + + assert {"1": array("L", [0, 1]), "2": array("L", [2])} == index + + def test_get_all_rows_by_coreid(self): + path = self._archive() + + with DwCAReader(path) as dwca: + ext = dwca.extension_files[0] + + several = ext.get_all_rows_by_coreid("1") + assert ["elephant", "tiger"] == [r.data[VERNACULAR_TERM] for r in several] + + one = ext.get_all_rows_by_coreid("2") + assert ["monkey"] == [r.data[VERNACULAR_TERM] for r in one] + + assert [] == ext.get_all_rows_by_coreid("unknown") + + def test_orphaned_extension_rows(self): + path = build_archive( + temp_archive_dir(self), + rows=[["1", "Borneo"], ["2", "Mumbai"]], + extension=[["1", "elephant"], ["99", "ghost"]], + ) + + with DwCAReader(path) as dwca: + assert {"extension.txt": {"99": [1]}} == dwca.orphaned_extension_rows() + + def test_core_row_extensions_content_and_ordering(self): + path = self._archive() + + with DwCAReader(path) as dwca: + rows = list(dwca) + + assert ["elephant", "tiger"] == [ + r.data[VERNACULAR_TERM] for r in rows[0].extensions + ] + assert ["monkey"] == [r.data[VERNACULAR_TERM] for r in rows[1].extensions] + + def test_ignore_header_lines_leaks_into_extension_index(self): + path = self._archive( + ignore_header_lines=2, header_rows=[["idA", "locA"], ["idB", "locB"]] + ) + + with DwCAReader(path) as dwca: + index = dwca.extension_files[0].coreid_index + + # CHARACTERIZATION: wrong, see B1. Same readlines(byte-hint) bug as the core file + # (TestHeaderLines.test_two_headers_iteration_and_random_access_agree): the second + # header line leaks into the index as a data row referencing core id "idB", and + # every position after it is shifted by one. + assert { + "idB": array("L", [0]), + "1": array("L", [1, 2]), + "2": array("L", [3]), + } == index + + +class TestRandomAccessDirect(unittest.TestCase): + """DwCAReader.next() is implemented as CSVDataFile.get_row_by_position() (see + dwca/read.py), so today every iteration test incidentally exercises random access too. + A rewrite that stops delegating would make that coverage disappear silently unless + something calls get_row_by_position() directly, which is what these tests do. + """ + + def test_multibyte_utf8(self): + # "\u4e2d\u6587" ("Chinese" in Chinese) is 2 characters but 6 bytes in UTF-8, so the + # byte offset of row 1 (used internally for seek()) differs from what a + # character-based offset would be. + locality = "caf\u00e9 \u4e2d\u6587" + path = build_archive( + temp_archive_dir(self), + rows=[["1", locality], ["2", "Mumbai"]], + ) + + with DwCAReader(path) as dwca: + first = dwca.core_file.get_row_by_position(0) + second = dwca.core_file.get_row_by_position(1) + + assert locality == first.data[TERM1] + assert ["1", locality] == first.raw_fields + assert "Mumbai" == second.data[TERM1] + assert ["2", "Mumbai"] == second.raw_fields + + def test_dos_terminator(self): + path = build_archive( + temp_archive_dir(self), + rows=[["1", "Borneo"], ["2", "Mumbai"]], + lines_terminated_by="\r\n", + ) + + with DwCAReader(path) as dwca: + first = dwca.core_file.get_row_by_position(0) + second = dwca.core_file.get_row_by_position(1) + + assert "Borneo" == first.data[TERM1] + assert ["1", "Borneo"] == first.raw_fields + assert "Mumbai" == second.data[TERM1] + assert ["2", "Mumbai"] == second.raw_fields + + def test_quoted_field_containing_the_separator(self): + path = build_archive( + temp_archive_dir(self), + rows=[["1", "plain, with comma"], ["2", "second"]], + fields_terminated_by=",", + fields_enclosed_by='"', + ) + + with DwCAReader(path) as dwca: + first = dwca.core_file.get_row_by_position(0) + second = dwca.core_file.get_row_by_position(1) + + assert "plain, with comma" == first.data[TERM1] + assert ["1", "plain, with comma"] == first.raw_fields + assert "second" == second.data[TERM1] + assert ["2", "second"] == second.raw_fields + + +class TestRowExtensionsDuringCoreIteration(unittest.TestCase): + def _archive(self): + return build_archive( + temp_archive_dir(self), + rows=[["1", "Borneo"], ["2", "Mumbai"]], + extension=[["1", "elephant"], ["1", "tiger"], ["2", "monkey"]], + ) + + def test_row_extensions_accessible_during_core_iteration(self): + path = self._archive() + + seen = {} + with DwCAReader(path) as dwca: + for row in dwca: + seen[row.id] = [e.data[VERNACULAR_TERM] for e in row.extensions] + + assert {"1": ["elephant", "tiger"], "2": ["monkey"]} == seen + + def test_row_extensions_are_cached(self): + path = self._archive() + + with DwCAReader(path) as dwca: + row = next(iter(dwca)) + first = row.extensions + second = row.extensions + + # Lazy-loaded and cached on the instance: same list object both times, not rebuilt. + assert first is second + assert ["elephant", "tiger"] == [e.data[VERNACULAR_TERM] for e in first] + + +class TestNegativePosition(unittest.TestCase): + def test_negative_position_returns_the_header_line(self): + # get_row_by_position() computes self._line_offsets[position + lines_to_ignore]. For + # position=-1 this wraps around to the last entry in the offsets array, which is the + # header line (kept in the index but skipped during normal iteration). + with DwCAReader(sample_data_path("dwca-simple-test-archive.zip")) as dwca: + row = dwca.core_file.get_row_by_position(-1) + + # CHARACTERIZATION: wrong, see B7. A rewrite will likely raise IndexError instead. + assert "id" == row.id + + +class TestDuplicateCoreIds(unittest.TestCase): + """get_corerow_by_id()'s docstring disclaims which row wins when ids repeat, and + coreid_index maps one id to several positions. Pin both so a rewrite has to decide + deliberately rather than by accident. + """ + + def _archive(self): + return build_archive( + temp_archive_dir(self), + rows=[["1", "Borneo"], ["1", "Mumbai"], ["2", "Paris"]], + ) + + def test_get_corerow_by_id_returns_the_first_match(self): + with DwCAReader(self._archive()) as dwca: + row = dwca.get_corerow_by_id("1") + + assert "Borneo" == row.data[TERM1] + + def test_coreid_index_holds_every_position(self): + with DwCAReader(self._archive()) as dwca: + index = dwca.core_file.coreid_index + + assert {"1": array("L", [0, 1]), "2": array("L", [2])} == index diff --git a/dwca/test/test_rows.py b/dwca/test/test_rows.py index 2716185..83e991b 100644 --- a/dwca/test/test_rows.py +++ b/dwca/test/test_rows.py @@ -64,6 +64,21 @@ def test_extension_rows_are_hashable(self): assert len(set(extension_rows)) == len(extension_rows) + def test_unlinked_core_rows_are_comparable(self): + """A CoreRow obtained directly from CSVDataFile.get_row_by_position() (rather than by + iterating the DwCAReader) is never linked to extension files or source metadata, so + self.extension_data_files and self.source_metadata don't exist on it. __key() used to + access self.extensions and self.source_metadata unconditionally, so hash()/__eq__ on + such a row raised AttributeError - which set() and put in a set both trigger.""" + with DwCAReader(sample_data_path("dwca-2extensions.zip")) as dwca: + one = dwca.core_file.get_row_by_position(0) + two = dwca.core_file.get_row_by_position(0) + + assert one is not two + assert one == two + assert hash(one) == hash(two) + assert len({one, two}) == 1 + def test_equal_rows_hash_equally(self): # Uses two independently-fetched (but equal) CoreRow instances from the *same* reader, # rather than from two separate readers. DataFileDescriptor has no __eq__ of its own, so From fd829b61962b5a813705a0f7c5d1f12c1efe07e7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Nicolas=20No=C3=A9?= Date: Tue, 28 Jul 2026 14:23:02 +0200 Subject: [PATCH 09/33] Compare rows and descriptors by value 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. --- CHANGES.txt | 6 +++ docs/decisions.md | 11 ++++ dwca/descriptors.py | 51 +++++++++++++++++++ dwca/rows.py | 17 ++++--- dwca/test/test_descriptors.py | 96 +++++++++++++++++++++++++++++++++++ dwca/test/test_rows.py | 77 +++++++++++++++++++++++++--- 6 files changed, 246 insertions(+), 12 deletions(-) create mode 100644 docs/decisions.md diff --git a/CHANGES.txt b/CHANGES.txt index 8ab23ed..806b2a4 100644 --- a/CHANGES.txt +++ b/CHANGES.txt @@ -8,6 +8,12 @@ v0.17.0 (unreleased) - Fixed: comparing or hashing a CoreRow obtained without linking to its DwCAReader (e.g. via CSVDataFile.get_row_by_position() directly rather than by iterating the reader) raised AttributeError instead of comparing successfully. +- Fixed: rows read from two DwCAReader instances over the same archive never compared equal, + even with identical content. Row equality embeds the DataFileDescriptor, which had no + __eq__ and therefore compared by object identity. DataFileDescriptor now compares (and + hashes) by value: the data file layout it describes. +- Fixed: comparing a CoreRow to an ExtensionRow (or either to a non-row object) raised + AttributeError instead of returning False. v0.16.4 (2024-10-18) -------------------- diff --git a/docs/decisions.md b/docs/decisions.md new file mode 100644 index 0000000..21ff83b --- /dev/null +++ b/docs/decisions.md @@ -0,0 +1,11 @@ +# Decisions + +## 2026-07-28 - Rows and descriptors compare by value + +**What:** `DataFileDescriptor` got `__eq__`/`__hash__` over the data file layout it describes, +so rows from two readers over the same archive now compare equal. +**Why:** Row equality embeds the descriptor; with identity-based comparison, rows identical in +data, raw_fields, id, position and rowtype still compared unequal across readers. +**Rejected:** Documenting the single-reader scope on `CoreRow.__eq__` instead - a descriptor +describes a file layout, not the reader that built it, so identity comparison was a bug rather +than a boundary worth keeping. diff --git a/dwca/descriptors.py b/dwca/descriptors.py index 491f632..30a55e6 100644 --- a/dwca/descriptors.py +++ b/dwca/descriptors.py @@ -210,6 +210,57 @@ def make_from_metafile_section(cls, section_tag): fields_terminated_by=fields_terminated_by, ) + def __key(self): + """Return a tuple describing the data file layout. Common ground between equality and hash. + + Deliberately excludes `raw_element`: it's an xml.etree.ElementTree.Element, which + compares by identity, so including it would make two descriptors built from the same + metafile section never compare equal. `lines_to_ignore` stands in for the only part of + `raw_element` that affects parsing (the ignoreHeaderLines attribute). + + `created_from_file` is excluded too: it records where the descriptor came from, not how + the file is laid out, and its only behavioral effect is already covered by + `lines_to_ignore`. `represents_extension` is excluded as it's just the negation of + `represents_corefile`. + """ + return ( + self.file_location, + self.file_encoding, + self.type, + self.id_index, + self.coreid_index, + self.represents_corefile, + self.fields, + self.lines_terminated_by, + self.fields_enclosed_by, + self.fields_terminated_by, + self.lines_to_ignore, + ) + + def __eq__(self, other): + if not isinstance(other, DataFileDescriptor): + return NotImplemented + + return self.__key() == other.__key() + + def __hash__(self): + # __key() embeds `fields`, a list of dicts, which isn't hashable. Equal descriptors + # still hash equally because this is a subset of the equality key. + return hash( + ( + self.file_location, + self.file_encoding, + self.type, + self.id_index, + self.coreid_index, + self.represents_corefile, + self.lines_terminated_by, + self.fields_enclosed_by, + self.fields_terminated_by, + self.lines_to_ignore, + ) + ) + @property def terms(self) -> Set[str]: """Return a Python set containing all the Darwin Core terms appearing in file.""" diff --git a/dwca/rows.py b/dwca/rows.py index 5c7ceff..2024a6b 100644 --- a/dwca/rows.py +++ b/dwca/rows.py @@ -206,10 +206,13 @@ def __key(self): ) def __eq__(self, other): - return self.__key() == other.__key() + # The isinstance test is required, not just defensive: __key is name-mangled, so + # other.__key() means other._CoreRow__key(), which an ExtensionRow (or any non-row) + # doesn't have. Without it, comparing to anything else raises AttributeError. + if not isinstance(other, CoreRow): + return NotImplemented - def __ne__(self, other): - return not self.__eq__(other) + return self.__key() == other.__key() def __hash__(self): # __key() embeds the data dict, the raw field list and the lazily-loaded extensions, @@ -249,10 +252,12 @@ def __key(self): ) def __eq__(self, other): - return self.__key() == other.__key() + # See the note on CoreRow.__eq__: __key is name-mangled, so this test is what keeps + # comparisons against a CoreRow (or anything else) from raising AttributeError. + if not isinstance(other, ExtensionRow): + return NotImplemented - def __ne__(self, other): - return not self.__eq__(other) + return self.__key() == other.__key() def __hash__(self): return hash((self.descriptor, self.core_id, self.rowtype, self.position)) diff --git a/dwca/test/test_descriptors.py b/dwca/test/test_descriptors.py index 62df868..bae50da 100644 --- a/dwca/test/test_descriptors.py +++ b/dwca/test/test_descriptors.py @@ -455,6 +455,102 @@ def test_exposes_core_terms(self): assert fields == descriptor.core.terms +class TestDataFileDescriptorEquality(unittest.TestCase): + """Unit tests for DataFileDescriptor equality and hashing. + + Descriptors compare by value (the data file layout they describe) rather than by object + identity, so descriptors built twice from the same archive - by two DwCAReader instances, + for example - compare equal. + """ + + CORE_SECTION = """ + + + occurrence.txt + + + + + + """ + + def _make(self, section=None): + return DataFileDescriptor.make_from_metafile_section( + ET.fromstring(section if section is not None else self.CORE_SECTION) + ) + + def test_descriptors_from_identical_sections_are_equal(self): + one = self._make() + two = self._make() + + assert one is not two + # raw_element is an ET.Element, which compares by identity: these two are distinct + # objects, so equality can only hold if raw_element is left out of the comparison. + assert one.raw_element is not two.raw_element + assert one == two + assert not (one != two) + assert hash(one) == hash(two) + assert len({one, two}) == 1 + + def test_descriptors_of_the_same_archive_read_twice_are_equal(self): + path = sample_data_path("dwca-2extensions.zip") + + with DwCAReader(path) as one, DwCAReader(path) as two: + assert one.descriptor.core == two.descriptor.core + + for ext_one, ext_two in zip( + one.descriptor.extensions, two.descriptor.extensions + ): + assert ext_one == ext_two + + def test_core_and_extension_descriptors_differ(self): + with DwCAReader(sample_data_path("dwca-2extensions.zip")) as dwca: + core = dwca.descriptor.core + + for extension in dwca.descriptor.extensions: + assert core != extension + + def test_descriptors_differing_by_ignored_header_lines_differ(self): + one = self._make() + two = self._make( + self.CORE_SECTION.replace('ignoreHeaderLines="0"', 'ignoreHeaderLines="1"') + ) + + # Everything but the number of header lines to skip is identical here. That number + # lives in raw_element, which is excluded from the comparison, so it's only taken into + # account through the lines_to_ignore property. + assert one.lines_to_ignore != two.lines_to_ignore + assert one != two + + def test_descriptors_differing_by_fields_differ(self): + one = self._make() + two = self._make( + self.CORE_SECTION.replace( + 'term="http://rs.tdwg.org/dwc/terms/scientificName"', + 'term="http://rs.tdwg.org/dwc/terms/locality"', + ) + ) + + assert one != two + + def test_descriptors_differing_by_file_location_differ(self): + one = self._make() + two = self._make( + self.CORE_SECTION.replace("occurrence.txt", "other_occurrences.txt") + ) + + assert one != two + + def test_comparison_with_other_types_returns_false(self): + descriptor = self._make() + + assert descriptor != "not a descriptor" + assert descriptor != 42 + assert descriptor != None # noqa: E711 - we're testing __eq__, not identity + assert not (descriptor == "not a descriptor") + + class TestArchiveDescriptor(unittest.TestCase): """Unit tests for ArchiveDescriptor class.""" diff --git a/dwca/test/test_rows.py b/dwca/test/test_rows.py index 83e991b..99a6a63 100644 --- a/dwca/test/test_rows.py +++ b/dwca/test/test_rows.py @@ -80,12 +80,7 @@ def test_unlinked_core_rows_are_comparable(self): assert len({one, two}) == 1 def test_equal_rows_hash_equally(self): - # Uses two independently-fetched (but equal) CoreRow instances from the *same* reader, - # rather than from two separate readers. DataFileDescriptor has no __eq__ of its own, so - # instances from two separate readers never compare equal (identity-based comparison) - - # that is a pre-existing bug unrelated to hashing, out of scope for this fix. Within a - # single reader, descriptor is the same shared instance, so this still genuinely exercises - # "two distinct objects that compare equal must hash equal". + # Two distinct objects that compare equal must hash equal. with DwCAReader(sample_data_path("dwca-2extensions.zip")) as dwca: one = dwca.rows[0] two = dwca.rows[0] @@ -93,3 +88,73 @@ def test_equal_rows_hash_equally(self): assert one is not two assert one == two assert hash(one) == hash(two) + + +class TestRowEquality(unittest.TestCase): + """Rows compare by value, including across readers. + + Row equality used to embed the DataFileDescriptor, which compared by object identity: rows + read from two DwCAReader instances over the same archive never compared equal, even with + identical data, raw_fields, id, position and rowtype. + """ + + def test_core_rows_from_two_readers_over_the_same_archive_are_equal(self): + path = sample_data_path("dwca-2extensions.zip") + + with DwCAReader(path) as one, DwCAReader(path) as two: + row_one = one.rows[0] + row_two = two.rows[0] + + assert row_one.descriptor is not row_two.descriptor + assert row_one == row_two + assert not (row_one != row_two) + assert hash(row_one) == hash(row_two) + assert len({row_one, row_two}) == 1 + + def test_extension_rows_from_two_readers_over_the_same_archive_are_equal(self): + path = sample_data_path("dwca-2extensions.zip") + + with DwCAReader(path) as one, DwCAReader(path) as two: + row_one = one.rows[0].extensions[0] + row_two = two.rows[0].extensions[0] + + assert row_one.descriptor is not row_two.descriptor + assert row_one == row_two + assert hash(row_one) == hash(row_two) + assert len({row_one, row_two}) == 1 + + def test_different_core_rows_are_not_equal(self): + with DwCAReader(sample_data_path("dwca-2extensions.zip")) as dwca: + assert dwca.rows[0] != dwca.rows[1] + + def test_comparing_a_core_row_to_an_extension_row_returns_false(self): + """__key() is name-mangled, so CoreRow.__eq__ used to call other._CoreRow__key() on an + ExtensionRow, which doesn't have it: the comparison raised AttributeError instead of + returning False.""" + with DwCAReader(sample_data_path("dwca-2extensions.zip")) as dwca: + core_row = dwca.rows[0] + extension_row = core_row.extensions[0] + + assert core_row != extension_row + assert extension_row != core_row + assert not (core_row == extension_row) + + def test_comparing_rows_with_other_types_returns_false(self): + with DwCAReader(sample_data_path("dwca-2extensions.zip")) as dwca: + core_row = dwca.rows[0] + extension_row = core_row.extensions[0] + + for row in (core_row, extension_row): + assert row != "not a row" + assert row != 42 + assert row != None # noqa: E711 - we're testing __eq__, not identity + assert not (row == "not a row") + + def test_rows_of_different_types_can_share_a_set(self): + """A CoreRow and an ExtensionRow landing in the same hash bucket must compare, not + raise.""" + with DwCAReader(sample_data_path("dwca-2extensions.zip")) as dwca: + core_row = dwca.rows[0] + extension_row = core_row.extensions[0] + + assert len({core_row, extension_row}) == 2 From f533ed3c05e122ffaac155baaa3c4806452026af Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Nicolas=20No=C3=A9?= Date: Tue, 28 Jul 2026 14:27:34 +0200 Subject: [PATCH 10/33] Precompute the descriptor field mapping once per data file --- dwca/descriptors.py | 115 ++++++++++++++++++++++++++++++++++ dwca/test/test_descriptors.py | 96 ++++++++++++++++++++++++++++ 2 files changed, 211 insertions(+) diff --git a/dwca/descriptors.py b/dwca/descriptors.py index 30a55e6..d1e4f44 100644 --- a/dwca/descriptors.py +++ b/dwca/descriptors.py @@ -12,6 +12,7 @@ import io import os import re +from operator import itemgetter import xml.etree.ElementTree as ET from typing import Optional, List, Dict, Set from xml.etree.ElementTree import Element @@ -261,6 +262,15 @@ def __hash__(self): ) ) + @property + def field_plan(self) -> "FieldPlan": + """A cached :class:`FieldPlan` turning a split data row into a term -> value dict.""" + plan = self.__dict__.get("_field_plan") + if plan is None: + plan = FieldPlan(self.fields) + self.__dict__["_field_plan"] = plan + return plan + @property def terms(self) -> Set[str]: """Return a Python set containing all the Darwin Core terms appearing in file.""" @@ -315,6 +325,111 @@ def lines_to_ignore(self) -> int: return int(self.raw_element.get("ignoreHeaderLines", 0)) +class FieldPlan(object): + """Precomputed mapping from a split CSV line to a term -> value dict. + + Built once per :class:`DataFileDescriptor` (see its ``field_plan`` property) so that + parsing a row costs one C-level ``dict(zip(...))`` instead of a Python loop over the + field descriptors. + """ + + __slots__ = ( + "_fields", + "_contiguous_terms", + "_terms", + "_getter", + "_single_column", + "_defaults", + "_constants", + "_ordered", + "required_columns", + ) + + def __init__(self, fields): + self._fields = fields + + indexed = [f for f in fields if f["index"] is not None] + indexes = [f["index"] for f in indexed] + + #: Number of columns a data row must have for this plan to apply. + self.required_columns = max(indexes) + 1 if indexes else 0 + + # Terms without a column: the value is always the default. + self._constants = tuple( + (f["term"], f["default"] or "") for f in fields if f["index"] is None + ) + # Indexed terms that also carry a default, used when the cell is empty (issue #80). + self._defaults = tuple( + (f["term"], f["default"]) for f in indexed if f["default"] + ) + + # When some terms have no column, the fast paths below would append them after the + # mapped ones and change the key order of Row.data (which is user-visible through + # str(row)). Those archives use an ordered path instead; they are rare, and the + # ordered path is still free of the per-field try/except and int() it replaces. + self._ordered = ( + tuple((f["term"], f["index"], f["default"]) for f in fields) + if self._constants + else None + ) + + if indexes and sorted(indexes) == list(range(len(indexes))): + # Fast path: the indexed fields cover columns 0..n-1 exactly once. + ordered = sorted(indexed, key=lambda f: f["index"]) + self._contiguous_terms = tuple(f["term"] for f in ordered) + self._terms = None + self._getter = None + self._single_column = False + else: + self._contiguous_terms = None + self._terms = tuple(f["term"] for f in indexed) + self._getter = itemgetter(*indexes) if indexes else None + # itemgetter with a single argument returns a scalar, not a tuple. + self._single_column = len(indexes) == 1 + + def build_data(self, raw_fields): + """Return the term -> value dict for an already-split data row.""" + if len(raw_fields) < self.required_columns: + self._raise_missing_column(raw_fields) + + if self._ordered is not None: + data = {} + for term, index, default in self._ordered: + value = raw_fields[index] if index is not None else None + data[term] = value or default or "" + return data + + if self._contiguous_terms is not None: + data = dict(zip(self._contiguous_terms, raw_fields)) + elif self._getter is not None: + values = self._getter(raw_fields) + if self._single_column: + values = (values,) + data = dict(zip(self._terms, values)) + else: + data = {} + + for term, default in self._defaults: + if not data[term]: + data[term] = default + for term, constant in self._constants: + data[term] = constant + + return data + + def _raise_missing_column(self, raw_fields): + # Slow path: report the same index the old per-field loop would have reported. + for field in self._fields: + index = field["index"] + if index is not None and index >= len(raw_fields): + raise InvalidArchive( + "The descriptor references a non-existent field (index={i})".format( + i=index + ) + ) + raise InvalidArchive("The descriptor references a non-existent field") + + class ArchiveDescriptor(object): """Class used to encapsulate the whole Metafile (`meta.xml`).""" diff --git a/dwca/test/test_descriptors.py b/dwca/test/test_descriptors.py index bae50da..430d77a 100644 --- a/dwca/test/test_descriptors.py +++ b/dwca/test/test_descriptors.py @@ -3,8 +3,11 @@ import xml.etree.ElementTree as ET import zipfile +import pytest + from dwca.darwincore.utils import qualname as qn from dwca.descriptors import DataFileDescriptor, ArchiveDescriptor +from dwca.exceptions import InvalidArchive from dwca.read import DwCAReader from .helpers import sample_data_path @@ -700,3 +703,96 @@ def test_column_at_index_zero_is_not_dropped(self): assert len(descriptor.fields) == len(descriptor.headers) assert "gbifid" == descriptor.headers[0] + + +class TestFieldPlan(unittest.TestCase): + def _descriptor(self, fields_xml, tag="core", id_tag=''): + section = """ + <{tag} encoding="utf-8" fieldsTerminatedBy="\\t" linesTerminatedBy="\\n" \ +fieldsEnclosedBy="" ignoreHeaderLines="0" rowType="http://rs.tdwg.org/dwc/terms/Occurrence"> + occurrence.txt + {id_tag} + {fields} + + """.format( + tag=tag, id_tag=id_tag, fields=fields_xml + ) + return DataFileDescriptor.make_from_metafile_section(ET.fromstring(section)) + + def test_contiguous_columns(self): + descriptor = self._descriptor( + '' + '' + ) + + assert {"http://x/a": "1", "http://x/b": "Borneo"} == ( + descriptor.field_plan.build_data(["1", "Borneo"]) + ) + + def test_columns_out_of_order_and_with_gaps(self): + descriptor = self._descriptor( + '' + '' + ) + + assert {"http://x/a": "third", "http://x/b": "first"} == ( + descriptor.field_plan.build_data(["first", "second", "third"]) + ) + + def test_single_column(self): + descriptor = self._descriptor('') + + assert {"http://x/a": "Borneo"} == descriptor.field_plan.build_data( + ["1", "Borneo"] + ) + + def test_default_only_field_has_no_column(self): + descriptor = self._descriptor( + '' + '' + ) + + assert {"http://x/a": "1", "http://x/country": "Belgium"} == ( + descriptor.field_plan.build_data(["1"]) + ) + + def test_default_fills_an_empty_cell(self): + """A field can have both a column and a default (issue #80).""" + descriptor = self._descriptor( + '' + '' + ) + + assert "Borneo" == descriptor.field_plan.build_data(["1", "Borneo"])["http://x/b"] + assert "fallback" == descriptor.field_plan.build_data(["1", ""])["http://x/b"] + + def test_missing_value_without_default_becomes_empty_string(self): + descriptor = self._descriptor( + '' + '' + ) + + assert "" == descriptor.field_plan.build_data(["1", ""])["http://x/b"] + + def test_key_order_follows_the_metafile(self): + """Row.data key order is visible through str(row), so it must not drift.""" + descriptor = self._descriptor( + '' + '' + '' + ) + + assert ["http://x/a", "http://x/country", "http://x/b"] == list( + descriptor.field_plan.build_data(["1", "Borneo"]) + ) + + def test_row_with_too_few_columns_raises(self): + descriptor = self._descriptor('') + + with pytest.raises(InvalidArchive): + descriptor.field_plan.build_data(["1", "Borneo"]) + + def test_the_plan_is_cached(self): + descriptor = self._descriptor('') + + assert descriptor.field_plan is descriptor.field_plan From cd703074876937d959fdfad475cb3c5f79a84a4e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Nicolas=20No=C3=A9?= Date: Tue, 28 Jul 2026 14:34:18 +0200 Subject: [PATCH 11/33] Build rows from split fields and stop stripping quotes from content --- dwca/rows.py | 138 +++++++++++++++++++++-------------------- dwca/test/test_rows.py | 62 ++++++++++++++++++ 2 files changed, 134 insertions(+), 66 deletions(-) diff --git a/dwca/rows.py b/dwca/rows.py index 2024a6b..ff30bb0 100644 --- a/dwca/rows.py +++ b/dwca/rows.py @@ -2,10 +2,9 @@ import csv import sys -from typing import Dict, Optional +from typing import Dict, List, Optional from dwca.descriptors import DataFileDescriptor -from dwca.exceptions import InvalidArchive class Row(object): @@ -14,6 +13,8 @@ class Row(object): This class is intended to be subclassed rather than used directly. """ + __slots__ = ("descriptor", "position", "rowtype", "raw_fields", "data") + # Common ground for __str__ between subclasses def _build_str(self, source_str, id_str): txt = ( @@ -52,33 +53,50 @@ def _build_str(self, source_str, id_str): def __init__( self, csv_line: str, position: int, datafile_descriptor: DataFileDescriptor ) -> None: - #: An instance of :class:`dwca.descriptors.DataFileDescriptor` describing the originating - #: data file. + self._populate( + csv_line_to_fields( + csv_line, + line_ending=datafile_descriptor.lines_terminated_by, + field_ending=datafile_descriptor.fields_terminated_by, + fields_enclosed_by=datafile_descriptor.fields_enclosed_by, + ), + position, + datafile_descriptor, + ) + + @classmethod + def from_fields( + cls, + raw_fields: List[str], + position: int, + datafile_descriptor: DataFileDescriptor, + ) -> "Row": + """Build a Row from an already-split data row. + + This is the constructor used by the streaming engine. :meth:`__init__`, which takes a + raw CSV line, is kept for backwards compatibility. + """ + row = cls.__new__(cls) + row._populate(raw_fields, position, datafile_descriptor) + return row + + def _populate(self, raw_fields, position, datafile_descriptor) -> None: + #: An instance of :class:`dwca.descriptors.DataFileDescriptor` describing the + #: originating data file. self.descriptor = datafile_descriptor # type: DataFileDescriptor - #: The row position/index (starting at 0) in the source data file. This can be used, for example with - #: :meth:`dwca.read.DwCAReader.get_corerow_by_position` or :meth:`dwca.files.CSVDataFile.get_row_by_position`. + #: The row position/index (starting at 0) in the source data file. This can be used, + #: for example with :meth:`dwca.read.DwCAReader.get_corerow_by_position` or + #: :meth:`dwca.files.CSVDataFile.get_row_by_position`. self.position = position # type: int - #: The csv line type as stated in the archive descriptor. - #: (or None if the archive has no descriptor). Examples: - #: http://rs.tdwg.org/dwc/terms/Occurrence, + #: The csv line type as stated in the archive descriptor (or None if the archive has + #: no descriptor). Examples: http://rs.tdwg.org/dwc/terms/Occurrence, #: http://rs.gbif.org/terms/1.0/VernacularName, ... self.rowtype = self.descriptor.type # type: Optional[str] - # self.raw_fields is a list of the csv_line's content - #: - self.raw_fields = csv_line_to_fields( - csv_line, - line_ending=self.descriptor.lines_terminated_by, - field_ending=self.descriptor.fields_terminated_by, - fields_enclosed_by=self.descriptor.fields_enclosed_by, - ) - - # TODO: raw_fields is a new property: to test - - # TODO: Consistency check ?? self.raw_fields length should be : - # num of self.raw_fields described in core_meta + 2 (id and \n) + #: A list of the row's raw (unmapped) field values. + self.raw_fields = raw_fields #: A dict containing the Row data, such as:: #: @@ -90,29 +108,11 @@ def __init__( #: #: myrow.data['http://rs.tdwg.org/dwc/terms/locality'] # => "Brussels" #: - #: .. note:: The :func:`dwca.darwincore.utils.qualname` helper is available to make such calls less verbose. - self.data = {} # type: Dict[str, str] - - for field_descriptor in self.descriptor.fields: - try: - column_index = int(field_descriptor["index"]) - field_row_value = self.raw_fields[column_index] - except TypeError: - # int() argument must be a string... We don't have an index for this field - field_row_value = None - except IndexError: - msg = ( - "The descriptor references a non-existent field (index={i})".format( - i=column_index - ) - ) - raise InvalidArchive(msg) - - field_default_value = field_descriptor["default"] - - self.data[field_descriptor["term"]] = ( - field_row_value or field_default_value or "" - ) + #: .. note:: The :func:`dwca.darwincore.utils.qualname` helper is available to make + #: such calls less verbose. + self.data = datafile_descriptor.field_plan.build_data( + raw_fields + ) # type: Dict[str, str] class CoreRow(Row): @@ -127,14 +127,14 @@ def __str__(self) -> str: id_str = "Row id: " + str(self.id) return super(CoreRow, self)._build_str("Core file", id_str) - def __init__( - self, csv_line: str, position: int, datafile_descriptor: DataFileDescriptor - ) -> None: - super(CoreRow, self).__init__(csv_line, position, datafile_descriptor) + __slots__ = ("id", "source_metadata", "extension_data_files", "_extensions") + + def _populate(self, raw_fields, position, datafile_descriptor) -> None: + super(CoreRow, self)._populate(raw_fields, position, datafile_descriptor) - if self.descriptor.id_index is not None: + if datafile_descriptor.id_index is not None: #: The row id - self.id = self.raw_fields[self.descriptor.id_index] + self.id = raw_fields[datafile_descriptor.id_index] else: self.id = None @@ -232,13 +232,13 @@ def __str__(self): id_str = "Core row id: " + str(self.core_id) return super(ExtensionRow, self)._build_str("Extension file", id_str) - def __init__( - self, csv_line: str, position: int, datafile_descriptor: DataFileDescriptor - ) -> None: - super(ExtensionRow, self).__init__(csv_line, position, datafile_descriptor) + __slots__ = ("core_id",) + + def _populate(self, raw_fields, position, datafile_descriptor) -> None: + super(ExtensionRow, self)._populate(raw_fields, position, datafile_descriptor) #: The id of the core row this extension row is referring to. - self.core_id = self.raw_fields[datafile_descriptor.coreid_index] + self.core_id = raw_fields[datafile_descriptor.coreid_index] def __key(self): """Return a tuple representing the row. Common ground between equality and hash.""" @@ -269,15 +269,21 @@ def csv_line_to_fields(csv_line, line_ending, field_ending, fields_enclosed_by): Return a list of fields. Content is not trimmed. """ csv_line = csv_line.rstrip(line_ending) - raw_fields = [] if fields_enclosed_by == "": - opts = {"quoting": csv.QUOTE_NONE} - else: - opts = {"quoting": csv.QUOTE_ALL, "quotechar": fields_enclosed_by} - - for row in csv.reader([csv_line], delimiter=field_ending, **opts): - for f in row: - field = f.strip(fields_enclosed_by) - raw_fields.append(field) - return raw_fields + # No enclosure: the line is simply split on the separator. This also keeps any + # quote character that happens to appear in the content. + return csv_line.split(field_ending) + + # The csv module unwraps the enclosure and un-doubles escaped quote characters itself. + # Stripping the quote character afterwards would eat a legitimate leading or trailing + # one from the field's own content. + for row in csv.reader( + [csv_line], + delimiter=field_ending, + quotechar=fields_enclosed_by, + quoting=csv.QUOTE_MINIMAL, + ): + return row + + return [] diff --git a/dwca/test/test_rows.py b/dwca/test/test_rows.py index 99a6a63..492fb69 100644 --- a/dwca/test/test_rows.py +++ b/dwca/test/test_rows.py @@ -158,3 +158,65 @@ def test_rows_of_different_types_can_share_a_set(self): extension_row = core_row.extensions[0] assert len({core_row, extension_row}) == 2 + + +class TestRowFromFields(unittest.TestCase): + def _descriptor(self): + import xml.etree.ElementTree as ET + + from dwca.descriptors import DataFileDescriptor + + section = """ + + occurrence.txt + + + + + """ + return DataFileDescriptor.make_from_metafile_section(ET.fromstring(section)) + + def test_from_fields_matches_the_raw_line_constructor(self): + from dwca.rows import CoreRow + + descriptor = self._descriptor() + + from_line = CoreRow("1\tBorneo\n", 0, descriptor) + from_fields = CoreRow.from_fields(["1", "Borneo"], 0, descriptor) + + assert from_line.data == from_fields.data + assert from_line.raw_fields == from_fields.raw_fields + assert from_line.id == from_fields.id + assert from_line.position == from_fields.position + assert from_line.rowtype == from_fields.rowtype + + def test_from_fields_returns_the_right_class(self): + from dwca.rows import CoreRow + + row = CoreRow.from_fields(["1", "Borneo"], 0, self._descriptor()) + + assert isinstance(row, CoreRow) + + +class TestCsvLineToFieldsQuoting(unittest.TestCase): + def test_quote_at_the_edge_of_content_is_preserved(self): + """The csv module already un-doubles escaped quotes; stripping afterwards ate a + legitimate leading or trailing quote from the field's own content.""" + assert ['1', 'say "hi"'] == csv_line_to_fields( + '"1","say ""hi"""', "\n", ",", '"' + ) + assert ['1', '"hi" she said'] == csv_line_to_fields( + '"1","""hi"" she said"', "\n", ",", '"' + ) + + def test_delimiter_inside_a_quoted_field_still_works(self): + """Regression guard for the v0.11.0 fix.""" + assert ["field 1", "field 2, with comma", "field 3"] == csv_line_to_fields( + 'field 1,"field 2, with comma",field 3', "\n", ",", '"' + ) + + def test_unenclosed_line_keeps_quote_characters(self): + assert ["1", '"betta" splendens'] == csv_line_to_fields( + '1\t"betta" splendens', "\n", "\t", "" + ) From da6cd1bede8c3b287c2f94ef25b16a590a192a82 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Nicolas=20No=C3=A9?= Date: Tue, 28 Jul 2026 14:40:55 +0200 Subject: [PATCH 12/33] Stream data files in a single forward pass --- dwca/files.py | 87 +++++++++++++++++++++++++++++++------- dwca/test/test_datafile.py | 58 +++++++++++++++++++++++++ 2 files changed, 129 insertions(+), 16 deletions(-) diff --git a/dwca/files.py b/dwca/files.py index f6dc388..ad3488f 100644 --- a/dwca/files.py +++ b/dwca/files.py @@ -1,9 +1,11 @@ """File-related classes and functions.""" +import csv import io import os from array import array -from typing import List, Union, IO, Dict, Optional +from itertools import islice +from typing import Iterator, List, Union, IO, Dict, Optional from dwca.descriptors import DataFileDescriptor from dwca.rows import CoreRow, ExtensionRow, Row @@ -38,12 +40,11 @@ def __init__( #: constructor. self.file_descriptor = file_descriptor # type: DataFileDescriptor - self._file_stream = io.open( - os.path.join(work_directory, self.file_descriptor.file_location), - mode="r", - encoding=self.file_descriptor.file_encoding, - newline=self.file_descriptor.lines_terminated_by, - errors="replace", + self._file_path = os.path.join( + work_directory, self.file_descriptor.file_location + ) + self._file_stream = self._open_stream( + newline=self.file_descriptor.lines_terminated_by ) # On init, we parse the file once to build an index of newlines (including lines to ignore) @@ -60,10 +61,67 @@ def __init__( def __str__(self) -> str: return self.file_descriptor.file_location + def _open_stream(self, newline: str) -> IO: + return io.open( + self._file_path, + mode="r", + encoding=self.file_descriptor.file_encoding, + newline=newline, + errors="replace", + ) + + def _iter_field_lists(self) -> Iterator[List[str]]: + """Yield each data row of the file as a list of raw field values. + + Header lines are skipped. This is a single forward pass over a dedicated stream, so + it is safe to run several of these concurrently and alongside random access. + """ + descriptor = self.file_descriptor + quoted = descriptor.fields_enclosed_by != "" + + # The csv module refuses a stream that can hand it an embedded carriage return, so + # the quoted path has to let Python handle newlines. The unquoted path keeps the + # archive's own terminator, which is what stops U+0085 from being treated as a line + # break (issue #20). + stream = self._open_stream( + newline="" if quoted else descriptor.lines_terminated_by + ) + try: + if quoted: + source = csv.reader( + stream, + delimiter=descriptor.fields_terminated_by, + quotechar=descriptor.fields_enclosed_by, + quoting=csv.QUOTE_MINIMAL, + ) # type: Iterator[List[str]] + else: + line_ending = descriptor.lines_terminated_by + separator = descriptor.fields_terminated_by + source = (line.rstrip(line_ending).split(separator) for line in stream) + + for fields in islice(source, self.lines_to_ignore, None): + yield fields + finally: + stream.close() + + def iter_rows(self) -> Iterator[Union[CoreRow, ExtensionRow]]: + """Yield every data row of the file, in order of appearance. + + Unlike repeated :meth:`get_row_by_position` calls this is a single forward pass and + never touches the line offset index. + """ + descriptor = self.file_descriptor + row_class = CoreRow if descriptor.represents_corefile else ExtensionRow + + for position, fields in enumerate(self._iter_field_lists()): + yield row_class.from_fields(fields, position, descriptor) + def _position_file_after_header(self) -> None: self._file_stream.seek(0, 0) - if self.lines_to_ignore > 0: - self._file_stream.readlines(self.lines_to_ignore) + # NOTE: readlines() takes a byte-size hint, not a line count, so it cannot be used + # here. With ignoreHeaderLines="2" it would read a single line. + for _ in range(self.lines_to_ignore): + self._file_stream.readline() def __iter__(self) -> "CSVDataFile": self._position_file_after_header() @@ -108,13 +166,10 @@ def _build_coreid_index(self) -> Dict[str, List[int]]: """Build and return an index of Core Rows IDs suitable for `CSVDataFile.coreid_index`.""" index = {} # type: Dict[str, array[int]] - for position, row in enumerate(self): - if self.file_descriptor.represents_corefile: - tmp = CoreRow(row, position, self.file_descriptor) - index.setdefault(tmp.id, array("L")).append(position) - else: - tmp = ExtensionRow(row, position, self.file_descriptor) - index.setdefault(tmp.core_id, array("L")).append(position) + represents_corefile = self.file_descriptor.represents_corefile + for row in self.iter_rows(): + key = row.id if represents_corefile else row.core_id + index.setdefault(key, array("L")).append(row.position) return index diff --git a/dwca/test/test_datafile.py b/dwca/test/test_datafile.py index 2808ed1..7484604 100644 --- a/dwca/test/test_datafile.py +++ b/dwca/test/test_datafile.py @@ -160,3 +160,61 @@ def test_iterate(self): for row in data_file: assert isinstance(row, str) + + +class TestStreamingIteration(unittest.TestCase): + def test_iter_rows_yields_every_row_in_order(self): + with DwCAReader(sample_data_path("dwca-ids.zip")) as dwca: + rows = list(dwca.core_file.iter_rows()) + + # Row IDs appear in the core file in this order: 4-1-3-2 + assert ["4", "1", "3", "2"] == [row.id for row in rows] + assert [0, 1, 2, 3] == [row.position for row in rows] + + def test_iter_rows_agrees_with_random_access(self): + with DwCAReader(sample_data_path("dwca-2extensions.zip")) as dwca: + for data_file in [dwca.core_file] + dwca.extension_files: + streamed = list(data_file.iter_rows()) + seeked = [ + data_file.get_row_by_position(i) for i in range(len(streamed)) + ] + + assert [r.data for r in streamed] == [r.data for r in seeked] + assert [r.raw_fields for r in streamed] == [ + r.raw_fields for r in seeked + ] + + def test_iter_rows_can_be_nested(self): + """Each call gets its own stream, so concurrent passes do not interfere.""" + with DwCAReader(sample_data_path("dwca-ids.zip")) as dwca: + pairs = [ + (outer.id, inner.id) + for outer in dwca.core_file.iter_rows() + for inner in dwca.core_file.iter_rows() + ] + + assert 16 == len(pairs) + + def test_iter_rows_on_a_quoted_archive(self): + with DwCAReader(sample_data_path("dwca-csv-quote-dir")) as dwca: + rows = list(dwca.core_file.iter_rows()) + + assert 2 == len(rows) + + def test_raw_line_iteration_skips_every_header_line(self): + """readlines() takes a byte-size hint, not a line count, so with two header lines + the second used to leak through as data.""" + from .archive_builder import build_archive, temp_archive_dir + + path = build_archive( + temp_archive_dir(self), + rows=[["1", "Borneo"], ["2", "Mumbai"]], + ignore_header_lines=2, + header_rows=[["idA", "locA"], ["idB", "locB"]], + ) + with DwCAReader(path) as dwca: + lines = list(dwca.core_file) + + assert 2 == len(lines) + assert lines[0].startswith("1\t") + assert lines[1].startswith("2\t") From 9b348e0214e1b170b5299c42f743839ead99a3ad Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Nicolas=20No=C3=A9?= Date: Tue, 28 Jul 2026 14:50:51 +0200 Subject: [PATCH 13/33] Build the line offset index lazily and from the binary layer --- dwca/files.py | 90 +++++++++++++++++++++++++++----------- dwca/test/test_datafile.py | 66 ++++++++++++++++++++++++++++ 2 files changed, 130 insertions(+), 26 deletions(-) diff --git a/dwca/files.py b/dwca/files.py index ad3488f..2b2fb0a 100644 --- a/dwca/files.py +++ b/dwca/files.py @@ -27,8 +27,9 @@ class CSVDataFile(object): * For an extension data file, with :meth:`get_all_rows_by_coreid` (A :class:`dwca.rows.CoreRow` or \ :class:`dwca.rows.ExtensionRow` object is returned) - On initialization, an index of new lines is build. This may take time, but makes random access\ - faster. + An index of line offsets is built on first random access (:meth:`get_row_by_position` or\ + :meth:`get_all_rows_by_coreid`). This may take time for large files, but makes further random\ + access faster. """ # TODO: More tests for this class @@ -47,11 +48,9 @@ def __init__( newline=self.file_descriptor.lines_terminated_by ) - # On init, we parse the file once to build an index of newlines (including lines to ignore) - # that will make random access faster later on... - self._line_offsets = _get_all_line_offsets( - self._file_stream, self.file_descriptor.file_encoding - ) + # The index of line offsets is only needed for random access, so it is built on + # first use rather than here: opening an archive stays O(1) whatever its size. + self._line_offsets = None # type: Optional[array] #: Number of lines to ignore (header lines) in the CSV file. self.lines_to_ignore = self.file_descriptor.lines_to_ignore # type: int @@ -70,6 +69,15 @@ def _open_stream(self, newline: str) -> IO: errors="replace", ) + def _get_line_offsets(self) -> array: + if self._line_offsets is None: + self._line_offsets = _build_line_offsets( + self._file_path, + self.file_descriptor.file_encoding, + self.file_descriptor.lines_terminated_by, + ) + return self._line_offsets + def _iter_field_lists(self) -> Iterator[List[str]]: """Yield each data row of the file as a list of raw field values. @@ -198,7 +206,11 @@ def get_row_by_position(self, position: int) -> Union[CoreRow, ExtensionRow]: # Raises IndexError if position is incorrect def _get_line_by_position(self, position: int) -> str: - self._file_stream.seek(self._line_offsets[position + self.lines_to_ignore], 0) + if self._file_stream.closed: + raise ValueError("The data file has been closed.") + + offsets = self._get_line_offsets() + self._file_stream.seek(offsets[position + self.lines_to_ignore], 0) return self._file_stream.readline() def close(self) -> None: @@ -209,26 +221,52 @@ def close(self) -> None: self._file_stream.close() -def _get_all_line_offsets(f: IO, encoding: str) -> array: - """Parse the file whose handler is given and return an array (long) containing the start offset\ - of each line. +def _build_line_offsets( + path: str, encoding: str, lines_terminated_by: str, chunk_size: int = 1024 * 1024 +) -> array: + """Return an array of the byte offset of every line in the file at `path`. - The values in the array are suitable for seek() operations. + The values are suitable for seek() on a stream opened with the same encoding. - This function can take long for large files. + The file is scanned as bytes rather than as decoded text: decoding with + errors="replace" turns an undecodable byte into U+FFFD, which does not re-encode to the + same length, so computing offsets from decoded text desynchronises the index from the + file. - It needs to know the file encoding to properly count the bytes in a given string. + An array of Longs is used instead of a list. It is much more memory efficient, and a few + tests with 1-4Gb uncompressed archives didn't show any significant slowdown. """ - f.seek(0, 0) - - # We use an array of Longs instead of a list to store the index. - # It's much more memory efficient, and a few tests w/ 1-4Gb uncompressed archives - # didn't show any significant slowdown (see benchmarks/ for current measurements). - line_offsets = array("L") - offset = 0 - for line in f: - line_offsets.append(offset) - offset += len(line.encode(encoding)) - - f.seek(0, 0) + terminator = lines_terminated_by.encode(encoding) + terminator_length = len(terminator) + + line_offsets = array("L", [0]) + buffer = b"" + base = 0 # absolute offset of buffer[0] within the file + + with io.open(path, "rb") as f: + while True: + chunk = f.read(chunk_size) + if not chunk: + break + + buffer += chunk + consumed = 0 + while True: + found = buffer.find(terminator, consumed) + if found == -1: + break + consumed = found + terminator_length + line_offsets.append(base + consumed) + + # Whatever follows the last terminator stays in the buffer: it may be an + # unfinished line, or a terminator straddling the chunk boundary. + base += consumed + buffer = buffer[consumed:] + + # A file ending with the terminator has no extra empty line after it, so the offset + # pointing at EOF is spurious. This also empties the index for a zero-byte file, which + # must report no lines at all rather than one empty one. + if line_offsets and line_offsets[-1] == os.path.getsize(path): + line_offsets.pop() + return line_offsets diff --git a/dwca/test/test_datafile.py b/dwca/test/test_datafile.py index 7484604..20be194 100644 --- a/dwca/test/test_datafile.py +++ b/dwca/test/test_datafile.py @@ -1,3 +1,4 @@ +import os import unittest import xml.etree.ElementTree as ET from array import array @@ -218,3 +219,68 @@ def test_raw_line_iteration_skips_every_header_line(self): assert 2 == len(lines) assert lines[0].startswith("1\t") assert lines[1].startswith("2\t") + + +class TestLineOffsets(unittest.TestCase): + def test_opening_an_archive_does_not_build_the_index(self): + with DwCAReader(sample_data_path("dwca-2extensions.zip")) as dwca: + assert dwca.core_file._line_offsets is None + + dwca.core_file.get_row_by_position(0) + + assert dwca.core_file._line_offsets is not None + + def test_offsets_survive_an_undecodable_byte(self): + from .archive_builder import build_archive, temp_archive_dir + + path = build_archive( + temp_archive_dir(self), + rows=[], + columns=2, + raw_payload=b"1\tcaf\xe9\n2\tMumbai\n3\tBorneo\n", + ) + with DwCAReader(path) as dwca: + term = "http://rs.tdwg.org/dwc/terms/term1" + + assert "Mumbai" == dwca.core_file.get_row_by_position(1).data[term] + assert "Borneo" == dwca.core_file.get_row_by_position(2).data[term] + + def test_offsets_with_dos_line_endings(self): + from .archive_builder import build_archive, temp_archive_dir + + path = build_archive( + temp_archive_dir(self), + rows=[["1", "Borneo"], ["2", "Mumbai"]], + lines_terminated_by="\r\n", + ) + with DwCAReader(path) as dwca: + term = "http://rs.tdwg.org/dwc/terms/term1" + + assert "Borneo" == dwca.core_file.get_row_by_position(0).data[term] + assert "Mumbai" == dwca.core_file.get_row_by_position(1).data[term] + + def test_offsets_are_correct_across_a_chunk_boundary(self): + """The scanner reads in chunks, so a terminator can straddle two reads.""" + from dwca.files import _build_line_offsets + from .archive_builder import build_archive, temp_archive_dir + + rows = [[str(i), "locality-" + str(i)] for i in range(5000)] + path = build_archive(temp_archive_dir(self), rows=rows) + + data_path = os.path.join(path, "occurrence.txt") + reference = _build_line_offsets(data_path, "utf-8", "\n") + chunked = _build_line_offsets(data_path, "utf-8", "\n", chunk_size=7) + + assert 5000 == len(reference) + assert list(reference) == list(chunked) + + def test_a_zero_byte_core_file_has_no_rows(self): + """A zero-byte file must report no lines at all, not one spurious empty line.""" + from .archive_builder import build_archive, temp_archive_dir + + path = build_archive( + temp_archive_dir(self), rows=[], columns=2, raw_payload=b"" + ) + with DwCAReader(path) as dwca: + with pytest.raises(IndexError): + dwca.core_file.get_row_by_position(0) From 6dd7c7a89570d28bcbd2ee2622f2e11e3a325afe Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Nicolas=20No=C3=A9?= Date: Tue, 28 Jul 2026 14:57:04 +0200 Subject: [PATCH 14/33] Give each iteration of a reader its own iterator --- dwca/read.py | 41 ++++++++++++++++++++++++------------ dwca/test/test_dwcareader.py | 23 ++++++++++++++++++++ 2 files changed, 51 insertions(+), 13 deletions(-) diff --git a/dwca/read.py b/dwca/read.py index f4a152e..ebd476d 100644 --- a/dwca/read.py +++ b/dwca/read.py @@ -7,7 +7,7 @@ import zipfile from errno import ENOENT from tempfile import mkdtemp -from typing import List, Optional, Dict, Any, IO, Tuple +from typing import Iterator, List, Optional, Dict, Any, IO, Tuple from xml.etree.ElementTree import Element import dwca.vendor @@ -100,6 +100,7 @@ def __init__( #: The path to the Darwin Core Archive file, as passed to the constructor. self.archive_path = path # type: str + self._default_iterator = None # type: Optional[Iterator[CoreRow]] if os.path.isdir( self.archive_path @@ -536,22 +537,36 @@ def core_contains_term(self, term_url: str) -> bool: """Return `True` if the Core file of the archive contains the `term_url` term.""" return term_url in self.core_file.file_descriptor.terms - def __iter__(self) -> "DwCAReader": - self._corefile_pointer = 0 - return self + def __iter__(self) -> Iterator[CoreRow]: + # A fresh iterator each time, so nesting loops (or calling get_corerow_by_id() from + # inside one) behaves as expected. + return self._iter_core_rows() + + def _iter_core_rows(self) -> Iterator[CoreRow]: + extension_files = self.extension_files + source_metadata = self.source_metadata + + for row in self.core_file.iter_rows(): + # Set up linked data so the CoreRow will know about them + row.link_extension_files(extension_files) + row.link_source_metadata(source_metadata) + yield row def __next__(self): return self.next() def next(self) -> CoreRow: # NOQA - try: - row = self.core_file.get_row_by_position(self._corefile_pointer) + """Return the next core row. - # Set up linked data so the CoreRow will know about them - row.link_extension_files(self.extension_files) - row.link_source_metadata(self.source_metadata) + .. deprecated:: + Iterate over the reader instead. This method shares a single implicit iterator + between all callers. + """ + if self._default_iterator is None: + self._default_iterator = self._iter_core_rows() - self._corefile_pointer = self._corefile_pointer + 1 - return row - except IndexError: - raise StopIteration + try: + return next(self._default_iterator) + except StopIteration: + self._default_iterator = None + raise diff --git a/dwca/test/test_dwcareader.py b/dwca/test/test_dwcareader.py index f5a55b5..4b9d42d 100644 --- a/dwca/test/test_dwcareader.py +++ b/dwca/test/test_dwcareader.py @@ -1077,6 +1077,29 @@ def test_whitespace_before_xml_tag(self): # The next line will throw an exception if metadata.xml can't be parsed DwCAReader(sample_data_path("gbif-results-whitespace-in-xml.zip")) + def test_nested_iteration_is_independent(self): + with DwCAReader(sample_data_path("dwca-ids.zip")) as dwca: + pairs = [(outer.id, inner.id) for outer in dwca for inner in dwca] + + assert 16 == len(pairs) + + def test_lookup_inside_a_loop_terminates(self): + with DwCAReader(sample_data_path("dwca-ids.zip")) as dwca: + seen = [] + for row in dwca: + seen.append(row.id) + dwca.get_corerow_by_id("1") + + assert ["4", "1", "3", "2"] == seen + + def test_next_still_works(self): + dwca = DwCAReader(sample_data_path("dwca-ids.zip")) + try: + assert "4" == dwca.next().id + assert "1" == dwca.next().id + finally: + dwca.close() + if __name__ == "__main__": unittest.main() From c48124009f02fa24b0d5b3f853036f7b8f302f0a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Nicolas=20No=C3=A9?= Date: Tue, 28 Jul 2026 15:06:18 +0200 Subject: [PATCH 15/33] Record the behavior changes brought by the streaming engine --- CHANGES.txt | 17 +++++++ dwca/read.py | 7 ++- dwca/test/test_characterization.py | 75 +++++++++--------------------- 3 files changed, 43 insertions(+), 56 deletions(-) diff --git a/CHANGES.txt b/CHANGES.txt index 806b2a4..147f233 100644 --- a/CHANGES.txt +++ b/CHANGES.txt @@ -14,6 +14,23 @@ v0.17.0 (unreleased) hashes) by value: the data file layout it describes. - Fixed: comparing a CoreRow to an ExtensionRow (or either to a non-row object) raised AttributeError instead of returning False. +- Performance: iterating over an archive is a single streaming pass instead of one seek and + one throwaway csv.reader per row. Roughly 3.5x faster on a 400000-row GBIF download. +- Performance: the line offset index used for random access is now built on first use, so + opening an archive no longer scans every data file. +- Fixed: ignoreHeaderLines values above 1 only skipped a single line when iterating a + CSVDataFile directly, which let header lines leak into coreid_index and + orphaned_extension_rows. +- Fixed: an undecodable byte in a data file desynchronised the line offset index, so rows + after it were silently truncated or rejected. +- Fixed: a field whose content started or ended with the archive's fieldsEnclosedBy + character had that character stripped. +- Fixed: DwCAReader was its own iterator, so nesting two loops over the same reader silently + ran the inner one once, and calling get_corerow_by_id() or get_corerow_by_position() from + inside a loop never terminated. Each iteration now gets its own iterator. +- Changed: DwCAReader.next() now keeps an implicit iterator independent of any for loop over + the same reader, and starts a new pass after being exhausted. Iterate over the reader + instead; next() remains only for backwards compatibility. v0.16.4 (2024-10-18) -------------------- diff --git a/dwca/read.py b/dwca/read.py index ebd476d..84ea2ac 100644 --- a/dwca/read.py +++ b/dwca/read.py @@ -559,8 +559,11 @@ def next(self) -> CoreRow: # NOQA """Return the next core row. .. deprecated:: - Iterate over the reader instead. This method shares a single implicit iterator - between all callers. + Iterate over the reader instead. This method keeps its own implicit iterator, + which is independent of any `for row in reader:` loop: interleaving the two makes + each of them scan the archive separately. Once this iterator is exhausted it + raises StopIteration, and the call after that starts a fresh pass from the first + row rather than raising again. """ if self._default_iterator is None: self._default_iterator = self._iter_core_rows() diff --git a/dwca/test/test_characterization.py b/dwca/test/test_characterization.py index 901dd1a..c55b6e3 100644 --- a/dwca/test/test_characterization.py +++ b/dwca/test/test_characterization.py @@ -46,10 +46,10 @@ def test_raw_payload_is_written_verbatim(self): class TestHeaderLines(unittest.TestCase): """ignoreHeaderLines, for BOTH access paths. - The two paths disagree today: DwCAReader iteration goes through get_row_by_position() - (which offsets correctly), while CSVDataFile.__iter__ uses readlines(hint) where the - argument is a byte-size hint rather than a line count. coreid_index is built from the - second path, so it picks up leftover header lines. + The two paths used to disagree: DwCAReader iteration went through get_row_by_position() + (which offset correctly), while CSVDataFile.__iter__ used readlines(hint), where the + argument is a byte-size hint rather than a line count. coreid_index was built from the + second path, so it used to pick up leftover header lines. Both paths now agree. """ def _archive(self, ignore_header_lines, header_rows): @@ -84,9 +84,7 @@ def test_two_headers_iteration_and_random_access_agree(self): assert ["1", "2"] == [row.id for row in dwca] assert "1" == dwca.core_file.get_row_by_position(0).id - # CHARACTERIZATION: wrong, see B1. readlines() takes a byte-size hint, not a - # line count, so the second header line leaks into the index as a data row. - assert {"idB": [0], "1": [1], "2": [2]} == { + assert {"1": [0], "2": [1]} == { k: list(v) for k, v in dwca.core_file.coreid_index.items() } @@ -180,7 +178,7 @@ def test_utf8_bom_leaks_into_the_first_field(self): # BOM handling would change this deliberately. assert "\ufeff1" == rows[0].id - def test_undecodable_byte_desynchronises_random_access(self): + def test_undecodable_byte_does_not_break_random_access(self): path = build_archive( temp_archive_dir(self), rows=[], @@ -192,18 +190,13 @@ def test_undecodable_byte_desynchronises_random_access(self): # Row 0 is fine: the offset index has not drifted yet. assert "caf\ufffd" == dwca.core_file.get_row_by_position(0).data[TERM1] - # CHARACTERIZATION: wrong, see B2. errors="replace" turns the undecodable byte - # into U+FFFD, which re-encodes to three bytes instead of one, so every offset - # after it is two bytes too large. The seek lands mid-row and the truncated row - # no longer has enough columns. - with pytest.raises(InvalidArchive): - dwca.core_file.get_row_by_position(1) + assert "Mumbai" == dwca.core_file.get_row_by_position(1).data[ + "http://rs.tdwg.org/dwc/terms/term1" + ] - # CHARACTERIZATION: wrong, see B2. Iteration is broken too, because today it is - # implemented as repeated get_row_by_position() calls. After Phase 1 this reads - # ["caf\ufffd", "Mumbai", "Borneo"], which is the whole point of the fix. - with pytest.raises(InvalidArchive): - list(dwca) + assert ["caf\ufffd", "Mumbai", "Borneo"] == [ + row.data["http://rs.tdwg.org/dwc/terms/term1"] for row in dwca + ] class TestQuoting(unittest.TestCase): @@ -229,13 +222,9 @@ def test_delimiter_inside_a_quoted_field(self): def test_quote_in_the_middle_of_content(self): assert ['say "hi" there'] == self._read_localities(b'"1","say ""hi"" there"\n') - def test_quote_at_the_edge_of_content_is_eaten(self): - # CHARACTERIZATION: wrong, see B3. csv parses this correctly to 'say "hi"', then - # the trailing .strip(fields_enclosed_by) removes the legitimate closing quote. - assert ['say "hi'] == self._read_localities(b'"1","say ""hi"""\n') - - # Same at the start of the field. - assert ['hi" she said'] == self._read_localities(b'"1","""hi"" she said"\n') + def test_quote_at_the_edge_of_content_is_preserved(self): + assert ['say "hi"'] == self._read_localities(b'"1","say ""hi"""\n') + assert ['"hi" she said'] == self._read_localities(b'"1","""hi"" she said"\n') def test_quote_characters_are_kept_when_the_archive_declares_no_enclosure(self): path = build_archive( @@ -333,32 +322,18 @@ def test_sequential_re_iteration(self): def test_nested_iteration(self): with DwCAReader(self._archive()) as dwca: - pairs = [] - for outer in dwca: - for inner in dwca: - pairs.append((outer.id, inner.id)) - if len(pairs) > 20: - break + pairs = [(outer.id, inner.id) for outer in dwca for inner in dwca] - # CHARACTERIZATION: wrong, see B4. DwCAReader is its own iterator with one shared - # pointer, so the inner loop consumes it and the outer loop ends after one pass. - assert 3 == len(pairs) + assert 9 == len(pairs) def test_lookup_inside_a_loop(self): seen = [] with DwCAReader(self._archive()) as dwca: for row in dwca: seen.append(row.id) - dwca.get_corerow_by_id("2") - if len(seen) > 8: - break + assert "2" == dwca.get_corerow_by_id("2").id - # CHARACTERIZATION: wrong, see B4. get_corerow_by_id() resets the shared pointer, - # so this never terminates. Without the break it would loop forever. The exact value - # (rather than a looser bound) makes a future fix's effect on this test unambiguous: - # get_corerow_by_id("2") always rewinds to id "2", so after the first row (id "1"), - # every subsequent row is "3" until the len(seen) > 8 guard fires. - assert 9 == len(seen) + assert ["1", "2", "3"] == seen def test_random_access_interleaved_with_iteration_is_safe(self): with DwCAReader(self._archive()) as dwca: @@ -433,7 +408,7 @@ def test_core_row_extensions_content_and_ordering(self): ] assert ["monkey"] == [r.data[VERNACULAR_TERM] for r in rows[1].extensions] - def test_ignore_header_lines_leaks_into_extension_index(self): + def test_ignore_header_lines_are_skipped_in_the_extension_index(self): path = self._archive( ignore_header_lines=2, header_rows=[["idA", "locA"], ["idB", "locB"]] ) @@ -441,15 +416,7 @@ def test_ignore_header_lines_leaks_into_extension_index(self): with DwCAReader(path) as dwca: index = dwca.extension_files[0].coreid_index - # CHARACTERIZATION: wrong, see B1. Same readlines(byte-hint) bug as the core file - # (TestHeaderLines.test_two_headers_iteration_and_random_access_agree): the second - # header line leaks into the index as a data row referencing core id "idB", and - # every position after it is shifted by one. - assert { - "idB": array("L", [0]), - "1": array("L", [1, 2]), - "2": array("L", [3]), - } == index + assert {"1": array("L", [0, 1]), "2": array("L", [2])} == index class TestRandomAccessDirect(unittest.TestCase): From 9948ddecba09d1ec4f49755fa4989a4c97b82371 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Nicolas=20No=C3=A9?= Date: Tue, 28 Jul 2026 15:22:52 +0200 Subject: [PATCH 16/33] Record the streaming engine benchmark and refresh stale docstrings 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. --- CHANGES.txt | 3 +- benchmarks/README.md | 69 ++++++++++++++++++++++++++++++++++++++ benchmarks/bench_reader.py | 11 +++--- dwca/files.py | 1 - 4 files changed, 77 insertions(+), 7 deletions(-) diff --git a/CHANGES.txt b/CHANGES.txt index 147f233..185a522 100644 --- a/CHANGES.txt +++ b/CHANGES.txt @@ -15,7 +15,8 @@ v0.17.0 (unreleased) - Fixed: comparing a CoreRow to an ExtensionRow (or either to a non-row object) raised AttributeError instead of returning False. - Performance: iterating over an archive is a single streaming pass instead of one seek and - one throwaway csv.reader per row. Roughly 3.5x faster on a 400000-row GBIF download. + one throwaway csv.reader per row. Measured roughly 3.8-4.0x faster on a 400000-row GBIF + download on one machine; see benchmarks/README.md for the full before/after measurement. - Performance: the line offset index used for random access is now built on first use, so opening an archive no longer scans every data file. - Fixed: ignoreHeaderLines values above 1 only skipped a single line when iterating a diff --git a/benchmarks/README.md b/benchmarks/README.md index 4f2eb28..ef626f2 100644 --- a/benchmarks/README.md +++ b/benchmarks/README.md @@ -67,3 +67,72 @@ calls on an already-built dict add only marginal cost on top of the row-parsing both variants pay. The gap between the two iterate variants is a better indicator of "reading fields" cost added ON TOP of parsing than a full picture of parsing cost itself, which the "no field access" line represents. + +## After the streaming engine + +Measured on: + +- Commit: `c48124009f02fa24b0d5b3f853036f7b8f302f0a` (branch `parsing-performance`) +- Python: 3.12.0 (CPython) +- Machine: MacBook Pro (Mac14,5, Apple Silicon, arm64), macOS 26.5 + +The Phase 0 baseline above was recorded in a separate session. Machine variance between +sessions has been observed to be as large as ~40 percent on identical code, so it is not a +trustworthy comparison by itself. To get an honest pair, both sides below were re-measured +back to back, in one sitting, on an otherwise idle machine: the Phase 0 starting commit +(`fd829b6`, checked out into a scratch worktree) immediately followed by the current commit +above, against the same generated archive. + +Generator output (shared by both sides): + + wrote 400000 rows, 50 columns, 248MB to /tmp/dwca-bench + +Before (commit `fd829b61962b5a813705a0f7c5d1f12c1efe07e7`, run 1 of 2): + + archive: /tmp/dwca-bench + open archive 0.23s n=occurrence.txt peak=73MB + iterate, no field access 6.76s n=400000 peak=73MB + iterate + read 14 terms 6.84s n=400000 peak=73MB + random access, every 7th row up to 100k 0.40s n=14286 peak=73MB + +Before, run 2 of 2 (same archive, same process type, run immediately after): + + archive: /tmp/dwca-bench + open archive 0.16s n=occurrence.txt peak=71MB + iterate, no field access 6.80s n=400000 peak=71MB + iterate + read 14 terms 6.88s n=400000 peak=71MB + random access, every 7th row up to 100k 0.38s n=14286 peak=71MB + +After (commit `c48124009f02fa24b0d5b3f853036f7b8f302f0a`, run 1 of 2, measured immediately +after the "before" runs, same archive): + + archive: /tmp/dwca-bench + open archive 0.00s n=occurrence.txt peak=67MB + iterate, no field access 1.65s n=400000 peak=67MB + iterate + read 14 terms 1.74s n=400000 peak=67MB + random access, every 7th row up to 100k 0.26s n=14286 peak=79MB + +After, run 2 of 2: + + archive: /tmp/dwca-bench + open archive 0.00s n=occurrence.txt peak=67MB + iterate, no field access 1.68s n=400000 peak=67MB + iterate + read 14 terms 1.79s n=400000 peak=67MB + random access, every 7th row up to 100k 0.19s n=14286 peak=76MB + +Both sides are consistent run to run (within a few percent). Using the average of the two +runs on each side: + +- `open archive`: 0.20s -> 0.00s (below the timer's resolution; opening no longer scans the + data file to build the line offset index, it is now built lazily on first positional + access). +- `iterate, no field access`: 6.78s -> 1.67s, roughly 4.1x faster. +- `iterate + read 14 terms`: 6.86s -> 1.77s, roughly 3.9x faster (range 3.8x-4.0x across the + two run pairs). This is the headline number: it clears the phase's 2.5x target by a wide + margin. +- `random access, every 7th row up to 100k`: 0.39s -> 0.23s, roughly 1.7x faster. + +`peak=` rose slightly on the "after" random access line (79MB / 76MB vs 67MB elsewhere in +the same runs) because that is the first operation in the process that builds the line +offset index; it remains well below the "before" side's peak, where the index was built +eagerly on open. diff --git a/benchmarks/bench_reader.py b/benchmarks/bench_reader.py index 11bbbd3..797b2e9 100644 --- a/benchmarks/bench_reader.py +++ b/benchmarks/bench_reader.py @@ -63,11 +63,12 @@ def iterate(read_fields): def random_access(): with DwCAReader(archive_path, skip_metadata=True) as reader: data_file = reader.core_file - # No public API exposes the row count. _line_offsets is already built when the - # CSVDataFile is opened (that's the whole point of the index), so reading its - # length here is free and lets the range below scale with the actual archive - # instead of hardcoding 100000 (which raises IndexError on smaller archives). - row_count = len(data_file._line_offsets) - data_file.lines_to_ignore + # No public API exposes the row count. _line_offsets is built lazily on first + # positional access, so we go through _get_line_offsets() (which builds it if + # needed) rather than reading the raw attribute directly. This lets the range + # below scale with the actual archive instead of hardcoding 100000 (which raises + # IndexError on smaller archives). + row_count = len(data_file._get_line_offsets()) - data_file.lines_to_ignore upper_bound = min(row_count, 100000) return sum( 1 diff --git a/dwca/files.py b/dwca/files.py index 2b2fb0a..d6489c3 100644 --- a/dwca/files.py +++ b/dwca/files.py @@ -32,7 +32,6 @@ class CSVDataFile(object): access faster. """ - # TODO: More tests for this class def __init__( self, work_directory: str, file_descriptor: DataFileDescriptor ) -> None: From 3ba01c1b7981b108c3567e95d28632c900fe2b82 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Nicolas=20No=C3=A9?= Date: Tue, 28 Jul 2026 15:58:02 +0200 Subject: [PATCH 17/33] Index CSV records rather than physical lines --- CHANGES.txt | 3 + dwca/files.py | 223 ++++++++++++++++++++++++++--- dwca/test/test_characterization.py | 45 ++++++ dwca/test/test_datafile.py | 46 ++++++ 4 files changed, 298 insertions(+), 19 deletions(-) diff --git a/CHANGES.txt b/CHANGES.txt index 185a522..6fe63c8 100644 --- a/CHANGES.txt +++ b/CHANGES.txt @@ -32,6 +32,9 @@ v0.17.0 (unreleased) - Changed: DwCAReader.next() now keeps an implicit iterator independent of any for loop over the same reader, and starts a new pass after being exhausted. Iterate over the reader instead; next() remains only for backwards compatibility. +- Fixed: in archives using fieldsEnclosedBy, a field containing the line terminator made + random access and iteration disagree, so CoreRow.extensions could return silently truncated + rows. The line offset index now indexes CSV records rather than physical lines. v0.16.4 (2024-10-18) -------------------- diff --git a/dwca/files.py b/dwca/files.py index d6489c3..b7742d0 100644 --- a/dwca/files.py +++ b/dwca/files.py @@ -47,6 +47,10 @@ def __init__( newline=self.file_descriptor.lines_terminated_by ) + # Opened lazily, on first random-access read: a dedicated binary stream used to + # fetch the exact byte range of a record (see _read_record). + self._binary_stream = None # type: Optional[IO] + # The index of line offsets is only needed for random access, so it is built on # first use rather than here: opening an archive stays O(1) whatever its size. self._line_offsets = None # type: Optional[array] @@ -70,10 +74,13 @@ def _open_stream(self, newline: str) -> IO: def _get_line_offsets(self) -> array: if self._line_offsets is None: + descriptor = self.file_descriptor self._line_offsets = _build_line_offsets( self._file_path, - self.file_descriptor.file_encoding, - self.file_descriptor.lines_terminated_by, + descriptor.file_encoding, + descriptor.lines_terminated_by, + descriptor.fields_enclosed_by, + descriptor.fields_terminated_by, ) return self._line_offsets @@ -209,8 +216,44 @@ def _get_line_by_position(self, position: int) -> str: raise ValueError("The data file has been closed.") offsets = self._get_line_offsets() - self._file_stream.seek(offsets[position + self.lines_to_ignore], 0) - return self._file_stream.readline() + index = position + self.lines_to_ignore + return self._read_record(offsets, index) + + def _get_binary_stream(self) -> IO: + # A dedicated handle, separate from self._file_stream: random access reads the + # exact byte range of a record (see _read_record), and must not disturb the + # position of the text stream used for sequential iteration. + if self._binary_stream is None: + self._binary_stream = io.open(self._file_path, mode="rb") + return self._binary_stream + + def _read_record(self, offsets: array, index: int) -> str: + """Return the full text of the record starting at offsets[index]. + + offsets holds byte offsets, computed by scanning the file as bytes (see + _build_line_offsets). A text stream's read(n) counts characters, not bytes, so + turning a byte delta into a character count would silently truncate or over-read + wherever a character takes more than one byte in the file's encoding (e.g. + multi-byte UTF-8). Reading the exact byte range through a dedicated binary stream + and decoding it afterwards keeps this correct regardless of encoding, and also + regardless of how many physical lines the record spans (a quoted field may + legally contain the line terminator). + + `index` supports the same negative-indexing quirk as a plain list/array + (get_row_by_position(-1) is pinned by TestNegativePosition), so `offsets[index]` + is used rather than any manual wraparound arithmetic. + """ + start = offsets[index] + stream = self._get_binary_stream() + stream.seek(start, 0) + + following = index + 1 + if following < len(offsets) and offsets[following] > start: + raw = stream.read(offsets[following] - start) + else: + raw = stream.read() + + return raw.decode(self.file_descriptor.file_encoding, errors="replace") def close(self) -> None: """Close the file. @@ -218,15 +261,27 @@ def close(self) -> None: The content of the file will not be accessible in any way afterwards. """ self._file_stream.close() + if self._binary_stream is not None: + self._binary_stream.close() def _build_line_offsets( - path: str, encoding: str, lines_terminated_by: str, chunk_size: int = 1024 * 1024 + path: str, + encoding: str, + lines_terminated_by: str, + fields_enclosed_by: str = "", + fields_terminated_by: str = "\t", + chunk_size: int = 1024 * 1024, ) -> array: - """Return an array of the byte offset of every line in the file at `path`. + """Return an array of the byte offset of every CSV record in the file at `path`. The values are suitable for seek() on a stream opened with the same encoding. + Without an enclosure character every terminator ends a record, so this is a plain scan. + With one, a terminator inside an enclosed field is data rather than a record boundary + (a quoted field may legally contain the line terminator), so the scan tracks whether it + is inside an enclosure. + The file is scanned as bytes rather than as decoded text: decoding with errors="replace" turns an undecodable byte into U+FFFD, which does not re-encode to the same length, so computing offsets from decoded text desynchronises the index from the @@ -236,36 +291,166 @@ def _build_line_offsets( tests with 1-4Gb uncompressed archives didn't show any significant slowdown. """ terminator = lines_terminated_by.encode(encoding) - terminator_length = len(terminator) + tlen = len(terminator) - line_offsets = array("L", [0]) - buffer = b"" - base = 0 # absolute offset of buffer[0] within the file + if not fields_enclosed_by: + return _scan_plain(path, terminator, tlen, chunk_size) + + quote = fields_enclosed_by.encode(encoding) + + # Fast screen: if no physical line ends with an unbalanced number of quote characters, + # then no record spans a line break and the plain scan is already correct. This is the + # overwhelmingly common case - including files where EVERY field is quoted - and it costs + # one C-level count() per line instead of a Python step per quote character. + offsets = _scan_screened(path, terminator, tlen, quote, chunk_size) + if offsets is not None: + return offsets + + return _scan_enclosed( + path, + terminator, + tlen, + quote, + fields_terminated_by.encode(encoding), + chunk_size, + ) + +def _scan_plain(path, terminator, tlen, chunk_size): + offsets = array("L", [0]) + buffer = b"" + base = 0 with io.open(path, "rb") as f: while True: chunk = f.read(chunk_size) if not chunk: break - buffer += chunk consumed = 0 while True: found = buffer.find(terminator, consumed) if found == -1: break - consumed = found + terminator_length - line_offsets.append(base + consumed) + consumed = found + tlen + offsets.append(base + consumed) + base += consumed + buffer = buffer[consumed:] + _trim(offsets, path) + return offsets - # Whatever follows the last terminator stays in the buffer: it may be an - # unfinished line, or a terminator straddling the chunk boundary. + +def _scan_screened(path, terminator, tlen, quote, chunk_size): + """Return record offsets if every physical line has balanced quotes, else None.""" + offsets = array("L", [0]) + buffer = b"" + base = 0 + line_start = 0 # index within buffer of the current line's first byte + with io.open(path, "rb") as f: + while True: + chunk = f.read(chunk_size) + if not chunk: + break + buffer += chunk + consumed = 0 + while True: + found = buffer.find(terminator, consumed) + if found == -1: + break + if buffer.count(quote, line_start, found) % 2: + return None # a quoted field spans this line break; needs the exact scan + consumed = found + tlen + line_start = consumed + offsets.append(base + consumed) base += consumed buffer = buffer[consumed:] + line_start = 0 + _trim(offsets, path) + return offsets + + +def _scan_enclosed(path, terminator, tlen, quote, delimiter, chunk_size): + qlen = len(quote) + dlen = len(delimiter) + # A quote opens a field only at the very start of a record or immediately after a + # delimiter; anywhere else it is literal content, which is what csv.reader does under + # QUOTE_MINIMAL. Deciding that needs to look back at the bytes before the quote, and + # deciding whether a quote is escaped needs to look ahead, so the buffer keeps a margin + # of context on both sides of the cursor rather than being trimmed flush to it. + margin = max(dlen, tlen, 2 * qlen) + + offsets = array("L", [0]) + buffer = b"" + base = 0 # absolute offset of buffer[0] + pos = 0 # cursor within buffer + in_quotes = False + record_start = 0 # absolute offset of the current record + + with io.open(path, "rb") as f: + while True: + chunk = f.read(chunk_size) + eof = not chunk + if not eof: + buffer += chunk + + # Without more bytes coming, patterns can be resolved right up to the end. + limit = len(buffer) if eof else len(buffer) - margin + + while pos < len(buffer): + if in_quotes: + found = buffer.find(quote, pos) + if found == -1 or (not eof and found > limit): + pos = max(pos, limit) + break + if not eof and found + 2 * qlen > len(buffer): + break # cannot yet tell an escaped quote from a closing one + if buffer.startswith(quote, found + qlen): + pos = found + 2 * qlen + continue + in_quotes = False + pos = found + qlen + continue + + nq = buffer.find(quote, pos) + nt = buffer.find(terminator, pos) + if nt == -1 and nq == -1: + pos = max(pos, limit) + break + if not eof and min(x for x in (nq, nt) if x != -1) > limit: + pos = max(pos, limit) + break + if nt != -1 and (nq == -1 or nt < nq): + pos = nt + tlen + offsets.append(base + pos) + record_start = base + pos + continue + if base + nq == record_start: + opens = True + else: + opens = ( + (nq >= dlen and buffer.startswith(delimiter, nq - dlen)) + or (nq >= tlen and buffer.startswith(terminator, nq - tlen)) + ) + if opens: + in_quotes = True + pos = nq + qlen + + if eof: + break + + # Keep `margin` bytes behind the cursor so the look-back above still works after + # the buffer is trimmed. + drop = max(0, pos - margin) + buffer = buffer[drop:] + base += drop + pos -= drop + + _trim(offsets, path) + return offsets + +def _trim(offsets: array, path: str) -> None: # A file ending with the terminator has no extra empty line after it, so the offset # pointing at EOF is spurious. This also empties the index for a zero-byte file, which # must report no lines at all rather than one empty one. - if line_offsets and line_offsets[-1] == os.path.getsize(path): - line_offsets.pop() - - return line_offsets + if offsets and offsets[-1] == os.path.getsize(path): + offsets.pop() diff --git a/dwca/test/test_characterization.py b/dwca/test/test_characterization.py index c55b6e3..eb8de0e 100644 --- a/dwca/test/test_characterization.py +++ b/dwca/test/test_characterization.py @@ -545,3 +545,48 @@ def test_coreid_index_holds_every_position(self): index = dwca.core_file.coreid_index assert {"1": array("L", [0, 1]), "2": array("L", [2])} == index + + +class TestQuotedRecordsSpanningLines(unittest.TestCase): + """A quoted field may contain the line terminator. Iteration and random access must agree.""" + + QUOTED_META = {"fields_terminated_by": ",", "fields_enclosed_by": '"'} + + def _archive(self, payload, columns=3): + return build_archive( + temp_archive_dir(self), + rows=[], + columns=columns, + raw_payload=payload, + **self.QUOTED_META + ) + + def test_iteration_and_random_access_agree(self): + path = self._archive(b'0,b,a\n1,a,"x\ny\nz"\n2,"x\ny\nz",a\n') + + with DwCAReader(path) as dwca: + streamed = [row.raw_fields for row in dwca] + seeked = [ + dwca.core_file.get_row_by_position(i).raw_fields + for i in range(len(streamed)) + ] + + assert [["0", "b", "a"], ["1", "a", "x\ny\nz"], ["2", "x\ny\nz", "a"]] == streamed + assert streamed == seeked + + def test_extensions_are_not_truncated(self): + """coreid_index is built from the streaming pass but consumed through seeks.""" + directory = temp_archive_dir(self) + path = build_archive( + directory, + rows=[["1", "Lagopus"], ["2", "Struthio"]], + fields_terminated_by=",", + fields_enclosed_by='"', + extension=[["1", "grouse\nfoo"], ["2", "ostrich"]], + ) + + with DwCAReader(path) as dwca: + per_core = [[e.raw_fields for e in row.extensions] for row in dwca] + + assert [["1", "grouse\nfoo"]] == per_core[0] + assert [["2", "ostrich"]] == per_core[1] diff --git a/dwca/test/test_datafile.py b/dwca/test/test_datafile.py index 20be194..6f61aa5 100644 --- a/dwca/test/test_datafile.py +++ b/dwca/test/test_datafile.py @@ -284,3 +284,49 @@ def test_a_zero_byte_core_file_has_no_rows(self): with DwCAReader(path) as dwca: with pytest.raises(IndexError): dwca.core_file.get_row_by_position(0) + + def test_multibyte_utf8_and_an_embedded_newline_in_the_same_file(self): + """_read_record() must use a byte-accurate read. + + offsets are byte offsets, but a text stream's read(n) counts characters, so a + naive `self._file_stream.read(next_offset - start)` would desync as soon as the + file contains a character that takes more than one byte in UTF-8 - truncating or + over-reading the record. This archive puts multi-byte characters (which make + byte count and character count diverge) and a quoted embedded newline (which + makes a record span more than one physical line) in the same file, so both + failure modes would have to hold simultaneously to pass. + """ + from .archive_builder import build_archive, temp_archive_dir + + # ACCENTED_E is a 2-byte UTF-8 character, SNOWMAN a 3-byte one: both make byte + # count and character count diverge. Row 2's third field is quoted and contains + # an embedded newline, so that record spans two physical lines. + ACCENTED_E = "\xe9" + SNOWMAN = "\u2603" + payload = ( + "1,caf" + ACCENTED_E + "," + SNOWMAN + "\n" + '2,"multi\nline",' + SNOWMAN + ACCENTED_E + "\n" + "3," + ACCENTED_E * 3 + ",end\n" + ).encode("utf-8") + path = build_archive( + temp_archive_dir(self), + rows=[], + columns=3, + raw_payload=payload, + fields_terminated_by=",", + fields_enclosed_by='"', + ) + + with DwCAReader(path) as dwca: + streamed = [row.raw_fields for row in dwca] + seeked = [ + dwca.core_file.get_row_by_position(i).raw_fields + for i in range(len(streamed)) + ] + + assert [ + ["1", "caf" + ACCENTED_E, SNOWMAN], + ["2", "multi\nline", SNOWMAN + ACCENTED_E], + ["3", ACCENTED_E * 3, "end"], + ] == streamed + assert streamed == seeked From f7488946ef35810647f1d1bc506fc45b5bb2b569 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Nicolas=20No=C3=A9?= Date: Tue, 28 Jul 2026 16:03:50 +0200 Subject: [PATCH 18/33] Whole-phase review cleanups: unused import, stale docstring, benchmark doc pointers --- benchmarks/README.md | 15 ++++++++++++--- dwca/rows.py | 1 - dwca/test/test_characterization.py | 9 +++++---- 3 files changed, 17 insertions(+), 8 deletions(-) diff --git a/benchmarks/README.md b/benchmarks/README.md index ef626f2..d6d79e9 100644 --- a/benchmarks/README.md +++ b/benchmarks/README.md @@ -7,9 +7,9 @@ Not part of the test suite. Run manually before and after a change that claims a `generate_archive.py` writes a GBIF-shaped archive: 50 columns, tab separated, no field enclosure, no header line. 400000 rows is roughly 250MB. Because `fieldsEnclosedBy=""`, -only the unquoted parsing path (`csv.QUOTE_NONE`, see `csv_line_to_fields()` in -`dwca/rows.py`) is exercised by these benchmarks - the quoted-field path is not measured -here. +only the unquoted parsing path (the `line.rstrip(...).split(...)` branch of +`CSVDataFile._iter_field_lists()` in `dwca/files.py`) is exercised by these benchmarks - the +quoted-field path is not measured here. `PYTHONPATH=.` is required because the scripts are run directly (not via `python -m`), so the repository root is not otherwise on `sys.path` and `import dwca` fails. @@ -136,3 +136,12 @@ runs on each side: the same runs) because that is the first operation in the process that builds the line offset index; it remains well below the "before" side's peak, where the index was built eagerly on open. + +The absolute timings above are not comparable across sessions - only the ratio between two +runs measured back to back in the same sitting is. Confirmed later: the same two commits +(`fd829b6` and `c481240`) that recorded `iterate + read 14 terms` at 6.86s and 1.77s here +measured 9.34s and 2.50s on a later, busier session on the same machine - the absolute +numbers moved by roughly a third, but the ratio held (3.7x-4.0x measured back to back that +time, against 3.8x-4.0x recorded above). Do not read the absolute seconds as a target or a +regression signal in isolation; re-measure both sides back to back before drawing any +conclusion from them. diff --git a/dwca/rows.py b/dwca/rows.py index ff30bb0..19b78bb 100644 --- a/dwca/rows.py +++ b/dwca/rows.py @@ -1,7 +1,6 @@ """Objects that represents data rows coming from DarwinCore Archives.""" import csv -import sys from typing import Dict, List, Optional from dwca.descriptors import DataFileDescriptor diff --git a/dwca/test/test_characterization.py b/dwca/test/test_characterization.py index eb8de0e..15fe8f1 100644 --- a/dwca/test/test_characterization.py +++ b/dwca/test/test_characterization.py @@ -350,11 +350,12 @@ class TestExtensionFiles(unittest.TestCase): """extension= was never passed to build_archive by any test, so that whole branch of the builder was dead code, and every extension-bearing behavior below ran only on the three bundled sample archives, which all happen to share one configuration (utf-8, tab, no - enclosure, ignoreHeaderLines="1"). CSVDataFile.coreid_index is built through + enclosure, ignoreHeaderLines="1"). CSVDataFile.coreid_index used to be built through CSVDataFile.__iter__ (the readlines(byte-hint) header bug, see TestHeaderLines above and - B1), and get_all_rows_by_coreid() feeds those positions into get_row_by_position(), which - re-applies lines_to_ignore - exactly the seam the B1 bug lives in, and it was unpinned for - extensions until now. + B1), and get_all_rows_by_coreid() fed those positions into get_row_by_position(), which + re-applied lines_to_ignore - exactly the seam the B1 bug lived in, and it was unpinned for + extensions until this test was added. coreid_index is now built through iter_rows() + instead, so that seam no longer exists here. """ def _archive(self, **kwargs): From 942cb5d12c4042bd6f87738d864dba34f58b6ffe Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Nicolas=20No=C3=A9?= Date: Tue, 28 Jul 2026 16:14:54 +0200 Subject: [PATCH 19/33] Add a term getter that maps a data row straight to a tuple --- dwca/descriptors.py | 52 +++++++++++++++++++ dwca/test/test_descriptors.py | 94 +++++++++++++++++++++++++++++++++++ 2 files changed, 146 insertions(+) diff --git a/dwca/descriptors.py b/dwca/descriptors.py index d1e4f44..68a053a 100644 --- a/dwca/descriptors.py +++ b/dwca/descriptors.py @@ -417,6 +417,58 @@ def build_data(self, raw_fields): return data + def term_getter(self, terms): + """Return a callable mapping a split data row to a tuple of values for `terms`. + + :raises ValueError: if any requested term is absent from the data file. + """ + by_term = {f["term"]: f for f in self._fields} + + missing = [term for term in terms if term not in by_term] + if missing: + raise ValueError( + "These terms are not in this data file: {t}".format( + t=", ".join(sorted(missing)) + ) + ) + + indexes = tuple(by_term[term]["index"] for term in terms) + defaults = tuple(by_term[term]["default"] for term in terms) + + if indexes and all(i is not None for i in indexes) and not any(defaults): + # Fast path: every term maps to a column and none has a default, so the whole + # tuple comes out of a single C-level call. + getter = itemgetter(*indexes) + required = max(indexes) + 1 + + if len(indexes) == 1: + # itemgetter with a single argument returns a scalar, not a tuple. + def get_one(raw_fields): + if len(raw_fields) < required: + self._raise_missing_column(raw_fields) + return (getter(raw_fields),) + + return get_one + + def get_many(raw_fields): + if len(raw_fields) < required: + self._raise_missing_column(raw_fields) + return getter(raw_fields) + + return get_many + + required = max([i for i in indexes if i is not None] or [-1]) + 1 + + def get_general(raw_fields): + if len(raw_fields) < required: + self._raise_missing_column(raw_fields) + return tuple( + (raw_fields[index] if index is not None else None) or default or "" + for index, default in zip(indexes, defaults) + ) + + return get_general + def _raise_missing_column(self, raw_fields): # Slow path: report the same index the old per-field loop would have reported. for field in self._fields: diff --git a/dwca/test/test_descriptors.py b/dwca/test/test_descriptors.py index 430d77a..68fdfbe 100644 --- a/dwca/test/test_descriptors.py +++ b/dwca/test/test_descriptors.py @@ -796,3 +796,97 @@ def test_the_plan_is_cached(self): descriptor = self._descriptor('') assert descriptor.field_plan is descriptor.field_plan + + +class TestTermGetter(unittest.TestCase): + def _plan(self, fields_xml): + section = """ + + occurrence.txt + + {fields} + + """.format( + fields=fields_xml + ) + descriptor = DataFileDescriptor.make_from_metafile_section( + ET.fromstring(section) + ) + return descriptor.field_plan + + def test_returns_values_in_the_requested_order(self): + plan = self._plan( + '' + '' + '' + ) + getter = plan.term_getter(["http://x/c", "http://x/a"]) + + assert ("third", "first") == getter(["first", "second", "third"]) + + def test_a_single_term_still_yields_a_tuple(self): + plan = self._plan('') + getter = plan.term_getter(["http://x/b"]) + + assert ("second",) == getter(["first", "second"]) + + def test_no_terms(self): + plan = self._plan('') + getter = plan.term_getter([]) + + assert () == getter(["first"]) + + def test_a_term_may_be_requested_twice(self): + plan = self._plan('') + getter = plan.term_getter(["http://x/a", "http://x/a"]) + + assert ("first", "first") == getter(["first"]) + + def test_default_only_term_yields_the_constant(self): + plan = self._plan( + '' + '' + ) + getter = plan.term_getter(["http://x/country", "http://x/a"]) + + assert ("Belgium", "first") == getter(["first"]) + + def test_default_fills_an_empty_cell(self): + plan = self._plan( + '' + '' + ) + getter = plan.term_getter(["http://x/b"]) + + assert ("value",) == getter(["first", "value"]) + assert ("fallback",) == getter(["first", ""]) + + def test_missing_value_without_default_becomes_empty_string(self): + plan = self._plan( + '' + '' + ) + getter = plan.term_getter(["http://x/b"]) + + assert ("",) == getter(["first", ""]) + + def test_unknown_terms_are_named_in_the_error(self): + plan = self._plan('') + + with pytest.raises(ValueError) as excinfo: + plan.term_getter(["http://x/missing", "http://x/a", "http://x/gone"]) + + message = str(excinfo.value) + assert "http://x/missing" in message + assert "http://x/gone" in message + assert "http://x/a" not in message + + def test_short_row_raises_invalid_archive(self): + plan = self._plan( + '' + '' + ) + getter = plan.term_getter(["http://x/e"]) + + with pytest.raises(InvalidArchive): + getter(["first", "second"]) From 6bfa3666f16c64db35b551795908f58c2e8736aa Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Nicolas=20No=C3=A9?= Date: Tue, 28 Jul 2026 16:22:21 +0200 Subject: [PATCH 20/33] Add iter_terms() for reading a subset of columns --- CHANGES.txt | 3 ++ benchmarks/README.md | 42 ++++++++++++++++++++++++++++ dwca/files.py | 26 ++++++++++++++++++ dwca/read.py | 17 ++++++++++++ dwca/test/test_datafile.py | 53 ++++++++++++++++++++++++++++++++++++ dwca/test/test_dwcareader.py | 13 +++++++++ 6 files changed, 154 insertions(+) diff --git a/CHANGES.txt b/CHANGES.txt index 6fe63c8..b3b94cc 100644 --- a/CHANGES.txt +++ b/CHANGES.txt @@ -35,6 +35,9 @@ v0.17.0 (unreleased) - Fixed: in archives using fieldsEnclosedBy, a field containing the line terminator made random access and iteration disagree, so CoreRow.extensions could return silently truncated rows. The line offset index now indexes CSV records rather than physical lines. +- New: DwCAReader.iter_terms() and CSVDataFile.iter_terms() yield a tuple of values per row + for a chosen list of terms, skipping both the Row object and its data dict. Roughly twice + as fast as iterating over rows when only a few terms are needed. v0.16.4 (2024-10-18) -------------------- diff --git a/benchmarks/README.md b/benchmarks/README.md index d6d79e9..5a7cac3 100644 --- a/benchmarks/README.md +++ b/benchmarks/README.md @@ -145,3 +145,45 @@ numbers moved by roughly a third, but the ratio held (3.7x-4.0x measured back to time, against 3.8x-4.0x recorded above). Do not read the absolute seconds as a target or a regression signal in isolation; re-measure both sides back to back before drawing any conclusion from them. + +## iter_terms + +`DwCAReader.iter_terms()` / `CSVDataFile.iter_terms()` skip building both the `Row` object +and its term-to-value dict, yielding a plain tuple of the requested terms per row instead. +Measured against the row API on the same 400000-row, 50-column archive generated above +(`/tmp/dwca-bench`), reading 14 terms per row in both cases, both paths measured back to +back in one run (rerun twice to check consistency): + + # Row path + with DwCAReader(ARCHIVE, skip_metadata=True) as dwca: + for row in dwca: + data = row.data + for term in TERMS: + data.get(term) + + # iter_terms path + with DwCAReader(ARCHIVE, skip_metadata=True) as dwca: + for values in dwca.iter_terms(TERMS): + pass + +Run 1: + + rows path 2.86s + iter_terms 0.98s + ratio (rows/iter_terms): 2.93x + +Run 2 (immediately after, same process type): + + rows path 2.13s + iter_terms 0.75s + ratio (rows/iter_terms): 2.85x + +Run 3: + + rows path 2.12s + iter_terms 0.72s + ratio (rows/iter_terms): 2.95x + +Consistent at roughly 2.85x-2.95x across three back-to-back runs, comfortably above the +"roughly half the rows path" (2x) expectation. As with the numbers above, absolute seconds +vary by session; only the ratio, measured back to back, is meaningful. diff --git a/dwca/files.py b/dwca/files.py index b7742d0..b9ca441 100644 --- a/dwca/files.py +++ b/dwca/files.py @@ -60,6 +60,32 @@ def __init__( self._coreid_index = None # type: Optional[Dict[str, List[int]]] + def iter_terms(self, terms: List[str]) -> Iterator[tuple]: + """Yield one tuple of values per data row, holding `terms` in the order given. + + This is a faster alternative to iterating over rows for consumers that only need a + few of the file's columns: neither a :class:`dwca.rows.Row` object nor its term to + value dict is built. A single term still yields a one-element tuple. + + Usage:: + + for locality, latitude in data_file.iter_terms([qn('locality'), + qn('decimalLatitude')]): + pass + + :param terms: a list of full term identifiers. + :raises ValueError: if any of `terms` is not present in this data file. + """ + # The getter is resolved eagerly so an unknown term is reported by this call rather + # than on first iteration. + getter = self.file_descriptor.field_plan.term_getter(terms) + + return self._iter_terms(getter) + + def _iter_terms(self, getter) -> Iterator[tuple]: + for fields in self._iter_field_lists(): + yield getter(fields) + def __str__(self) -> str: return self.file_descriptor.file_location diff --git a/dwca/read.py b/dwca/read.py index 84ea2ac..95d64b4 100644 --- a/dwca/read.py +++ b/dwca/read.py @@ -257,6 +257,23 @@ def pd_read(self, relative_path, **kwargs): return df_or_textreader + def iter_terms(self, terms: List[str]) -> Iterator[tuple]: + """Yield one tuple of values per core row, holding `terms` in the order given. + + A faster alternative to iterating over the reader when only a few terms are needed. + See :meth:`dwca.files.CSVDataFile.iter_terms`. + + Usage:: + + for identifier, latitude, longitude in dwca.iter_terms( + [qn('occurrenceID'), qn('decimalLatitude'), qn('decimalLongitude')]): + pass + + :param terms: a list of full term identifiers. + :raises ValueError: if any of `terms` is not present in the core data file. + """ + return self.core_file.iter_terms(terms) + def orphaned_extension_rows(self) -> Dict[str, Dict[str, List[int]]]: """Return a dict of the orphaned extension rows. diff --git a/dwca/test/test_datafile.py b/dwca/test/test_datafile.py index 6f61aa5..18b95e6 100644 --- a/dwca/test/test_datafile.py +++ b/dwca/test/test_datafile.py @@ -330,3 +330,56 @@ def test_multibyte_utf8_and_an_embedded_newline_in_the_same_file(self): ["3", ACCENTED_E * 3, "end"], ] == streamed assert streamed == seeked + + +class TestIterTerms(unittest.TestCase): + def test_matches_the_row_api(self): + with DwCAReader(sample_data_path("dwca-2extensions.zip")) as dwca: + terms = sorted(dwca.core_file.file_descriptor.terms) + + via_rows = [tuple(row.data[t] for t in terms) for row in dwca] + via_terms = list(dwca.core_file.iter_terms(terms)) + + assert via_rows == via_terms + + def test_works_on_an_extension_file(self): + with DwCAReader(sample_data_path("dwca-2extensions.zip")) as dwca: + extension = dwca.extension_files[0] + terms = sorted(extension.file_descriptor.terms) + + via_rows = [tuple(r.data[t] for t in terms) for r in extension.iter_rows()] + via_terms = list(extension.iter_terms(terms)) + + assert via_rows == via_terms + + def test_subset_of_columns(self): + with DwCAReader(sample_data_path("dwca-simple-test-archive.zip")) as dwca: + values = list( + dwca.core_file.iter_terms(["http://rs.tdwg.org/dwc/terms/locality"]) + ) + + assert [("Borneo",), ("Mumbai",)] == values + + def test_unknown_term_raises_before_reading_anything(self): + with DwCAReader(sample_data_path("dwca-simple-test-archive.zip")) as dwca: + with pytest.raises(ValueError): + dwca.core_file.iter_terms(["http://rs.tdwg.org/dwc/terms/nope"]) + + def test_default_only_term(self): + with DwCAReader(sample_data_path("dwca-test-default.zip")) as dwca: + values = list( + dwca.core_file.iter_terms(["http://rs.tdwg.org/dwc/terms/country"]) + ) + + assert [("Belgium",), ("Belgium",)] == values + + def test_can_be_nested(self): + with DwCAReader(sample_data_path("dwca-ids.zip")) as dwca: + term = ["http://rs.tdwg.org/dwc/terms/family"] + pairs = [ + (a, b) + for a in dwca.core_file.iter_terms(term) + for b in dwca.core_file.iter_terms(term) + ] + + assert 16 == len(pairs) diff --git a/dwca/test/test_dwcareader.py b/dwca/test/test_dwcareader.py index 4b9d42d..90085cd 100644 --- a/dwca/test/test_dwcareader.py +++ b/dwca/test/test_dwcareader.py @@ -1100,6 +1100,19 @@ def test_next_still_works(self): finally: dwca.close() + def test_iter_terms_on_the_reader(self): + with DwCAReader(sample_data_path("dwca-simple-test-archive.zip")) as dwca: + values = list( + dwca.iter_terms( + [ + "http://rs.tdwg.org/dwc/terms/locality", + "http://rs.tdwg.org/dwc/terms/family", + ] + ) + ) + + assert [("Borneo", "Tetraodontidae"), ("Mumbai", "Osphronemidae")] == values + if __name__ == "__main__": unittest.main() From 9e511b2fd7399c6266efec8457a25ffae8663951 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Nicolas=20No=C3=A9?= Date: Tue, 28 Jul 2026 16:33:21 +0200 Subject: [PATCH 21/33] Fix documentation calling a method removed in v0.15.0 --- CHANGES.txt | 2 ++ doc/gbif_results.rst | 4 ++-- doc/tutorial.rst | 2 +- dwca/read.py | 4 ++-- 4 files changed, 7 insertions(+), 5 deletions(-) diff --git a/CHANGES.txt b/CHANGES.txt index b3b94cc..552dd22 100644 --- a/CHANGES.txt +++ b/CHANGES.txt @@ -38,6 +38,8 @@ v0.17.0 (unreleased) - New: DwCAReader.iter_terms() and CSVDataFile.iter_terms() yield a tuple of values per row for a chosen list of terms, skipping both the Row object and its data dict. Roughly twice as fast as iterating over rows when only a few terms are needed. +- Documentation: the tutorial and the GBIF results page still called get_row_by_index(), + removed in v0.15.0. They now use get_corerow_by_position(). v0.16.4 (2024-10-18) -------------------- diff --git a/doc/gbif_results.rst b/doc/gbif_results.rst index b5e115d..2c96379 100644 --- a/doc/gbif_results.rst +++ b/doc/gbif_results.rst @@ -29,7 +29,7 @@ You can access this source metadata like this: # 'dataset2_UUID': , ...} # 2. From a CoreRow instance, we can get back to the metadata of its source dataset: - first_row = results.get_row_by_index(0) + first_row = results.get_corerow_by_position(0) print(first_row.source_metadata) # => @@ -45,7 +45,7 @@ Because there's a standard core-extension relationship (star schema) between tho from dwca.read import DwCAReader with DwCAReader('gbif-results.zip') as results: - first_row = results.get_row_by_index(0) + first_row = results.get_corerow_by_position(0) first_row.extensions diff --git a/doc/tutorial.rst b/doc/tutorial.rst index 2be852e..874b26d 100644 --- a/doc/tutorial.rst +++ b/doc/tutorial.rst @@ -80,7 +80,7 @@ Basic use, access to metadata and data from the Core file # guarantee unicity (nor even that there will be an id). The index (position) of the row (starting at 0) is # generally preferable. - occurrence_on_second_line = dwca.get_row_by_index(1) + occurrence_on_second_line = dwca.get_corerow_by_position(1) # We can retreive the (absolute) of embedded files # NOTE: this path point to a temporary directory that will be removed at the end of the DwCAReader object life diff --git a/dwca/read.py b/dwca/read.py index 95d64b4..f716272 100644 --- a/dwca/read.py +++ b/dwca/read.py @@ -359,8 +359,8 @@ def get_corerow_by_position(self, position: int) -> CoreRow: .. note:: - - If index is bigger than the length of the archive, None is returned - - The position is often an appropriate way to unambiguously identify a core row in a DwCA. + The position is often an appropriate way to unambiguously identify a core row in + a DwCA. """ for i, row in enumerate(self): From 3c915cddf23e108d02bd993e56743ffda3311c6a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Nicolas=20No=C3=A9?= Date: Tue, 28 Jul 2026 16:33:51 +0200 Subject: [PATCH 22/33] Document iter_terms and add star_record to the API reference --- doc/api.rst | 6 ++++++ doc/tutorial.rst | 29 +++++++++++++++++++++++++++++ 2 files changed, 35 insertions(+) diff --git a/doc/api.rst b/doc/api.rst index 02b98cb..2501151 100644 --- a/doc/api.rst +++ b/doc/api.rst @@ -28,6 +28,12 @@ File objects .. automodule:: dwca.files :members: +Star record objects +------------------- + +.. automodule:: dwca.star_record + :members: + Helpers ------- diff --git a/doc/tutorial.rst b/doc/tutorial.rst index 874b26d..81a1af5 100644 --- a/doc/tutorial.rst +++ b/doc/tutorial.rst @@ -88,6 +88,35 @@ Basic use, access to metadata and data from the Core file path = dwca.absolute_temporary_path('occurrence.txt') +Reading only a few terms +~~~~~~~~~~~~~~~~~~~~~~~~ + +If you only need a handful of terms and the archive is large, :meth:`~dwca.read.DwCAReader.iter_terms` +is roughly twice as fast as iterating over rows: it yields a plain tuple per row and never +builds a :class:`~dwca.rows.CoreRow` object nor its ``data`` dictionary. + +.. code:: python + + from dwca.read import DwCAReader + from dwca.darwincore.utils import qualname as qn + + with DwCAReader('my-archive.zip') as dwca: + for locality, family, scientific_name in dwca.iter_terms( + [qn('locality'), qn('family'), qn('scientificName')]): + print(locality, family, scientific_name) + +Values are returned in the order the terms were requested, and they are normalised exactly as +``row.data`` would be: an empty cell falls back to the term's default value from the Metafile, +or to an empty string. Requesting a term the data file does not contain raises ``ValueError``. + +A single term still yields a one-element tuple: + +.. code:: python + + for (locality,) in dwca.iter_terms([qn('locality')]): + print(locality) + + Access to Darwin Core Archives with extensions (star schema) ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ From ac33abb7bfc17923b9a989067e393f3b9ce2f188 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Nicolas=20No=C3=A9?= Date: Tue, 28 Jul 2026 16:40:38 +0200 Subject: [PATCH 23/33] Drop typing_extensions and require Python 3.8 or later --- .github/workflows/run-unit-tests.yml | 8 +------- CHANGES.txt | 3 +++ dwca/star_record.py | 3 +-- requirements-dev.txt | 3 +-- setup.py | 4 +++- 5 files changed, 9 insertions(+), 12 deletions(-) diff --git a/.github/workflows/run-unit-tests.yml b/.github/workflows/run-unit-tests.yml index c0e415c..999d189 100644 --- a/.github/workflows/run-unit-tests.yml +++ b/.github/workflows/run-unit-tests.yml @@ -6,13 +6,7 @@ jobs: strategy: matrix: os: [ubuntu-latest, macos-latest, windows-latest] - python-version: ['3.7', '3.8', '3.9', '3.10', '3.11', '3.12', '3.13', 'pypy-3.9' ] - exclude: # Python < v3.8 does not support Apple Silicon ARM64. - - python-version: "3.7" - os: macos-latest - include: # So run those legacy versions on Intel CPUs. - - python-version: "3.7" - os: macos-13 + python-version: ['3.8', '3.9', '3.10', '3.11', '3.12', '3.13', 'pypy-3.9'] steps: - uses: actions/checkout@v2 - uses: actions/setup-python@v2 diff --git a/CHANGES.txt b/CHANGES.txt index 552dd22..8205b7b 100644 --- a/CHANGES.txt +++ b/CHANGES.txt @@ -1,6 +1,9 @@ v0.17.0 (unreleased) -------------------- +- Removed the undeclared typing_extensions dependency (dwca.star_record now uses + typing.Literal). Python 3.7, which reached end of life in June 2023, is no longer + supported; the minimum is now 3.8. - Fixed: DataFileDescriptor.headers dropped the column at index 0 for archives without a metafile, which also made pd_read() promote that column to the DataFrame index. - Fixed: hash() on a CoreRow or an ExtensionRow raised TypeError. Rows have been documented diff --git a/dwca/star_record.py b/dwca/star_record.py index 1e71757..a3ff49d 100644 --- a/dwca/star_record.py +++ b/dwca/star_record.py @@ -1,6 +1,5 @@ from dwca.files import CSVDataFile -from typing import List -from typing_extensions import Literal +from typing import List, Literal import itertools diff --git a/requirements-dev.txt b/requirements-dev.txt index d07437e..1b724d6 100644 --- a/requirements-dev.txt +++ b/requirements-dev.txt @@ -1,4 +1,3 @@ pandas mock==2.0.0 -pytest -typing-extensions \ No newline at end of file +pytest \ No newline at end of file diff --git a/setup.py b/setup.py index 3b1cb32..c44a4b5 100644 --- a/setup.py +++ b/setup.py @@ -12,13 +12,15 @@ license="BSD licence, see LICENCE.txt", description="A simple Python package to read Darwin Core Archive (DwC-A) files.", long_description=open("README.rst").read(), + python_requires=">=3.8", classifiers=[ "Programming Language :: Python :: 3", - "Programming Language :: Python :: 3.7", "Programming Language :: Python :: 3.8", "Programming Language :: Python :: 3.9", "Programming Language :: Python :: 3.10", "Programming Language :: Python :: 3.11", + "Programming Language :: Python :: 3.12", + "Programming Language :: Python :: 3.13", "Programming Language :: Python :: Implementation :: PyPy", ], ) From 81fca38f973f0f6ceabac1dc091eb5846d174564 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Nicolas=20No=C3=A9?= Date: Tue, 28 Jul 2026 16:41:26 +0200 Subject: [PATCH 24/33] Raise when iterating a closed data file --- CHANGES.txt | 2 ++ dwca/files.py | 3 +++ dwca/test/test_datafile.py | 21 +++++++++++++++++++++ 3 files changed, 26 insertions(+) diff --git a/CHANGES.txt b/CHANGES.txt index 8205b7b..995627e 100644 --- a/CHANGES.txt +++ b/CHANGES.txt @@ -4,6 +4,8 @@ v0.17.0 (unreleased) - Removed the undeclared typing_extensions dependency (dwca.star_record now uses typing.Literal). Python 3.7, which reached end of life in June 2023, is no longer supported; the minimum is now 3.8. +- Fixed: iterating a CSVDataFile or a DwCAReader after close() silently returned rows instead + of raising, for archives read directly from a directory. - Fixed: DataFileDescriptor.headers dropped the column at index 0 for archives without a metafile, which also made pd_read() promote that column to the DataFrame index. - Fixed: hash() on a CoreRow or an ExtensionRow raised TypeError. Rows have been documented diff --git a/dwca/files.py b/dwca/files.py index b9ca441..817a022 100644 --- a/dwca/files.py +++ b/dwca/files.py @@ -116,6 +116,9 @@ def _iter_field_lists(self) -> Iterator[List[str]]: Header lines are skipped. This is a single forward pass over a dedicated stream, so it is safe to run several of these concurrently and alongside random access. """ + if self._file_stream.closed: + raise ValueError("The data file has been closed.") + descriptor = self.file_descriptor quoted = descriptor.fields_enclosed_by != "" diff --git a/dwca/test/test_datafile.py b/dwca/test/test_datafile.py index 18b95e6..7f0c7b0 100644 --- a/dwca/test/test_datafile.py +++ b/dwca/test/test_datafile.py @@ -383,3 +383,24 @@ def test_can_be_nested(self): ] assert 16 == len(pairs) + + +class TestClosedFileGuarantee(unittest.TestCase): + def test_iteration_after_close_raises(self): + """close() documents that content is not accessible in any way afterwards.""" + with DwCAReader(sample_data_path("dwca-simple-dir")) as dwca: + data_file = dwca.core_file + + assert list(data_file.iter_rows()) # works while open + data_file.close() + + with pytest.raises(ValueError): + list(data_file.iter_rows()) + + def test_reader_iteration_after_close_raises(self): + dwca = DwCAReader(sample_data_path("dwca-simple-dir")) + assert list(dwca) + dwca.close() + + with pytest.raises(ValueError): + list(dwca) From 82711168f170b29fc7a6e15c590883db4972ba25 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Nicolas=20No=C3=A9?= Date: Tue, 28 Jul 2026 16:52:07 +0200 Subject: [PATCH 25/33] Organize and correct the unreleased changelog section 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. --- CHANGES.txt | 32 +++++++++++++++++--------------- 1 file changed, 17 insertions(+), 15 deletions(-) diff --git a/CHANGES.txt b/CHANGES.txt index 995627e..fb54e39 100644 --- a/CHANGES.txt +++ b/CHANGES.txt @@ -1,11 +1,18 @@ v0.17.0 (unreleased) -------------------- -- Removed the undeclared typing_extensions dependency (dwca.star_record now uses - typing.Literal). Python 3.7, which reached end of life in June 2023, is no longer - supported; the minimum is now 3.8. +- New: DwCAReader.iter_terms() and CSVDataFile.iter_terms() yield a tuple of values per row + for a chosen list of terms, skipping both the Row object and its data dict. Measured + roughly 2.9x faster than iterating over rows and reading the same terms, and roughly 10x + faster than the pre-rewrite implementation, on a 400000-row archive reading 14 of 50 + columns, on one machine; see benchmarks/README.md for the full measurement. +- Performance: iterating over an archive is a single streaming pass instead of one seek and + one throwaway csv.reader per row. Measured roughly 3.8-4.0x faster on a 400000-row GBIF + download on one machine; see benchmarks/README.md for the full before/after measurement. +- Performance: the line offset index used for random access is now built on first use, so + opening an archive no longer scans every data file. - Fixed: iterating a CSVDataFile or a DwCAReader after close() silently returned rows instead - of raising, for archives read directly from a directory. + of raising. - Fixed: DataFileDescriptor.headers dropped the column at index 0 for archives without a metafile, which also made pd_read() promote that column to the DataFrame index. - Fixed: hash() on a CoreRow or an ExtensionRow raised TypeError. Rows have been documented @@ -19,11 +26,6 @@ v0.17.0 (unreleased) hashes) by value: the data file layout it describes. - Fixed: comparing a CoreRow to an ExtensionRow (or either to a non-row object) raised AttributeError instead of returning False. -- Performance: iterating over an archive is a single streaming pass instead of one seek and - one throwaway csv.reader per row. Measured roughly 3.8-4.0x faster on a 400000-row GBIF - download on one machine; see benchmarks/README.md for the full before/after measurement. -- Performance: the line offset index used for random access is now built on first use, so - opening an archive no longer scans every data file. - Fixed: ignoreHeaderLines values above 1 only skipped a single line when iterating a CSVDataFile directly, which let header lines leak into coreid_index and orphaned_extension_rows. @@ -34,15 +36,15 @@ v0.17.0 (unreleased) - Fixed: DwCAReader was its own iterator, so nesting two loops over the same reader silently ran the inner one once, and calling get_corerow_by_id() or get_corerow_by_position() from inside a loop never terminated. Each iteration now gets its own iterator. -- Changed: DwCAReader.next() now keeps an implicit iterator independent of any for loop over - the same reader, and starts a new pass after being exhausted. Iterate over the reader - instead; next() remains only for backwards compatibility. - Fixed: in archives using fieldsEnclosedBy, a field containing the line terminator made random access and iteration disagree, so CoreRow.extensions could return silently truncated rows. The line offset index now indexes CSV records rather than physical lines. -- New: DwCAReader.iter_terms() and CSVDataFile.iter_terms() yield a tuple of values per row - for a chosen list of terms, skipping both the Row object and its data dict. Roughly twice - as fast as iterating over rows when only a few terms are needed. +- Changed: removed the undeclared typing_extensions dependency (dwca.star_record now uses + typing.Literal). Python 3.7, which reached end of life in June 2023, is no longer + supported; the minimum is now 3.8. +- Changed: DwCAReader.next() now keeps an implicit iterator independent of any for loop over + the same reader, and starts a new pass after being exhausted. Iterate over the reader + instead; next() remains only for backwards compatibility. - Documentation: the tutorial and the GBIF results page still called get_row_by_index(), removed in v0.15.0. They now use get_corerow_by_position(). From 1f4f366efc1b1ba03ad25b51341d895f3d9e22c8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Nicolas=20No=C3=A9?= Date: Tue, 28 Jul 2026 17:10:08 +0200 Subject: [PATCH 26/33] Fix term_getter generator exhaustion, correct changelog, and minor cleanups - 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. --- CHANGES.txt | 7 +++---- doc/index.rst | 2 +- dwca/descriptors.py | 5 +++++ dwca/files.py | 4 ++-- dwca/read.py | 2 +- dwca/test/test_descriptors.py | 18 ++++++++++++++++++ requirements-dev.txt | 2 +- 7 files changed, 31 insertions(+), 9 deletions(-) diff --git a/CHANGES.txt b/CHANGES.txt index fb54e39..cc180bb 100644 --- a/CHANGES.txt +++ b/CHANGES.txt @@ -11,8 +11,6 @@ v0.17.0 (unreleased) download on one machine; see benchmarks/README.md for the full before/after measurement. - Performance: the line offset index used for random access is now built on first use, so opening an archive no longer scans every data file. -- Fixed: iterating a CSVDataFile or a DwCAReader after close() silently returned rows instead - of raising. - Fixed: DataFileDescriptor.headers dropped the column at index 0 for archives without a metafile, which also made pd_read() promote that column to the DataFrame index. - Fixed: hash() on a CoreRow or an ExtensionRow raised TypeError. Rows have been documented @@ -37,8 +35,9 @@ v0.17.0 (unreleased) ran the inner one once, and calling get_corerow_by_id() or get_corerow_by_position() from inside a loop never terminated. Each iteration now gets its own iterator. - Fixed: in archives using fieldsEnclosedBy, a field containing the line terminator made - random access and iteration disagree, so CoreRow.extensions could return silently truncated - rows. The line offset index now indexes CSV records rather than physical lines. + iteration raise InvalidArchive and random access (e.g. CoreRow.extensions) silently return + a truncated row. Such archives are now read correctly. The line offset index now indexes + CSV records rather than physical lines. - Changed: removed the undeclared typing_extensions dependency (dwca.star_record now uses typing.Literal). Python 3.7, which reached end of life in June 2023, is no longer supported; the minimum is now 3.8. diff --git a/doc/index.rst b/doc/index.rst index 2a2e3d7..2fb3f48 100644 --- a/doc/index.rst +++ b/doc/index.rst @@ -13,7 +13,7 @@ Archives can be enclosed in either a directory or a zip/tgz archive. It supports most common features from the Darwin Core Archive standard, including extensions and `Simple Darwin Core`_ expressed as text (aka Archives consisting of a single CSV data file, possibly with Metadata but without Metafile). -It officially supports Python 3.5+ and has been reported to work on Jython by at least one user. It works on Linux, Mac OS and since v0.10.2 also on Windows. +It officially supports Python 3.8+ and has been reported to work on Jython by at least one user. It works on Linux, Mac OS and since v0.10.2 also on Windows. Use version 0.13.2 if you need Python 2.7 support. Status diff --git a/dwca/descriptors.py b/dwca/descriptors.py index 68a053a..a6ec701 100644 --- a/dwca/descriptors.py +++ b/dwca/descriptors.py @@ -422,6 +422,11 @@ def term_getter(self, terms): :raises ValueError: if any requested term is absent from the data file. """ + # `terms` is iterated three times below (missing, indexes, defaults). A generator or + # other one-shot iterable would be exhausted after the first pass, silently turning + # every later pass empty rather than raising - so normalise to a list once up front. + terms = list(terms) + by_term = {f["term"]: f for f in self._fields} missing = [term for term in terms if term not in by_term] diff --git a/dwca/files.py b/dwca/files.py index 817a022..7d5f3e7 100644 --- a/dwca/files.py +++ b/dwca/files.py @@ -5,7 +5,7 @@ import os from array import array from itertools import islice -from typing import Iterator, List, Union, IO, Dict, Optional +from typing import Iterator, List, Tuple, Union, IO, Dict, Optional from dwca.descriptors import DataFileDescriptor from dwca.rows import CoreRow, ExtensionRow, Row @@ -60,7 +60,7 @@ def __init__( self._coreid_index = None # type: Optional[Dict[str, List[int]]] - def iter_terms(self, terms: List[str]) -> Iterator[tuple]: + def iter_terms(self, terms: List[str]) -> Iterator[Tuple[str, ...]]: """Yield one tuple of values per data row, holding `terms` in the order given. This is a faster alternative to iterating over rows for consumers that only need a diff --git a/dwca/read.py b/dwca/read.py index f716272..c94d65d 100644 --- a/dwca/read.py +++ b/dwca/read.py @@ -257,7 +257,7 @@ def pd_read(self, relative_path, **kwargs): return df_or_textreader - def iter_terms(self, terms: List[str]) -> Iterator[tuple]: + def iter_terms(self, terms: List[str]) -> Iterator[Tuple[str, ...]]: """Yield one tuple of values per core row, holding `terms` in the order given. A faster alternative to iterating over the reader when only a few terms are needed. diff --git a/dwca/test/test_descriptors.py b/dwca/test/test_descriptors.py index 68fdfbe..1470edb 100644 --- a/dwca/test/test_descriptors.py +++ b/dwca/test/test_descriptors.py @@ -890,3 +890,21 @@ def test_short_row_raises_invalid_archive(self): with pytest.raises(InvalidArchive): getter(["first", "second"]) + + def test_terms_may_be_a_one_shot_iterable(self): + # term_getter() used to iterate `terms` three times internally (missing, indexes, + # defaults). A generator is exhausted after the first pass, which silently made + # every later pass - and therefore every returned tuple - empty. + plan = self._plan( + '' + '' + ) + expected = ("second", "first") + + terms_list = ["http://x/b", "http://x/a"] + assert expected == plan.term_getter(t for t in terms_list)(["first", "second"]) + + # A tuple and a set of a single term must still work (the normalisation must not + # break non-list iterables that already worked before). + assert expected == plan.term_getter(tuple(terms_list))(["first", "second"]) + assert ("second",) == plan.term_getter({"http://x/b"})(["first", "second"]) diff --git a/requirements-dev.txt b/requirements-dev.txt index 1b724d6..ff7f233 100644 --- a/requirements-dev.txt +++ b/requirements-dev.txt @@ -1,3 +1,3 @@ pandas mock==2.0.0 -pytest \ No newline at end of file +pytest From aa8c7bfbfe88e6e6e5ee4ec0796a985fc4ff9ceb Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Nicolas=20No=C3=A9?= Date: Tue, 28 Jul 2026 17:18:56 +0200 Subject: [PATCH 27/33] Describe the quoted-record fix as users of 0.16.4 experienced it 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. --- CHANGES.txt | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/CHANGES.txt b/CHANGES.txt index cc180bb..67ddd86 100644 --- a/CHANGES.txt +++ b/CHANGES.txt @@ -34,10 +34,10 @@ v0.17.0 (unreleased) - Fixed: DwCAReader was its own iterator, so nesting two loops over the same reader silently ran the inner one once, and calling get_corerow_by_id() or get_corerow_by_position() from inside a loop never terminated. Each iteration now gets its own iterator. -- Fixed: in archives using fieldsEnclosedBy, a field containing the line terminator made - iteration raise InvalidArchive and random access (e.g. CoreRow.extensions) silently return - a truncated row. Such archives are now read correctly. The line offset index now indexes - CSV records rather than physical lines. +- Fixed: in archives using fieldsEnclosedBy, a field containing the line terminator was read + as truncated at that terminator, by iteration and by random access alike, and the rows after + it were misaligned (often raising InvalidArchive on an unrelated row). Such archives are now + read correctly: the line offset index indexes CSV records rather than physical lines. - Changed: removed the undeclared typing_extensions dependency (dwca.star_record now uses typing.Literal). Python 3.7, which reached end of life in June 2023, is no longer supported; the minimum is now 3.8. From 39ab414d0df68968c674cfdb90bd37c544af9731 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Nicolas=20No=C3=A9?= Date: Tue, 28 Jul 2026 17:31:27 +0200 Subject: [PATCH 28/33] Let iter_terms request the id and coreid columns --- CHANGES.txt | 2 ++ doc/tutorial.rst | 16 ++++++++++++++ dwca/descriptors.py | 17 +++++++++++++-- dwca/files.py | 7 +++++++ dwca/read.py | 7 +++++++ dwca/test/test_datafile.py | 39 +++++++++++++++++++++++++++++++++++ dwca/test/test_descriptors.py | 17 +++++++++++++++ 7 files changed, 103 insertions(+), 2 deletions(-) diff --git a/CHANGES.txt b/CHANGES.txt index 67ddd86..7a98d71 100644 --- a/CHANGES.txt +++ b/CHANGES.txt @@ -6,6 +6,8 @@ v0.17.0 (unreleased) roughly 2.9x faster than iterating over rows and reading the same terms, and roughly 10x faster than the pre-rewrite implementation, on a 400000-row archive reading 14 of 50 columns, on one machine; see benchmarks/README.md for the full measurement. +- New: iter_terms() accepts "id" and "coreid" to request the archive's key columns, which + often have no declared term of their own. These are the names headers() already uses. - Performance: iterating over an archive is a single streaming pass instead of one seek and one throwaway csv.reader per row. Measured roughly 3.8-4.0x faster on a 400000-row GBIF download on one machine; see benchmarks/README.md for the full before/after measurement. diff --git a/doc/tutorial.rst b/doc/tutorial.rst index 81a1af5..5ae60e9 100644 --- a/doc/tutorial.rst +++ b/doc/tutorial.rst @@ -116,6 +116,22 @@ A single term still yields a one-element tuple: for (locality,) in dwca.iter_terms([qn('locality')]): print(locality) +The special name ``"id"`` requests the core file's id column - the same name used by +:attr:`~dwca.descriptors.DataFileDescriptor.headers`. It can be mixed freely with regular +terms, and it resolves even when the Metafile declares no ```` for that column, which +is the common case: + +.. code:: python + + for identifier, family in dwca.iter_terms(['id', qn('family')]): + print(identifier, family) + +If the Metafile happens to declare a field literally named ``"id"`` (this occurs with +metafile-less archives, where terms are the raw CSV header names), that declared term takes +precedence over the id column, matching what ``row.data['id']`` already returns. The same +applies to ``"coreid"`` when calling :meth:`~dwca.files.CSVDataFile.iter_terms` on an +extension file (``dwca.extension_files[i]``) instead of on the reader. + Access to Darwin Core Archives with extensions (star schema) ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ diff --git a/dwca/descriptors.py b/dwca/descriptors.py index a6ec701..61fe926 100644 --- a/dwca/descriptors.py +++ b/dwca/descriptors.py @@ -267,7 +267,7 @@ def field_plan(self) -> "FieldPlan": """A cached :class:`FieldPlan` turning a split data row into a term -> value dict.""" plan = self.__dict__.get("_field_plan") if plan is None: - plan = FieldPlan(self.fields) + plan = FieldPlan(self.fields, self.id_index, self.coreid_index) self.__dict__["_field_plan"] = plan return plan @@ -335,6 +335,8 @@ class FieldPlan(object): __slots__ = ( "_fields", + "_id_index", + "_coreid_index", "_contiguous_terms", "_terms", "_getter", @@ -345,8 +347,10 @@ class FieldPlan(object): "required_columns", ) - def __init__(self, fields): + def __init__(self, fields, id_index=None, coreid_index=None): self._fields = fields + self._id_index = id_index + self._coreid_index = coreid_index indexed = [f for f in fields if f["index"] is not None] indexes = [f["index"] for f in indexed] @@ -429,6 +433,15 @@ def term_getter(self, terms): by_term = {f["term"]: f for f in self._fields} + # "id" and "coreid" name the archive's key columns. These are the names this library + # already uses for them in headers(), short_headers() and pd_read(). A declared term of + # the same name wins, which matters for metafile-less archives whose terms are raw CSV + # header names, and keeps this consistent with row.data. + if "id" not in by_term and self._id_index is not None: + by_term["id"] = {"term": "id", "index": self._id_index, "default": None} + if "coreid" not in by_term and self._coreid_index is not None: + by_term["coreid"] = {"term": "coreid", "index": self._coreid_index, "default": None} + missing = [term for term in terms if term not in by_term] if missing: raise ValueError( diff --git a/dwca/files.py b/dwca/files.py index 7d5f3e7..2549191 100644 --- a/dwca/files.py +++ b/dwca/files.py @@ -73,6 +73,13 @@ def iter_terms(self, terms: List[str]) -> Iterator[Tuple[str, ...]]: qn('decimalLatitude')]): pass + The special names "id" and "coreid" request the file's key column - the same names + used by :attr:`dwca.descriptors.DataFileDescriptor.headers`. "id" resolves for a core + file, "coreid" for an extension file; the other one raises, as does either name when + the file has no such column (metafile-less archives have neither). If the file + declares an actual term of the same name, that declared term takes precedence, which + matches what `Row.data['id']` already returns. + :param terms: a list of full term identifiers. :raises ValueError: if any of `terms` is not present in this data file. """ diff --git a/dwca/read.py b/dwca/read.py index c94d65d..3f71fad 100644 --- a/dwca/read.py +++ b/dwca/read.py @@ -269,6 +269,13 @@ def iter_terms(self, terms: List[str]) -> Iterator[Tuple[str, ...]]: [qn('occurrenceID'), qn('decimalLatitude'), qn('decimalLongitude')]): pass + The special name "id" requests the core file's id column - the same name used by + :attr:`dwca.descriptors.DataFileDescriptor.headers`. It resolves even when the + Metafile declares no ```` for that column, which is the common case. If the + core file declares an actual term named "id" (possible in metafile-less archives, + where terms are raw CSV header names), that declared term takes precedence, which + matches what `CoreRow.data['id']` already returns. + :param terms: a list of full term identifiers. :raises ValueError: if any of `terms` is not present in the core data file. """ diff --git a/dwca/test/test_datafile.py b/dwca/test/test_datafile.py index 7f0c7b0..966db5d 100644 --- a/dwca/test/test_datafile.py +++ b/dwca/test/test_datafile.py @@ -385,6 +385,45 @@ def test_can_be_nested(self): assert 16 == len(pairs) +class TestIterTermsKeyColumns(unittest.TestCase): + def test_core_id_is_reachable(self): + """dwca-ids.zip declares no for its id column.""" + with DwCAReader(sample_data_path("dwca-ids.zip")) as dwca: + from_rows = [row.id for row in dwca] + from_terms = [values[0] for values in dwca.iter_terms(["id"])] + + assert ["4", "1", "3", "2"] == from_rows + assert from_rows == from_terms + + def test_extension_coreid_is_reachable(self): + with DwCAReader(sample_data_path("dwca-2extensions.zip")) as dwca: + extension = dwca.extension_files[0] + + from_rows = [row.core_id for row in extension.iter_rows()] + from_terms = [values[0] for values in extension.iter_terms(["coreid"])] + + assert from_rows == from_terms + + def test_key_column_mixes_with_real_terms_in_the_requested_order(self): + order = "http://rs.tdwg.org/dwc/terms/order" + with DwCAReader(sample_data_path("dwca-2extensions.zip")) as dwca: + got = list(dwca.iter_terms([order, "id"])) + expected = [(row.data[order], row.id) for row in dwca] + + assert expected == got + + def test_id_is_still_unknown_when_the_file_has_no_id_column(self): + """A metafile-less archive has no id column, so the name must not resolve.""" + with DwCAReader(sample_data_path("dwca-simple-csv.zip")) as dwca: + with pytest.raises(ValueError): + dwca.iter_terms(["id"]) + + def test_coreid_is_unknown_on_a_core_file(self): + with DwCAReader(sample_data_path("dwca-2extensions.zip")) as dwca: + with pytest.raises(ValueError): + dwca.iter_terms(["coreid"]) + + class TestClosedFileGuarantee(unittest.TestCase): def test_iteration_after_close_raises(self): """close() documents that content is not accessible in any way afterwards.""" diff --git a/dwca/test/test_descriptors.py b/dwca/test/test_descriptors.py index 1470edb..faf5f3b 100644 --- a/dwca/test/test_descriptors.py +++ b/dwca/test/test_descriptors.py @@ -908,3 +908,20 @@ def test_terms_may_be_a_one_shot_iterable(self): # break non-list iterables that already worked before). assert expected == plan.term_getter(tuple(terms_list))(["first", "second"]) assert ("second",) == plan.term_getter({"http://x/b"})(["first", "second"]) + + def test_a_declared_term_named_id_wins_over_the_key_column(self): + """Terms in metafile-less archives are raw header names, so one can be "id". + + The declared term wins, which is what row.data["id"] already returns. + """ + section = """ + + occurrence.txt + + + + """ + descriptor = DataFileDescriptor.make_from_metafile_section(ET.fromstring(section)) + getter = descriptor.field_plan.term_getter(["id"]) + + assert ("declared",) == getter(["key", "declared"]) From 7ce987d476cfa3d53a3be0268d8c493d8b9345f9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Nicolas=20No=C3=A9?= Date: Tue, 28 Jul 2026 17:46:16 +0200 Subject: [PATCH 29/33] Document skip_metadata, CSVDataFile.next(), and drop stale ordering/coreid 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. --- dwca/files.py | 17 ++++++++++++++--- dwca/read.py | 8 +++++++- 2 files changed, 21 insertions(+), 4 deletions(-) diff --git a/dwca/files.py b/dwca/files.py index 2549191..68cad37 100644 --- a/dwca/files.py +++ b/dwca/files.py @@ -181,6 +181,12 @@ def __next__(self) -> str: return self.next() def next(self) -> str: # NOQA + """Return the next raw line of the data file, including its line terminator. + + Header lines are skipped. Raises `StopIteration` once the file is exhausted. This is + the iterator protocol behind `for line in data_file`, described in the class + docstring. + """ for line in self._file_stream: return line @@ -223,10 +229,15 @@ def _build_coreid_index(self) -> Dict[str, List[int]]: return index - # TODO: For ExtensionRow and a specific field only, generalize ? - # TODO: What happens if called on a Core Row? def get_all_rows_by_coreid(self, core_id: int) -> List[Row]: - """Return a list of :class:`dwca.rows.ExtensionRow` whose Core Id field match `core_id`.""" + """Return the rows whose linking id matches `core_id`. + + For an extension file, this is the row's `coreid` field. For a core file, it is the + row's own `id`, since `coreid_index` then maps each row's id to its position. The + return type is `List[Row]` to cover both :class:`dwca.rows.CoreRow` (core file) and + :class:`dwca.rows.ExtensionRow` (extension file). Returns an empty list if `core_id` + is not found. + """ if core_id not in self.coreid_index: return [] diff --git a/dwca/read.py b/dwca/read.py index 3f71fad..aeb53bd 100644 --- a/dwca/read.py +++ b/dwca/read.py @@ -38,6 +38,10 @@ class DwCAReader(object): :param tmp_dir: temporary directory to use to uncompress the archive (if needed). If not provided, Python default \ will be used. :type tmp_dir: str + :param skip_metadata: if `True`, the archive's scientific metadata file is not parsed and the `metadata` \ + attribute stays `None`. This does not affect `source_metadata`, which is still populated either way. Use this \ + to avoid the parsing cost when only the data rows are needed. + :type skip_metadata: bool :raises: :class:`dwca.exceptions.InvalidArchive` :raises: :class:`dwca.exceptions.InvalidSimpleArchive` @@ -321,10 +325,12 @@ def use_extensions(self) -> bool: return (self.descriptor is not None) and (len(self.descriptor.extensions) > 0) @property - # TODO: decide, test and document what we guarantee about ordering def rows(self) -> List[CoreRow]: """A list of :class:`rows.CoreRow` objects representing the content of the archive. + The list is in order of appearance in the core data file, the same order produced by + iterating the reader. + .. warning:: All rows will be loaded in memory. In case of a large Darwin Core Archive, you may prefer using a for loop. From 62862edb90ad8fdabe110dd5d1b9dd3fffdaae85 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Nicolas=20No=C3=A9?= Date: Tue, 28 Jul 2026 18:06:07 +0200 Subject: [PATCH 30/33] Update the CI workflow off deprecated actions 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. --- .github/workflows/run-unit-tests.yml | 24 ++++++++++-------------- 1 file changed, 10 insertions(+), 14 deletions(-) diff --git a/.github/workflows/run-unit-tests.yml b/.github/workflows/run-unit-tests.yml index 999d189..a816825 100644 --- a/.github/workflows/run-unit-tests.yml +++ b/.github/workflows/run-unit-tests.yml @@ -8,23 +8,19 @@ jobs: os: [ubuntu-latest, macos-latest, windows-latest] python-version: ['3.8', '3.9', '3.10', '3.11', '3.12', '3.13', 'pypy-3.9'] steps: - - uses: actions/checkout@v2 - - uses: actions/setup-python@v2 + - uses: actions/checkout@v4 + - uses: actions/setup-python@v5 with: python-version: ${{ matrix.python-version }} + # setup-python caches pip itself, which replaces the manual actions/cache step this + # workflow used to carry. The dependency path is explicit because the default looks + # for requirements.txt, which this project does not have. + cache: pip + cache-dependency-path: | + requirements-dev.txt + setup.py - name: Upgrade pip run: python -m pip install --upgrade pip - - name: Get pip cache dir - id: pip-cache - run: | - echo "::set-output name=dir::$(python -m pip cache dir)" - - name: pip cache - uses: actions/cache@v2 - with: - path: ${{ steps.pip-cache.outputs.dir }} - key: ${{ runner.os }}-${{ matrix.python-version }}-pip-${{ hashFiles('**/requirements.txt') }} - restore-keys: | - ${{ runner.os }}-${{ matrix.python-version }}-pip- - run: python -m pip install . - run: pip install -r requirements-dev.txt - - run: pytest \ No newline at end of file + - run: pytest From 1060d1d40b6bcf96c7011fc45ba66f2bcd97803a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Nicolas=20No=C3=A9?= Date: Tue, 28 Jul 2026 18:11:42 +0200 Subject: [PATCH 31/33] Strip a stray carriage return when the file is CRLF but declares LF 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. --- dwca/files.py | 6 +++++- dwca/rows.py | 5 ++++- dwca/test/test_characterization.py | 24 ++++++++++++++++++++++++ 3 files changed, 33 insertions(+), 2 deletions(-) diff --git a/dwca/files.py b/dwca/files.py index 68cad37..1d83bb9 100644 --- a/dwca/files.py +++ b/dwca/files.py @@ -145,7 +145,11 @@ def _iter_field_lists(self) -> Iterator[List[str]]: quoting=csv.QUOTE_MINIMAL, ) # type: Iterator[List[str]] else: - line_ending = descriptor.lines_terminated_by + # A carriage return is stripped alongside the declared terminator: archives + # routinely declare "\n" while the file itself has CRLF line endings, and the + # csv module this replaced dropped the stray CR for us. Without this, the last + # field of every row would keep it. + line_ending = descriptor.lines_terminated_by + "\r" separator = descriptor.fields_terminated_by source = (line.rstrip(line_ending).split(separator) for line in stream) diff --git a/dwca/rows.py b/dwca/rows.py index 19b78bb..09ecdf2 100644 --- a/dwca/rows.py +++ b/dwca/rows.py @@ -267,7 +267,10 @@ def csv_line_to_fields(csv_line, line_ending, field_ending, fields_enclosed_by): Return a list of fields. Content is not trimmed. """ - csv_line = csv_line.rstrip(line_ending) + # A carriage return is stripped alongside the declared terminator: archives routinely + # declare "\n" while the file itself has CRLF line endings. This matches both the csv + # module's own behavior and CSVDataFile._iter_field_lists, so the two access paths agree. + csv_line = csv_line.rstrip(line_ending + "\r") if fields_enclosed_by == "": # No enclosure: the line is simply split on the separator. This also keeps any diff --git a/dwca/test/test_characterization.py b/dwca/test/test_characterization.py index 15fe8f1..4235a34 100644 --- a/dwca/test/test_characterization.py +++ b/dwca/test/test_characterization.py @@ -110,6 +110,30 @@ def test_dos_terminator(self): with DwCAReader(path) as dwca: assert ["Borneo", "Mumbai"] == [row.data[TERM1] for row in dwca] + def test_crlf_file_declaring_a_bare_lf_terminator(self): + """A CRLF file whose Metafile declares "\\n" must not leave a stray CR on the row. + + This combination is common rather than exotic: git checks text files out with CRLF on + Windows, so an archive committed to a repository hits it without anyone choosing it. + The library's own dwca-simple-dir fixture behaves this way on a Windows runner, which + is how a regression here was caught. + """ + path = build_archive( + temp_archive_dir(self), + rows=[], + columns=2, + raw_payload=b"1\tBorneo\r\n2\tMumbai\r\n", + ) + + with DwCAReader(path) as dwca: + streamed = [row.data[TERM1] for row in dwca] + seeked = [ + dwca.core_file.get_row_by_position(i).data[TERM1] for i in range(2) + ] + + assert ["Borneo", "Mumbai"] == streamed + assert streamed == seeked + def test_multichar_terminator_is_rejected_by_python_io(self): path = build_archive( temp_archive_dir(self), From 911e8246cab6257ab9ab96d7081ee2bce8bc0557 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Nicolas=20No=C3=A9?= Date: Tue, 28 Jul 2026 18:19:44 +0200 Subject: [PATCH 32/33] Run the suite without pandas on PyPy 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. --- .github/workflows/run-unit-tests.yml | 15 ++++++++++++++- .gitignore | 2 +- dwca/test/test_dwcareader.py | 19 ++++++++++++++++++- 3 files changed, 33 insertions(+), 3 deletions(-) diff --git a/.github/workflows/run-unit-tests.yml b/.github/workflows/run-unit-tests.yml index a816825..32db29d 100644 --- a/.github/workflows/run-unit-tests.yml +++ b/.github/workflows/run-unit-tests.yml @@ -22,5 +22,18 @@ jobs: - name: Upgrade pip run: python -m pip install --upgrade pip - run: python -m pip install . - - run: pip install -r requirements-dev.txt + - name: Install dev dependencies + shell: bash + run: | + if [[ "${{ matrix.python-version }}" == pypy* ]]; then + # pandas publishes no PyPy wheels, so pip falls back to building numpy from + # source and its C++ fails to compile on these runners. pandas is an optional + # dependency of this library and the tests that need it skip themselves, so PyPy + # runs the suite without it. That also gives the "pandas is not installed" code + # path the only CI coverage it has. + grep -v '^pandas' requirements-dev.txt > requirements-ci.txt + pip install -r requirements-ci.txt + else + pip install -r requirements-dev.txt + fi - run: pytest diff --git a/.gitignore b/.gitignore index 4e72ff3..cc7f114 100644 --- a/.gitignore +++ b/.gitignore @@ -9,4 +9,4 @@ build/ *.egg-info/ .python-version .mypy_cache -.tmp/ \ No newline at end of file +.tmp/requirements-ci.txt diff --git a/dwca/test/test_dwcareader.py b/dwca/test/test_dwcareader.py index 90085cd..74f8ce9 100644 --- a/dwca/test/test_dwcareader.py +++ b/dwca/test/test_dwcareader.py @@ -3,7 +3,6 @@ import unittest import xml.etree.ElementTree as ET -import pandas as pd from unittest.mock import patch from dwca.darwincore.utils import qualname as qn @@ -12,9 +11,18 @@ from dwca.files import CSVDataFile from dwca.read import DwCAReader from dwca.rows import CoreRow, ExtensionRow +from dwca.vendor import _has_pandas from .helpers import sample_data_path import pytest +# Pandas is an optional dependency of the library, so the test suite has to run without +# it too. The tests that genuinely exercise pd_read() are skipped when it is absent; the +# one that checks pd_read's behavior WITHOUT pandas deliberately still runs. +if _has_pandas: + import pandas as pd + +requires_pandas = unittest.skipUnless(_has_pandas, "pandas is not installed") + class TestPandasIntegration(unittest.TestCase): """Tests of Pandas integration features.""" @@ -31,6 +39,7 @@ def test_pd_read_pandas_unavailable(self): with pytest.raises(ImportError): dwca.pd_read("occurrence.txt") + @requires_pandas def test_pd_read_simple_case(self): with DwCAReader(sample_data_path("dwca-simple-test-archive.zip")) as dwca: df = dwca.pd_read("occurrence.txt") @@ -56,6 +65,7 @@ def test_pd_read_simple_case(self): "betta splendens", ] + @requires_pandas def test_pd_read_chunked_default_value(self): """Pandas chuncksize should not be used with default values. @@ -66,6 +76,7 @@ def test_pd_read_chunked_default_value(self): for chunk in dwca.pd_read("occurrence.txt", chunksize=1): pass + @requires_pandas def test_pd_read_chunked(self): """If no default values are available in the archive, chunksize should work. @@ -75,6 +86,7 @@ def test_pd_read_chunked(self): for chunk in dwca.pd_read("occurrence.txt", chunksize=2): assert isinstance(chunk, pd.DataFrame) + @requires_pandas def test_pd_read_no_data_files(self): with DwCAReader(sample_data_path("dwca-simple-test-archive.zip")) as dwca: with pytest.raises(NotADataFile): @@ -83,6 +95,7 @@ def test_pd_read_no_data_files(self): with pytest.raises(NotADataFile): dwca.pd_read("eml.xml") + @requires_pandas def test_pd_read_extensions(self): with DwCAReader(sample_data_path("dwca-2extensions.zip")) as dwca: desc_df = dwca.pd_read("description.txt") @@ -95,6 +108,7 @@ def test_pd_read_extensions(self): assert vern_df.shape == (4, 4) assert vern_df["countryCode"].values.tolist() == ["US", "ZA", "FI", "ZA"] + @requires_pandas def test_pd_read_quotedir(self): with DwCAReader(sample_data_path("dwca-csv-quote-dir")) as dwca: df = dwca.pd_read("occurrence.txt") @@ -102,6 +116,7 @@ def test_pd_read_quotedir(self): assert df.shape == (2, 5) assert df["basisOfRecord"].values.tolist()[0] == "Observation, something" + @requires_pandas def test_pd_read_default_values(self): with DwCAReader(sample_data_path("dwca-test-default.zip")) as dwca: df = dwca.pd_read("occurrence.txt") @@ -110,6 +125,7 @@ def test_pd_read_default_values(self): for country in df["country"].values.tolist(): assert country == "Belgium" + @requires_pandas def test_pd_read_utf8_eol_ignored(self): """Ensure we don't split lines based on the x85 utf8 EOL char. @@ -121,6 +137,7 @@ def test_pd_read_utf8_eol_ignored(self): # (61 - and probably an IndexError - if errors) assert 64 == df.shape[1] + @requires_pandas def test_pd_read_simple_csv(self): with DwCAReader(sample_data_path("dwca-simple-csv.zip")) as dwca: df = dwca.pd_read("0008333-160118175350007.csv") From 408785d0251622e65f74e94b281d25b5dd31e09d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Nicolas=20No=C3=A9?= Date: Tue, 28 Jul 2026 18:20:26 +0200 Subject: [PATCH 33/33] Repair the .gitignore entry broken by a missing trailing newline 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. --- .gitignore | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/.gitignore b/.gitignore index cc7f114..d672b17 100644 --- a/.gitignore +++ b/.gitignore @@ -9,4 +9,5 @@ build/ *.egg-info/ .python-version .mypy_cache -.tmp/requirements-ci.txt +.tmp/ +requirements-ci.txt