Support overwrite of string array attributes

src/io/BaseIO.hpp

@@ -329,11 +329,13 @@ class BaseIO
    * @param data The string array attribute data.
    * @param path The location in the file to set the attribute.
    * @param name The name of the attribute.
+   * @param overwrite Overwrite the attribute if it already exists.
    * @return The status of the attribute creation operation.
    */
   virtual Status createAttribute(const std::vector<std::string>& data,
                                  const std::string& path,
-                                 const std::string& name) = 0;
+                                 const std::string& name,
+                                 const bool overwrite = false) = 0;
 
   /**
    * @brief Creates a string array attribute at a given location in the file.

src/io/hdf5/HDF5IO.cpp

@@ -683,7 +683,8 @@ Status HDF5IO::createAttribute(const std::string& data,
 
 Status HDF5IO::createAttribute(const std::vector<std::string>& data,
                                const std::string& path,
-                               const std::string& name)
+                               const std::string& name,
+                               const bool overwrite)
 {
   H5Object* loc;
   Group gloc;
@@ -715,7 +716,12 @@ Status HDF5IO::createAttribute(const std::vector<std::string>& data,
 
   try {
     if (loc->attrExists(name)) {
-      return Status::Failure;  // don't allow overwriting
+      if (overwrite) {
+        // Delete the existing attribute
+        loc->removeAttr(name);
+      } else {
+        return Status::Failure;  // don't allow overwriting
+      }
     }
 
     // Create dataspace based on number of strings

src/io/hdf5/HDF5IO.hpp

@@ -157,11 +157,13 @@ class HDF5IO : public BaseIO
    * @param data The string array attribute data.
    * @param path The location in the file to set the attribute.
    * @param name The name of the attribute.
+   * @param overwrite Overwrite the attribute if it already exists.
    * @return The status of the attribute creation operation.
    */
   Status createAttribute(const std::vector<std::string>& data,
                          const std::string& path,
-                         const std::string& name) override;
+                         const std::string& name,
+                         const bool overwrite = false) override;
 
   /**
    * @brief Creates a fixed-length string array attribute at a given location in

src/nwb/file/ElectrodeTable.cpp

@@ -54,14 +54,14 @@ Status ElectrodeTable::initialize(const std::string& description)
                                    AQNWB::mergePaths(m_path, "id"))));
   Status groupNameStatus = m_groupNamesDataset->initialize(
       std::unique_ptr<IO::BaseRecordingData>(
-          m_io->createArrayDataSet(IO::BaseDataType::STR(250),
+          m_io->createArrayDataSet(IO::BaseDataType::V_STR,
                                    SizeArray {0},
                                    SizeArray {1},
                                    AQNWB::mergePaths(m_path, "group_name"))),
       "the name of the ElectrodeGroup this electrode is a part of");
   Status locationStatus = m_locationsDataset->initialize(
       std::unique_ptr<IO::BaseRecordingData>(
-          m_io->createArrayDataSet(IO::BaseDataType::STR(250),
+          m_io->createArrayDataSet(IO::BaseDataType::V_STR,
                                    SizeArray {0},
                                    SizeArray {1},
                                    AQNWB::mergePaths(m_path, "location"))),
@@ -91,14 +91,14 @@ Status ElectrodeTable::finalize()
 {
   Status rowIdStatus = setRowIDs(m_electrodeDataset, m_electrodeNumbers);
   Status locationColStatus = addColumn(m_locationsDataset, m_locationNames);
-  Status grouColStatus = addReferenceColumn(
+  Status groupColStatus = addReferenceColumn(
       "group",
       "a reference to the ElectrodeGroup this electrode is a part of",
       m_groupReferences);
   Status groupNameColStatus = addColumn(m_groupNamesDataset, m_groupNames);
   Status finalizeStatus = DynamicTable::finalize();
   if (rowIdStatus != Status::Success || locationColStatus != Status::Success
-      || grouColStatus != Status::Success
+      || groupColStatus != Status::Success
       || groupNameColStatus != Status::Success
       || finalizeStatus != Status::Success)
   {

src/nwb/hdmf/base/Container.cpp

@@ -19,9 +19,12 @@ Container::~Container() {}
 /** Initialize */
 Status Container::initialize()
 {
-  m_io->createGroup(m_path);
+  auto createGroupStatus = m_io->createGroup(m_path);
   // setup common attributes
-  m_io->createCommonNWBAttributes(
+  auto createAttrsStatus = m_io->createCommonNWBAttributes(
       m_path, this->getNamespace(), this->getTypeName());
-  return Status::Success;
+  return (createGroupStatus == Status::Success
+          && createAttrsStatus == Status::Success)
+      ? Status::Success
+      : Status::Failure;
 }

src/nwb/hdmf/table/DynamicTable.cpp

@@ -14,6 +14,16 @@ DynamicTable::DynamicTable(const std::string& path,
     : Container(path, io)
     , m_colNames({})
 {
+  // Read the colNames attribute if it exists such that any columns
+  // we may add append to the existing list of columns rather than
+  // replacing it. This is important for the finalize function
+  // to ensure that all columns are correctly listed.
+  if (m_io->isOpen()) {
+    auto colNamesFromFile = readColNames();
+    if (colNamesFromFile->exists()) {
+      m_colNames = colNamesFromFile->values().data;
+    }
+  }
 }
 
 /** Destructor */
@@ -23,8 +33,9 @@ DynamicTable::~DynamicTable() {}
 Status DynamicTable::initialize(const std::string& description)
 {
   Status containerStatus = Container::initialize();
-  if (description != "")
+  if (description != "") {
     m_io->createAttribute(description, m_path, "description");
+  }
   return containerStatus;
 }
 
@@ -37,18 +48,12 @@ Status DynamicTable::addColumn(std::unique_ptr<VectorData>& vectorData,
     return Status::Failure;
   } else {
     // write in loop because variable length string
-    Status writeStatus = Status::Success;
-    Status blockStatus = Status::Success;
-    for (SizeType i = 0; i < values.size(); i++) {
-      blockStatus = vectorData->m_dataset->writeDataBlock(
-          std::vector<SizeType> {1},
-          std::vector<SizeType> {i},
-          IO::BaseDataType::STR(values[i].size() + 1),
-          values);  // TODO - add tests for this
-      if (blockStatus != Status::Success) {
-        writeStatus = Status::Failure;
-      }
-    }
+    // Write all strings in a single block
+    Status writeStatus = vectorData->m_dataset->writeDataBlock(
+        std::vector<SizeType> {values.size()},
+        std::vector<SizeType> {0},
+        IO::BaseDataType::V_STR,
+        values);
     m_colNames.push_back(vectorData->getName());
     return writeStatus;
   }
@@ -101,6 +106,11 @@ Status DynamicTable::addReferenceColumn(const std::string& name,
 
 Status DynamicTable::finalize()
 {
-  Status colNamesStatus = m_io->createAttribute(m_colNames, m_path, "colnames");
+  Status colNamesStatus = m_io->createAttribute(
+      m_colNames,
+      m_path,
+      "colnames",
+      true  // overwrite the attribute if it already exists
+  );
   return colNamesStatus;
 }

src/nwb/hdmf/table/DynamicTable.hpp

@@ -41,7 +41,6 @@ class DynamicTable : public Container
    * column names.
    *
    * @param description The description of the table (optional).
-   * @param colNames Names of the columns for the table
    * @return Status::Success if successful, otherwise Status::Failure.
    */
   Status initialize(const std::string& description);
@@ -86,7 +85,17 @@ class DynamicTable : public Container
                    const std::vector<int>& values);
 
   /**
-   * @brief Sets the column names of the ElectrodeTable.
+   * @brief Sets the column names of the DynamicTable
+   *
+   * ..note::
+   * For this change to take affect in the file we need to call
+   * finalize() after setting the column names to write the data to the file.
+   *
+   * .. warning::
+   * This will overwrite any existing column names. It is up to
+   * the caller to ensure that all existing columns are included in the new
+   * list.
+   *
    * @param newColNames The vector of new column names.
    */
   virtual void setColNames(const std::vector<std::string>& newColNames)

tests/CMakeLists.txt

@@ -15,6 +15,7 @@ add_executable(aqnwb_test
     testBaseIO.cpp
     testData.cpp
     testDevice.cpp
+    testDynamicTable.cpp
     testEcephys.cpp
     testElementIdentifiers.cpp
     testFile.cpp

tests/testDynamicTable.cpp

@@ -0,0 +1,168 @@
+#include <catch2/catch_test_macros.hpp>
+#include <catch2/matchers/catch_matchers_all.hpp>
+
+#include "io/hdf5/HDF5IO.hpp"
+#include "nwb/hdmf/table/DynamicTable.hpp"
+#include "testUtils.hpp"
+
+using namespace AQNWB;
+
+TEST_CASE("DynamicTable", "[table]")
+{
+  std::string tablePath = "/test_table";
+
+  SECTION("test initialization and column names")
+  {
+    std::string path = getTestFilePath("testDynamicTable.h5");
+    std::shared_ptr<BaseIO> io = createIO("HDF5", path);
+    io->open();
+
+    NWB::DynamicTable table(tablePath, io);
+    Status status = table.initialize("A test dynamic table");
+    REQUIRE(status == Status::Success);
+
+    // Test reading description
+    auto readDesc = table.readDescription()->values().data;
+    REQUIRE(readDesc[0] == "A test dynamic table");
+
+    // Test setting and reading column names
+    std::vector<std::string> colNames = {"col1", "col2", "col3"};
+    table.setColNames(colNames);
+    status = table.finalize();
+    REQUIRE(status == Status::Success);
+
+    auto readColNames = table.readColNames()->values().data;
+    REQUIRE(readColNames == colNames);
+
+    io->close();
+  }
+
+  SECTION("test adding columns and row IDs")
+  {
+    std::string path = getTestFilePath("testDynamicTableColumns.h5");
+    std::shared_ptr<BaseIO> io = createIO("HDF5", path);
+    io->open();
+
+    NWB::DynamicTable table(tablePath, io);
+    Status status = table.initialize("Table with columns");
+    REQUIRE(status == Status::Success);
+
+    // Add string vector data column
+    std::vector<std::string> values = {"value1", "value2", "value3"};
+    SizeArray dataShape = {values.size()};
+    SizeArray chunking = {values.size()};
+    auto columnDataset = io->createArrayDataSet(
+        BaseDataType::V_STR, dataShape, chunking, tablePath + "/col1");
+    auto vectorData =
+        std::make_unique<NWB::VectorData>(tablePath + "/col1", io);
+    vectorData->initialize(std::move(columnDataset), "Column 1");
+    status = table.addColumn(vectorData, values);
+    REQUIRE(status == Status::Success);
+
+    // Set row IDs
+    std::vector<int> ids = {1, 2, 3};
+    SizeArray idShape = {ids.size()};
+    SizeArray idChunking = {ids.size()};
+    auto idDataset = io->createArrayDataSet(
+        BaseDataType::I32, idShape, idChunking, tablePath + "/id");
+    auto elementIDs =
+        std::make_unique<NWB::ElementIdentifiers>(tablePath + "/id", io);
+    elementIDs->initialize(std::move(idDataset));
+    status = table.setRowIDs(elementIDs, ids);
+    REQUIRE(status == Status::Success);
+
+    // Finalize table
+    status = table.finalize();
+    REQUIRE(status == Status::Success);
+
+    io->close();
+
+    // Reopen file and verify data
+    io = createIO("HDF5", path);
+    io->open();
+    NWB::DynamicTable readTable(tablePath, io);
+
+    auto readColNames = readTable.readColNames()->values().data;
+    std::vector<std::string> expectedColNames = {"col1"};
+    REQUIRE(readColNames == expectedColNames);
+
+    // Read row IDs
+    auto readIdsData = readTable.readId<int>()->values().data;
+    REQUIRE(readIdsData == ids);
+
+    io->close();
+  }
+
+  SECTION("test appending column to existing table")
+  {
+    // First create a table with initial columns
+    std::string path = getTestFilePath("testDynamicTableAppend.h5");
+    {
+      std::shared_ptr<BaseIO> io = createIO("HDF5", path);
+      io->open();
+
+      NWB::DynamicTable table(tablePath, io);
+      Status status = table.initialize("Table for appending");
+      REQUIRE(status == Status::Success);
+
+      // Add initial column
+      std::vector<std::string> values = {"value1", "value2", "value3"};
+      SizeArray dataShape = {values.size()};
+      SizeArray chunking = {values.size()};
+      std::string columnPath = mergePaths(tablePath, "col1");
+      auto columnDataset = io->createArrayDataSet(
+          BaseDataType::V_STR, dataShape, chunking, columnPath);
+      auto vectorData = std::make_unique<NWB::VectorData>(columnPath, io);
+      vectorData->initialize(std::move(columnDataset), "Column 1");
+      status = table.addColumn(vectorData, values);
+      REQUIRE(status == Status::Success);
+
+      // table.setColNames({"col1"});
+      status = table.finalize();
+      REQUIRE(status == Status::Success);
+
+      io->close();
+    }
+
+    // Now reopen and append new column
+    {
+      std::shared_ptr<BaseIO> io = createIO("HDF5", path);
+      io->open();
+
+      NWB::DynamicTable table(tablePath, io);
+
+      // Add new column
+      std::vector<std::string> newValues = {"new1", "new2", "new3"};
+      SizeArray newDataShape = {newValues.size()};
+      SizeArray newChunking = {newValues.size()};
+      std::string columnPath2 = mergePaths(tablePath, "col2");
+      auto newColumnDataset = io->createArrayDataSet(
+          BaseDataType::V_STR, newDataShape, newChunking, columnPath2);
+      auto newVectorData = std::make_unique<NWB::VectorData>(columnPath2, io);
+      newVectorData->initialize(std::move(newColumnDataset), "Column 2");
+      Status status = table.addColumn(newVectorData, newValues);
+      REQUIRE(status == Status::Success);
+
+      // Finalize the table
+      status = table.finalize();
+      REQUIRE(status == Status::Success);
+
+      // Verify updated column names
+      std::vector<std::string> colNames = {"col1", "col2"};
+      auto readColNames = table.readColNames()->values().data;
+      REQUIRE(readColNames == colNames);
+
+      // Swap the columns
+      colNames = {"col2", "col1"};
+      table.setColNames(colNames);
+      status = table.finalize();
+      REQUIRE(status == Status::Success);
+
+      // Verify updated column names
+      auto readColNames2 = table.readColNames()->values().data;
+      REQUIRE(readColNames2 == colNames);
+
+      io->close();
+    }
+  }
+}