Note on provenance: This issue was found by Claude Opus 5 while reviewing #325. The reproductions and output shown below were produced by building and running the code, but I have only briefly reviewed the analysis myself and I am not an expert in this area. Please review it critically.
Impact
If you pass addRow a value for a column that does not exist on the table, the value is thrown away and addRow returns Status::Success. Nothing is printed and nothing is written.
The most likely way to hit this is a typo in a column name, or adding a value for an optional column you forgot to enable when creating the table. Either way you get a file that is quietly missing data you thought you had written, and no signal at the time of writing.
The previous behaviour was to return Status::Failure, so this is a regression.
What happens
Create an epochs table without a tags column, then add a row that includes tags anyway:
auto epochs = nwbfile->createEpochs(false, 50); // false = no tags column
epochs->addRow({{"start_time", 1.0f}, {"stop_time", 2.0f}, {"tags", std::string("tag1")}});addRow returns Status::Success. Inspecting the file afterwards:
$ h5ls -r out.nwb/intervals/epochs
/id /start_time /stop_time
No tags anywhere in the file, and no diagnostic was emitted. The same happens with a plain misspelling on a table that does have the column: a row containing tagz alongside the correct keys writes the correct keys and discards tagz, still returning Success.
Cause
addRows validates in one direction only. It checks that every configured column is present in the row, then iterates m_configuredColumns and does row.find(col.name). It never iterates the caller's keys, so a key matching no column is simply never looked at.
What changed
The base revision had a size check that caught this:
if (row.size() != m_configuredColumns.size()) {
std::cerr << "... row size does not match configured column count." << std::endl;
return Status::Failure;
}That check is gone, and nothing replaced it. Current validation is at src/nwb/hdmf/table/DynamicTable.cpp:453-476, and the write loop that drives everything off m_configuredColumns is at :500 and :545.
Suggested direction
Restoring the old size comparison is not the right fix, because ragged columns break the arithmetic: a ragged column contributes two entries to m_configuredColumns (the target and its index) but only one key to the row. What is needed is a per key membership test, checking each row key against the configured column names plus the ragged target names, and failing on anything unmatched.
Worth noting that id is also never in m_configuredColumns, so a row carrying an explicit id key is silently dropped today as well.
Notes
The new test createTimeIntervalsTables in tests/testNWBFile.cpp does exactly the thing described above, writing rows with a tags key to tables created without a tags column, and passes. That test is currently asserting the buggy behaviour.
Impact
If you pass
addRowa value for a column that does not exist on the table, the value is thrown away andaddRowreturnsStatus::Success. Nothing is printed and nothing is written.The most likely way to hit this is a typo in a column name, or adding a value for an optional column you forgot to enable when creating the table. Either way you get a file that is quietly missing data you thought you had written, and no signal at the time of writing.
The previous behaviour was to return
Status::Failure, so this is a regression.What happens
Create an epochs table without a tags column, then add a row that includes tags anyway:
addRowreturnsStatus::Success. Inspecting the file afterwards:No tags anywhere in the file, and no diagnostic was emitted. The same happens with a plain misspelling on a table that does have the column: a row containing
tagzalongside the correct keys writes the correct keys and discardstagz, still returning Success.Cause
addRowsvalidates in one direction only. It checks that every configured column is present in the row, then iteratesm_configuredColumnsand doesrow.find(col.name). It never iterates the caller's keys, so a key matching no column is simply never looked at.What changed
The base revision had a size check that caught this:
That check is gone, and nothing replaced it. Current validation is at
src/nwb/hdmf/table/DynamicTable.cpp:453-476, and the write loop that drives everything offm_configuredColumnsis at:500and:545.Suggested direction
Restoring the old size comparison is not the right fix, because ragged columns break the arithmetic: a ragged column contributes two entries to
m_configuredColumns(the target and its index) but only one key to the row. What is needed is a per key membership test, checking each row key against the configured column names plus the ragged target names, and failing on anything unmatched.Worth noting that
idis also never inm_configuredColumns, so a row carrying an explicitidkey is silently dropped today as well.Notes
The new test
createTimeIntervalsTablesintests/testNWBFile.cppdoes exactly the thing described above, writing rows with atagskey to tables created without a tags column, and passes. That test is currently asserting the buggy behaviour.