Skip to content

refactor(dynamictable): derive schema-defined column routing from the schema - #858

Open
ehennestad wants to merge 6 commits into
mainfrom
codex/generate-dynamic-table-column-metadata
Open

refactor(dynamictable): derive schema-defined column routing from the schema#858
ehennestad wants to merge 6 commits into
mainfrom
codex/generate-dynamic-table-column-metadata

Conversation

@ehennestad

Copy link
Copy Markdown
Collaborator

Motivation

Background#811 taught addColumn to route schema-defined columns to their generated property instead of the generic vectordata set.

Problem — it answered "is this property a table column?" at runtime, by constructing a throwaway table plus throwaway VectorData/VectorIndex/DynamicTableRegion objects and testing which ones the property setter would accept. That is an expensive and roundabout way to ask a question the schema already answers, and it drags a pile of probe helpers into resolveColumnStorage. It is also inexact: an empty VectorData is a valid Data, so a dataset that merely accepts a column object was indistinguishable from one that is a column.

Solution — decide it from the schema, at code-generation time. A property is a table column when its dataset type descends from VectorData. Each generated DynamicTable class records its own column names, and addColumn reads that list — no dummy objects, no try-assign probing, and one shared definition of what counts as a column.

What changed

Refactor

  • resolveColumnStorage no longer instantiates throwaway objects; the probe helpers are gone and the routing decision is a lookup.
  • Generated types were regenerated; classes deriving from DynamicTable gain a private constant listing their schema-defined columns.
  • Routing of real columns is unchanged. Core types such as TimeIntervals and Units behave exactly as before: start_time still lands on the generated property, custom_column still goes to the vectordata set.

Bug fixes — neither of these was reported by a user; both surfaced once the column definition became exact.

  • Non-column datasets in a DynamicTable extension were treated as columns. An extension may declare a dataset inside a DynamicTable that is not a column — per-table metadata, a lookup table, anything typed Data rather than VectorData. Setting one registered its name in colnames, so the table advertised a column that does not exist. It no longer does, and addColumn on a name that collides with such a dataset now fails with NWB:DynamicTable:AddColumn:InvalidPropertyCollision naming the property and class, instead of a generic type-conversion error from inside the setter.
  • colnames was unvalidated where DynamicTable lives in the core namespace (NWB ≤ 2.1.0). Assigning duplicate column names to colnames was silently accepted on those schema versions. It is now rejected on every DynamicTable, matching the behavior on current schemas.
Implementation notes
  • file.internal.isSchemaDefinedTableColumn is the single definition of "this property is a table column": a scalar, non-constrained file.Dataset whose type descends from VectorData. Both fillClass and getPropertyHooks use it, so the generated syncNamedColumn post-set hooks and the column list can no longer disagree.
  • fillClass emits a private DeclaredSchemaColumns constant per class; DynamicTableBase.getSchemaDefinedColumns aggregates it across the generated hierarchy via matnwb.neurodata.internal.collectConstantPropertiesAcrossHierarchy, so inherited columns count. Index columns are included there (they are storage targets) but excluded from the syncNamedColumn hook, so they still do not appear in colnames.
  • resolveColumnStorage now returns both a storage target and a storage name, resolved through io.internal.getPropertyNameForSchemaName. The name mapping exists because the schema added events to NWBFile and events collides with a MATLAB classdef keyword; routing every property lookup through the remapper is the consistent direction, so a column whose schema name needs remapping resolves correctly. This is defensive generalization — no shipped schema has such a column today.
  • The second bug fix is the getPropertyHooks check for the colnames validator now keying on the type name rather than the fully qualified class name, so it applies wherever DynamicTable is defined.

Examples

The examples below use the test extension added in this PR (+tests/test-schema/dynamicTableColumnSchema): a DynamicTable with one VectorData dataset (schema_column, a real column) and one Data dataset (table_metadata, not a column).

A non-column dataset is no longer registered as a column

metadata = types.hdmf_common.Data('data', 'some metadata');
mixedTable = types.dtc.MixedDatasetTable( ...
    'description', 'test table', 'table_metadata', metadata);
