Make the ragged check in DynamicTable.add_row incremental

CHANGELOG.md

@@ -10,8 +10,10 @@
 - Added support for hdmf-common schema 1.10.0, which changes ``MeaningsTable.target`` from a link to an object-reference attribute (``dtype`` with ``reftype: object``, ``target_type: VectorData``). Files written with the hdmf-common 1.9.0 ``MeaningsTable`` (a link named "target") are still read correctly via a backwards-compatibility mapping in ``MeaningsTableMap``. There is no change in the read/write API; the change is limited to the representation on disk. @rly [#1525](https://github.com/hdmf-dev/hdmf/pull/1525)
 - ``DynamicTable.get_meanings_for_column`` (and the ``AlignedDynamicTable`` override) now returns ``None`` when the named column exists but has no ``MeaningsTable``, and raises ``KeyError`` only when the column itself does not exist. @rly [#1538](https://github.com/hdmf-dev/hdmf/pull/1538)
 - `HDMFIO.__del__` emits a `ResourceWarning` for an IO that was not closed instead of calling `close()`. Still-open IOs are flushed and closed by an `atexit` handler on the main thread. @rly [#1547](https://github.com/hdmf-dev/hdmf/pull/1547)
+- `DynamicTable.add_row` now warns at most once per column that the column has become ragged, on the row that makes it ragged, instead of on that row and every row after it. @h-mayorquin [#1561](https://github.com/hdmf-dev/hdmf/pull/1561)
 
 ### Fixed
+- Fixed `DynamicTable.add_row` being quadratic in the number of rows when `check_ragged` is on, which is the default. Filling 16,000 rows into a one-column table drops from 22 s to 0.3 s. @h-mayorquin [#1561](https://github.com/hdmf-dev/hdmf/pull/1561)
 - Fixed a deadlock when exporting a Zarr file to HDF5 with `HDF5IO.export`. A zarr array is now read into memory before the HDF5 write (in `__list_fill__`/`__scalar_fill__`), so the read does not run while h5py's global lock is held. @rly [#1547](https://github.com/hdmf-dev/hdmf/pull/1547)
 - Fixed `IndexError` when selecting an empty region from an in-memory `DynamicTableRegion` (e.g. `table["region"][i]` for a ragged region row that references no target rows, or an empty slice), both when the target table's columns hold their data as numpy arrays and when the target table has ragged columns. @h-mayorquin [#1549](https://github.com/hdmf-dev/hdmf/pull/1549)
 - Fixed the Jupyter HTML representation (`_repr_html_`) rendering a scalar numpy `bool` or `int` attribute (e.g. `np.bool_`, `np.int64`, as read back from an HDF5 attribute) as an expandable "array" block reporting `Shape: ()`, while `float` and `str` scalars rendered inline. `_unwrap_scalar` now also unwraps numpy scalars (`np.generic`) so every scalar renders inline consistently. @h-mayorquin [#1546](https://github.com/hdmf-dev/hdmf/pull/1546)

src/hdmf/common/table.py

@@ -394,6 +394,10 @@ def __init__(self, **kwargs):  # noqa: C901
         # map name to column specification
         self.__uninit_cols = dict()
 
+        # state that keeps the ragged check in add_row O(1) instead of a rescan of the whole column
+        self.__ragged_columns = set()                # columns found ragged, which are never checked again
+        self.__elements_checked_per_column = dict()  # column name to how many of its elements were checked
+
         # All tables must have ElementIdentifiers (i.e. a primary key column)
         # Here, we figure out what to do for that
         user_provided_ids = (id is not None)
@@ -818,7 +822,7 @@ def _add_extra_predefined_columns(self, data: dict):
              'default': False},
             {'name': 'check_ragged', 'type': bool, 'default': True,
              'doc': ('whether or not to check for ragged arrays when adding data to the table. '
-                     'Set to False to avoid checking every element if performance issues occur.')},
+                     'Set to False to skip the check.')},
             allow_extra=True)
     def add_row(self, **kwargs):
         """
@@ -849,12 +853,40 @@ def add_row(self, **kwargs):
                 col.add_vector(data[colname])
             else:
                 col.add_row(data[colname])
-                if check_ragged and is_ragged(col.data):
+                if check_ragged and self.__becomes_ragged(colname, col):
                     warn(("Data has elements with different lengths and therefore cannot be coerced into an "
                           "N-dimensional array. Use the 'index' argument when creating a column to add rows "
                           "with different lengths."),
                          stacklevel=3)
 
+    def __becomes_ragged(self, colname, col):
+        """
+        Whether the value just appended is the one that makes this column ragged.
+
+        A column is warned about once, on the row that makes it ragged: a ragged column never becomes
+        unragged, so the same message on every later row carries no new information. ``is_ragged``
+        answers that question by scanning the whole column, which costs O(rows) per row and makes
+        filling a table quadratic. Every element the check has already seen has the same length as the
+        first element and is not ragged within itself, so the appended value makes the column ragged
+        exactly when it differs from the first element, which is what this compares. The count of
+        elements seen guards that invariant: when the column grew by anything other than this one
+        append, such as a row added with ``check_ragged=False``, the column is rescanned in full once.
+        The count tracks length, so it does not detect elements swapped in place by code that reaches
+        into ``col.data`` directly.
+        """
+        data = col.data
+        if colname in self.__ragged_columns or not isinstance(data, (list, tuple)):
+            return False  # is_ragged is False for anything that is not a list or a tuple
+        checked = self.__elements_checked_per_column.get(colname, 0)
+        self.__elements_checked_per_column[colname] = len(data)
+        if len(data) != checked + 1:
+            ragged = is_ragged(data)  # data did not get here through a single append, so rescan it once
+        else:
+            ragged = is_ragged([data[0], data[-1]])
+        if ragged:
+            self.__ragged_columns.add(colname)
+        return ragged
+
     def __eq__(self, other):
         """Compare if the two DynamicTables contain the same data.
 

tests/unit/common/test_table.py

@@ -1,10 +1,14 @@
 from collections import OrderedDict
 import h5py
+import itertools
 import numpy as np
 import os
 import pandas as pd
 import unittest
+from unittest.mock import patch
+import warnings
 
+import hdmf.common.table
 from hdmf import Container
 from hdmf import TermSet, TermSetWrapper
 from hdmf.backends.hdf5 import H5DataIO, HDF5IO
@@ -20,7 +24,7 @@
     get_manager,
     SimpleMultiContainer)
 from hdmf.testing import TestCase, H5RoundTripMixin, remove_test_file
-from hdmf.utils import StrDataset
+from hdmf.utils import StrDataset, is_ragged
 from hdmf.data_utils import DataChunkIterator
 
 from tests.unit.helpers.utils import (
@@ -531,6 +535,73 @@ def test_add_row_without_required_index_and_no_ragged_check(self):
         table.add_row(foo=5, bar=50.0, baz='lizard', qux=[1, 2, 3])
         table.add_row(foo=5, bar=50.0, baz='lizard', qux=[1, 2, 3 ,4], check_ragged=False)
 
+    def test_add_row_ragged_check_matches_is_ragged(self):
+        """
+        For a column built one row at a time, the warning must fire on exactly the row where
+        is_ragged turns true and on no row after it, whatever the shapes of the values.
+        """
+        # one element per class is_ragged distinguishes: a scalar and a string, which both count as
+        # length 1, an empty sequence, sequences of length 1 and 2, a tuple, and a nested element that
+        # is ragged inside itself
+        element_shapes = (1, 'ab', [], [1], [1, 2], (1, 2), [[1], [2, 3]])
+        for combination in itertools.product(element_shapes, repeat=3):
+            column = VectorData(name='qux', description='qux column', data=[])
+            table = DynamicTable(name='table', description='table description', columns=[column])
+            data = []
+            was_ragged = False
+            for element in combination:
+                data.append(element)
+                with warnings.catch_warnings(record=True) as caught:
+                    warnings.simplefilter('always')
+                    table.add_row(qux=element)
+                warned = any('different lengths' in str(warning.message) for warning in caught)
+                self.assertEqual(warned, is_ragged(data) and not was_ragged, msg=str(data))
+                was_ragged = is_ragged(data)
+
+    def test_add_row_ragged_check_sees_elements_it_did_not_check(self):
+        """
+        Elements the check never saw, because the column already had data or because a row was added
+        with check_ragged=False, still count towards raggedness for the rows after them.
+        """
+        msg = ("Data has elements with different lengths and therefore cannot be coerced into an N-dimensional "
+               "array. Use the 'index' argument when creating a column to add rows with different lengths.")
+        # a column built with data before the first add_row is checked against all of it, and here the
+        # appended value matches the first element, so only the data already in the column makes it ragged
+        table = DynamicTable(name='table', description='table description',
+                             columns=[VectorData(name='qux', description='qux column', data=[[1], [2, 3]])])
+        with self.assertWarnsWith(UserWarning, msg):
+            table.add_row(qux=[4])
+
+        table = self.with_spec()
+        table.add_column(name='qux', description='qux column')
+        table.add_row(foo=5, bar=50.0, baz='lizard', qux=[1, 2])
+        table.add_row(foo=5, bar=50.0, baz='lizard', qux=[1, 2, 3], check_ragged=False)
+        # again the new row matches the length of the first element, so only the skipped row makes this ragged
+        with self.assertWarnsWith(UserWarning, msg):
+            table.add_row(foo=5, bar=50.0, baz='lizard', qux=[4, 5])
+
+    def test_add_row_ragged_check_does_not_rescan_the_column(self):
+        """
+        The raggedness check must cost O(1) per row: rescanning the column makes add_row quadratic.
+        """
+        visited = 0
+        original_is_ragged = hdmf.common.table.is_ragged
+
+        def counting_is_ragged(data):
+            nonlocal visited
+            visited += len(data) if isinstance(data, (list, tuple)) else 1
+            return original_is_ragged(data)
+
+        num_rows = 1000
+        table = self.with_spec()
+        table.add_column(name='qux', description='qux column')
+        with patch.object(hdmf.common.table, 'is_ragged', counting_is_ragged):
+            for _ in range(num_rows):
+                table.add_row(foo=5, bar=50.0, baz='lizard', qux=1)
+
+        # rescanning the column would visit about num_rows ** 2 / 2 elements
+        self.assertLess(visited, 10 * num_rows)
+
     def test_add_column_auto_index_int(self):
         """
         Add a column as a list of lists after we have already added data so that we need to create a single VectorIndex