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 write a table with a ragged column (for example tags on a TimeIntervals table), close the file, reopen it, and add another row, the new row's values are written but the index that says where each row starts and ends is never updated. addRow returns Status::Success, so nothing tells you anything went wrong.
The result is a file that no longer describes itself correctly. The row count and the index disagree, the appended values are unreachable, and tools like PyNWB will either fail to read that column or return the wrong values for it. Because the write reports success, the damage is only visible later, when someone tries to read the data back.
This affects the append after read workflow that #325 added support for. It works correctly for plain columns; ragged columns are the gap.
What happens
Write a table with a ragged column and three rows of differing lengths, close, reopen, then append one more row with a two element cell:
addRow status: Success
id = 4 entries [0 1 2 3]
ragged_data = 11 values [1 2 3 4 5 6 7 8 9 13 14]
ragged_data_index = 3 entries [3 5 9] <- never extended
Confirmed independently with h5dump. The index still describes three rows while id claims four, and elements 9 and 10 belong to no row.
Cause
DynamicTable::loadConfiguredColumnsFromFile() rebuilds the column list from the colnames attribute. By HDMF convention colnames lists only target columns, never the *_index companions, and registerColumn deliberately keeps them out. So a reopened table registers no VectorIndex, targetColumns comes back empty, and the ragged cell is treated as if it were a plain column and flattened into the target dataset.
Relevant code
src/nwb/hdmf/table/DynamicTable.cpp:900-916, the restore loop that only sees m_colNames and always builds a plain VectorData:
for (const auto& colName : m_colNames) {
auto col = readColumn<VectorData>(colName);
...
}src/nwb/hdmf/table/DynamicTable.cpp:885, where index columns are kept out of colnames.
src/nwb/hdmf/table/DynamicTable.cpp:441-451, where targetColumns is built from registered VectorIndex objects and therefore ends up empty.
src/Utils.hpp:405-410, where appendCellValueToBuffer happily splices a vector into a scalar buffer and returns Success, so nothing rejects the ragged cell.
Suggested direction
loadConfiguredColumnsFromFile needs to restore index columns too. The type information is already on disk: the two argument RegisteredType::create resolves the actual type from the file via getFullTypeName(path), so the restore path could use that instead of hardcoding VectorData. Failing that, it could look for a <name>_index dataset alongside each target column and register it.
A cheap safety net regardless of the fix: reject a vector CellValue for a column that has no registered index, rather than silently flattening it.
Notes
No test covers this. The append after reopen test at tests/testDynamicTable.cpp:706-721 uses a table with no ragged column, and every ragged test writes in a single session, so the whole reopen branch is unobserved.
Related
Must be fixed together with #332. That issue is currently unreachable because this one masks it: the reopen path never registers a VectorIndex, so the hardcoded uint32 index buffer in addRows is never exercised. Fixing this issue alone starts routing reopened files into that write path, where HDMF's uint8 and uint16 indexes hit a partial commit and leave the target dataset longer than its index.
Impact
If you write a table with a ragged column (for example
tagson aTimeIntervalstable), close the file, reopen it, and add another row, the new row's values are written but the index that says where each row starts and ends is never updated.addRowreturnsStatus::Success, so nothing tells you anything went wrong.The result is a file that no longer describes itself correctly. The row count and the index disagree, the appended values are unreachable, and tools like PyNWB will either fail to read that column or return the wrong values for it. Because the write reports success, the damage is only visible later, when someone tries to read the data back.
This affects the append after read workflow that #325 added support for. It works correctly for plain columns; ragged columns are the gap.
What happens
Write a table with a ragged column and three rows of differing lengths, close, reopen, then append one more row with a two element cell:
Confirmed independently with
h5dump. The index still describes three rows whileidclaims four, and elements 9 and 10 belong to no row.Cause
DynamicTable::loadConfiguredColumnsFromFile()rebuilds the column list from thecolnamesattribute. By HDMF conventioncolnameslists only target columns, never the*_indexcompanions, andregisterColumndeliberately keeps them out. So a reopened table registers noVectorIndex,targetColumnscomes back empty, and the ragged cell is treated as if it were a plain column and flattened into the target dataset.Relevant code
src/nwb/hdmf/table/DynamicTable.cpp:900-916, the restore loop that only seesm_colNamesand always builds a plainVectorData:src/nwb/hdmf/table/DynamicTable.cpp:885, where index columns are kept out ofcolnames.src/nwb/hdmf/table/DynamicTable.cpp:441-451, wheretargetColumnsis built from registeredVectorIndexobjects and therefore ends up empty.src/Utils.hpp:405-410, whereappendCellValueToBufferhappily splices a vector into a scalar buffer and returns Success, so nothing rejects the ragged cell.Suggested direction
loadConfiguredColumnsFromFileneeds to restore index columns too. The type information is already on disk: the two argumentRegisteredType::createresolves the actual type from the file viagetFullTypeName(path), so the restore path could use that instead of hardcodingVectorData. Failing that, it could look for a<name>_indexdataset alongside each target column and register it.A cheap safety net regardless of the fix: reject a vector
CellValuefor a column that has no registered index, rather than silently flattening it.Notes
No test covers this. The append after reopen test at
tests/testDynamicTable.cpp:706-721uses a table with no ragged column, and every ragged test writes in a single session, so the whole reopen branch is unobserved.Related
Must be fixed together with #332. That issue is currently unreachable because this one masks it: the reopen path never registers a
VectorIndex, so the hardcodeduint32index buffer inaddRowsis never exercised. Fixing this issue alone starts routing reopened files into that write path, where HDMF'suint8anduint16indexes hit a partial commit and leave the target dataset longer than its index.