Fix addColumn routing for schema-defined DynamicTable columns

[ehennestad]

Motivation

DynamicTable.addColumn always stored added columns in vectordata, which left DynamicTable subclasses with schema-defined column properties in an inconsistent state. For tables such as types.core.TimeIntervals, adding a column like start_time should populate the corresponding property rather than only updating the vectordata set.

This change updates column insertion to resolve the correct storage location for each added column. Schema-backed columns are routed to their matching property, generic columns still go to vectordata, and collisions with non-column properties now fail explicitly. The update also adds focused unit coverage for property-backed columns, generic columns, and invalid property collisions.

Todo

• Resolve whether this is the right approach, or if addColumn should not work for table properties at all
• Fix copilot suggestions
•  Improve patch coverage

How to test the behavior?

Example 1: Adding description column:

dynamicTable = types.hdmf_common.DynamicTable();
dynamicTable.addColumn('description', types.hdmf_common.VectorData('data', 1:2))

This should fail because description is a DynamicTable non-column property

Before:

>> dynamicTable

dynamicTable = 

  DynamicTable with properties:

       colnames: {'description'}
    description: ''
             id: [1×1 types.hdmf_common.ElementIdentifiers]
     vectordata: [1×1 types.untyped.Set]

Warning: The following required properties are missing for instance of type
"types.hdmf_common.DynamicTable":
    description 

a VectorData object with name "description" was added to the vectordata set.

After:

Error using types.util.dynamictable.addVarargColumn>resolveStorageTargets (line 76)
Cannot add column `description` because it collides with non-column property `description` on
`types.hdmf_common.DynamicTable`.

Error in types.util.dynamictable.addVarargColumn (line 10)
storageTargets = resolveStorageTargets(DynamicTable, newColNames, struct2cell(newVectorData));
                 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
Error in types.util.dynamictable.addColumn (line 37)
    types.util.dynamictable.addVarargColumn(DynamicTable, varargin{:});
    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
Error in types.hdmf_common.DynamicTable/addColumn (line 119)
        types.util.dynamictable.addColumn(obj, varargin{:});
        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

Example 2: Adding start_time column to TimeIntervals:

dynamicTable = types.core.TimeIntervals();
dynamicTable.addColumn('start_time', types.hdmf_common.VectorData('data',1:2))

Before:

>> dynamicTable

dynamicTable = 

  TimeIntervals with properties:

          start_time: []
           stop_time: []
                tags: []
          tags_index: []
          timeseries: []
    timeseries_index: []
            colnames: {'start_time'}
         description: ''
                  id: [1×1 types.hdmf_common.ElementIdentifiers]
          vectordata: [1×1 types.untyped.Set]

Warning: The following required properties are missing for instance of type
"types.core.TimeIntervals":
    description
    start_time
    stop_time 

A VectorData object with name "start_time" was added to the vectordata set, not the start_time property.

After:

>> dynamicTable

dynamicTable = 

  TimeIntervals with properties:

          start_time: [1×1 types.hdmf_common.VectorData]
           stop_time: []
                tags: []
          tags_index: []
          timeseries: []
    timeseries_index: []
            colnames: {'start_time'}
         description: ''
                  id: [1×1 types.hdmf_common.ElementIdentifiers]
          vectordata: [0×1 types.untyped.Set]

Warning: The following required properties are missing for instance of type
"types.core.TimeIntervals":
    description
    stop_time 

The VectorData object is now added to the start_time property.

Updated unitest:

nwbtest('Name', 'tests.unit.dynamicTableTest');

Checklist

• Have you ensured the PR description clearly describes the problem and solutions?
• Have you checked to ensure that there aren't other open or previously closed Pull Requests for the same change?
• If this PR fixes an issue, is the first line of the PR description fix #XX where XX is the issue number?

[codecov]

Codecov Report

❌ Patch coverage is 98.88889% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 95.32%. Comparing base (faf3801) to head (84f4b1e).

Files with missing lines	Patch %	Lines
+types/+util/+dynamictable/addVarargColumn.m	97.43%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #811      +/-   ##
==========================================
+ Coverage   95.25%   95.32%   +0.06%     
==========================================
  Files         209      212       +3     
  Lines        7508     7587      +79     
==========================================
+ Hits         7152     7232      +80     
+ Misses        356      355       -1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[ehennestad]

@bendichter @rly @oruebel Would appreciate input on the conceptual issue here.

Strictly speaking, we are not adding the start_time column itself, only populating its data. That said, addColumn is a convenient way to ensure "start_time" is included in colnames property and that id is initialized if empty.

An alternative would be to disallow addColumn for column properties defined on subtypes, such as start_time on TimeIntervals and only support creation via TimeIntervals constructor or direct dot-assignment, i.e

timeIntervals.start_time = types.hdmf_common.VectorData('data', 1)

[Copilot]

Pull request overview

Fixes DynamicTable.addColumn so newly added columns are stored in the correct location: schema-backed columns populate their corresponding class properties, while non-schema columns continue to go into vectordata. This prevents inconsistent state in DynamicTable subclasses with schema-defined column properties (e.g., types.core.TimeIntervals).

Changes:

• Add column storage resolution logic to route columns to either a schema property or vectordata, and explicitly error on collisions with non-column properties.
• Update vararg column insertion to use resolved storage targets and to detect pre-existing columns across both properties and vectordata.
• Add unit tests covering schema property routing, non-schema routing, wrong-type errors, and invalid property collisions.

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.

Show a summary per file

File	Description
+types/+util/+dynamictable/resolveColumnStorage.m	New resolver that decides whether a column name maps to a schema-backed property or should go to vectordata, and throws on non-column collisions.
+types/+util/+dynamictable/addVarargColumn.m	Uses the resolver to select storage targets, expands “already exists” detection, and assigns columns accordingly.
+tests/+unit/dynamicTableTest.m	Adds focused unit coverage for property-backed columns, vectordata columns, wrong-type routing behavior, and collision errors.
+matnwb/+common/+validation/mustBeVectorData.m	Adds an arguments-block validator used by the new resolver.
+matnwb/+common/+validation/mustBeDynamicTable.m	Adds an arguments-block validator used by the new resolver.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

resolveColumnStorage instantiates new dummy table/column objects (and potentially multiple dummy column objects) per column via feval(class(...)). This can add noticeable overhead when adding many columns. Consider caching a dummy table/dummy column set per DynamicTable class (e.g., with persistent storage keyed by class(dynamicTable)), or passing a prebuilt dummy table/dummy columns into the resolver when adding multiple columns.

[ehennestad]

Not concerned about overhead here. If that surfaces as a real world issue, it should be fixed then. A more stable long term fix is to improve the generated classes to use property validation in the property blocks, that way the metaclass object can be inspected instead of creating dummy objects.