disp(mixedTable.colnames)

Before

colnames                            : table_metadata

After

colnames                            : (empty)

Adding a colliding column reports the collision

mixedTable = types.dtc.MixedDatasetTable('description', 'test table');
invalidColumn = types.hdmf_common.VectorData( ...
    'description', 'invalid column', 'data', single((1:3)'));
mixedTable.addColumn('table_metadata', invalidColumn);

Before

ERROR (NWB:CheckDataType:InvalidConversion): Error setting property 'table_metadata' because value cannot be converted to 'char'.

After

ERROR (NWB:DynamicTable:AddColumn:InvalidPropertyCollision): Cannot add column `table_metadata` because it collides with non-column property `table_metadata` on `types.dtc.MixedDatasetTable`.

Duplicate column names are caught on core-namespace DynamicTable

With types generated for NWB 2.1.0, where DynamicTable is defined in the core namespace:

dynamicTable = types.core.DynamicTable('description', 'test');
dynamicTable.colnames = {'columnA', 'columnA'};

Before

no error raised; colnames: columnA, columnA

After

ERROR (NWB:DynamicTable:DuplicateColumnNames): Column names in `colnames` must be unique. Duplicate column name: `columnA`.

How to test

typesDir = fullfile(tempdir, "dtc-demo");
generateCore("savedir", typesDir);
generateExtension(fullfile(misc.getMatnwbDir(), "+tests", "test-schema", ...
    "dynamicTableColumnSchema", "dtc.namespace.yaml"), "savedir", typesDir);
addpath(typesDir, "-begin");
clear classes

% `table_metadata` is a plain Data dataset, not a table column.
metadata = types.hdmf_common.Data('data', 'some metadata');
mixedTable = types.dtc.MixedDatasetTable( ...
    'description', 'demo table', 'table_metadata', metadata);
fprintf("colnames after setting table_metadata: %d entries\n", numel(mixedTable.colnames));

% `schema_column` is a VectorData dataset, so it is a real column.
column = types.hdmf_common.VectorData( ...
    'description', 'demo column', 'data', single((1:3)'));
mixedTable.addColumn('schema_column', column);
fprintf("colnames after addColumn: %s\n", strjoin(mixedTable.colnames, ", "));
fprintf("stored on vectordata set: %d\n", mixedTable.vectordata.isKey('schema_column'));

% Adding a column that collides with the non-column dataset is rejected.
try
    mixedTable.addColumn('table_metadata', column);
catch ME
    fprintf("%s: %s\n", ME.identifier, ME.message);
end

Expected output:

colnames after setting table_metadata: 0 entries
colnames after addColumn: schema_column
stored on vectordata set: 0
NWB:DynamicTable:AddColumn:InvalidPropertyCollision: Cannot add column `table_metadata` because it collides with non-column property `table_metadata` on `types.dtc.MixedDatasetTable`.

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?

🤖 Generated with Claude Code

@codecov

codecov Bot commented Jul 30, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 95.16%. Comparing base (f690e82) to head (406c86a).

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #858      +/-   ##
==========================================
- Coverage   95.17%   95.16%   -0.01%     
==========================================
  Files         230      232       +2     
  Lines        8199     8198       -1     
==========================================
- Hits         7803     7802       -1     
  Misses        396      396              

☔ View full report in Codecov by Harness.
📢 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 and others added 5 commits July 30, 2026 10:41
Three of the four cases in DynamicTableColumnStorageTest duplicated
existing coverage in dynamicTableTest, and the file did no export or
read round-trip, so it did not belong under +tests/+system.

Move the one new case - a ragged schema column pair routing to its
generated properties with only the data column in colnames - into
dynamicTableTest alongside the other addColumn routing tests, and
drop the system test.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Removed test that needs full type generation for rare edge case
@ehennestad
ehennestad enabled auto-merge July 30, 2026 09:50
@ehennestad
ehennestad requested a review from bendichter July 30, 2026 09:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant