apply review suggestions

wraymo · wraymo · commit 67f3013e6dd6 · 2025-01-24T10:58:45.000-05:00
diff --git a/components/core/src/clp_s/indexer/CommandLineArguments.cpp b/components/core/src/clp_s/indexer/CommandLineArguments.cpp
@@ -28,7 +28,7 @@ CommandLineArguments::parse_arguments(int argc, char const** argv) {
             "db-config-file",
             po::value<std::string>(&metadata_db_config_file_path)->value_name("FILE")
                 ->default_value(metadata_db_config_file_path),
-            "Table metadata DB YAML config"
+            "Path to the YAML DB config file for metadata storage"
     );
     // clang-format on
 
@@ -37,16 +37,22 @@ CommandLineArguments::parse_arguments(int argc, char const** argv) {
     visible_options.add(general_options);
     visible_options.add(output_options);
 
-    // Define hidden positional options (not shown in Boost's program options help message)
+    std::string archive_path;
     po::options_description positional_options;
     // clang-format off
-    positional_options.add_options()
-            ("archive-dir", po::value<std::string>(&m_archive_dir))
-            ("archive-id", po::value<std::string>(&m_archive_id));
+    positional_options.add_options()(
+            "table-name",
+            po::value<std::string>(&m_table_name),
+            "Name of the table where fields from the archive will be indexed and stored"
+    )(
+            "archive-path",
+            po::value<std::string>(&archive_path),
+            "Path to an archive"
+    );
     // clang-format on
     po::positional_options_description positional_options_description;
-    positional_options_description.add("archive-dir", 1);
-    positional_options_description.add("archive-id", 1);
+    positional_options_description.add("table-name", 1);
+    positional_options_description.add("archive-path", 1);
 
     // Aggregate all options
     po::options_description all_options;
@@ -79,12 +85,13 @@ CommandLineArguments::parse_arguments(int argc, char const** argv) {
         }
 
         // Validate required parameters
-        if (m_archive_dir.empty()) {
-            throw std::invalid_argument("ARCHIVE_DIR not specified or empty.");
+        if (m_table_name.empty()) {
+            throw std::invalid_argument("Table name not specified or empty.");
         }
-        if (m_archive_id.empty()) {
-            throw std::invalid_argument("ARCHIVE_ID not specified or empty.");
+        if (archive_path.empty()) {
+            throw std::invalid_argument("Archive path not specified or empty.");
         }
+        m_archive_path = get_path_object_for_raw_path(archive_path);
         if (false == metadata_db_config_file_path.empty()) {
             clp::GlobalMetadataDBConfig metadata_db_config;
             try {
@@ -116,7 +123,7 @@ CommandLineArguments::parse_arguments(int argc, char const** argv) {
 }
 
 void CommandLineArguments::print_basic_usage() const {
-    std::cerr << "Usage: " << get_program_name() << " [OPTIONS] ARCHIVE_DIR ARCHIVE_ID"
+    std::cerr << "Usage: " << get_program_name() << " [OPTIONS] TABLE_NAME ARCHIVE_PATH"
               << std::endl;
 }
 }  // namespace clp_s::indexer
diff --git a/components/core/src/clp_s/indexer/CommandLineArguments.hpp b/components/core/src/clp_s/indexer/CommandLineArguments.hpp
@@ -5,6 +5,7 @@
 #include <string>
 
 #include "../../clp/GlobalMetadataDBConfig.hpp"
+#include "../InputConfig.hpp"
 
 namespace clp_s::indexer {
 /**
@@ -27,9 +28,9 @@ class CommandLineArguments {
 
     std::string const& get_program_name() const { return m_program_name; }
 
-    std::string const& get_archive_dir() const { return m_archive_dir; }
+    std::string const& get_table_name() const { return m_table_name; }
 
-    std::string const& get_archive_id() const { return m_archive_id; }
+    Path const& get_archive_path() const { return m_archive_path; }
 
     std::optional<clp::GlobalMetadataDBConfig> const& get_db_config() const {
         return m_metadata_db_config;
@@ -41,8 +42,8 @@ class CommandLineArguments {
 
     // Variables
     std::string m_program_name;
-    std::string m_archive_dir;
-    std::string m_archive_id;
+    std::string m_table_name;
+    Path m_archive_path;
 
     std::optional<clp::GlobalMetadataDBConfig> m_metadata_db_config;
 };
diff --git a/components/core/src/clp_s/indexer/IndexManager.cpp b/components/core/src/clp_s/indexer/IndexManager.cpp
@@ -8,15 +8,18 @@
 namespace clp_s::indexer {
 IndexManager::IndexManager(std::optional<clp::GlobalMetadataDBConfig> const& db_config) {
     if (db_config.has_value()) {
-        m_table_metadata_db = std::make_unique<MySQLIndexStorage>(
+        m_mysql_index_storage = std::make_unique<MySQLIndexStorage>(
                 db_config->get_metadata_db_host(),
                 db_config->get_metadata_db_port(),
                 db_config->get_metadata_db_username(),
                 db_config->get_metadata_db_password(),
                 db_config->get_metadata_db_name(),
                 db_config->get_metadata_table_prefix()
         );
-        m_table_metadata_db->open();
+        m_mysql_index_storage->open();
+        m_field_update_callback = [this](std::string& field_name, NodeType field_type) {
+            m_mysql_index_storage->add_field(field_name, field_type);
+        };
         m_output_type = OutputType::Database;
     } else {
         throw OperationFailed(ErrorCodeBadParam, __FILENAME__, __LINE__);
@@ -25,32 +28,17 @@ IndexManager::IndexManager(std::optional<clp::GlobalMetadataDBConfig> const& db_
 
 IndexManager::~IndexManager() {
     if (m_output_type == OutputType::Database) {
-        m_table_metadata_db->close();
+        m_mysql_index_storage->close();
     }
 }
 
-void IndexManager::update_metadata(std::string const& archive_dir, std::string const& archive_id) {
-    m_table_metadata_db->init(archive_dir);
-
-    auto archive_path = std::filesystem::path(archive_dir) / archive_id;
-    std::error_code ec;
-    if (false == std::filesystem::exists(archive_path, ec) || ec) {
-        throw OperationFailed(ErrorCodeBadParam, __FILENAME__, __LINE__);
-    }
+void IndexManager::update_metadata(std::string const& table_name, Path const& archive_path) {
+    m_mysql_index_storage->init(table_name);
 
     ArchiveReader archive_reader;
-    archive_reader.open(
-            clp_s::Path{.source = clp_s::InputSource::Filesystem, .path = archive_path.string()},
-            NetworkAuthOption{}
-    );
+    archive_reader.open(archive_path, NetworkAuthOption{});
 
-    auto schema_tree = archive_reader.get_schema_tree();
-    auto field_pairs = traverse_schema_tree(schema_tree);
-    if (OutputType::Database == m_output_type) {
-        for (auto& [name, type] : field_pairs) {
-            m_table_metadata_db->add_field(name, type);
-        }
-    }
+    traverse_schema_tree_and_update_metadata(archive_reader.get_schema_tree());
 }
 
 std::string IndexManager::escape_key_name(std::string_view const key_name) {
@@ -100,49 +88,42 @@ std::string IndexManager::escape_key_name(std::string_view const key_name) {
     return escaped_key_name;
 }
 
-std::vector<std::pair<std::string, clp_s::NodeType>> IndexManager::traverse_schema_tree(
+void IndexManager::traverse_schema_tree_and_update_metadata(
         std::shared_ptr<SchemaTree> const& schema_tree
 ) {
-    std::vector<std::pair<std::string, clp_s::NodeType>> fields;
     if (nullptr == schema_tree) {
-        return fields;
+        return;
     }
 
     std::string path_buffer;
     // Stack of pairs of node_id and path_length
     std::stack<std::pair<int32_t, uint64_t>> s;
-    for (auto const& node : schema_tree->get_nodes()) {
-        if (constants::cRootNodeId == node.get_parent_id()
-            && clp_s::NodeType::Metadata != node.get_type())
-        {
-            s.emplace(node.get_id(), 0);
-            break;
-        }
-    }
+    s.emplace(schema_tree->get_object_subtree_node_id(), 0);
 
     while (false == s.empty()) {
         auto [node_id, path_length] = s.top();
         s.pop();
 
         auto const& node = schema_tree->get_node(node_id);
-        auto const& children_ids = node.get_children_ids();
         auto node_type = node.get_type();
+        // TODO: Add support for structured arrays
+        if (NodeType::StructuredArray == node_type) {
+            continue;
+        }
+        auto const& children_ids = node.get_children_ids();
         path_buffer.resize(path_length);
         if (false == path_buffer.empty()) {
             path_buffer += ".";
         }
         path_buffer += escape_key_name(node.get_key_name());
-        if (children_ids.empty() && clp_s::NodeType::Object != node_type
-            && clp_s::NodeType::Unknown != node_type)
+        if (children_ids.empty() && NodeType::Object != node_type && NodeType::Unknown != node_type)
         {
-            fields.emplace_back(path_buffer, node_type);
+            m_field_update_callback(path_buffer, node_type);
         }
 
         for (auto child_id : children_ids) {
             s.emplace(child_id, path_buffer.size());
         }
     }
-
-    return fields;
 }
 }  // namespace clp_s::indexer
diff --git a/components/core/src/clp_s/indexer/IndexManager.hpp b/components/core/src/clp_s/indexer/IndexManager.hpp
@@ -1,17 +1,26 @@
 #ifndef CLP_S_INDEXER_INDEXMANAGER_HPP
 #define CLP_S_INDEXER_INDEXMANAGER_HPP
 
+#include <functional>
+#include <optional>
+
 #include "../../clp/GlobalMetadataDBConfig.hpp"
 #include "../ArchiveReader.hpp"
 #include "MySQLIndexStorage.hpp"
 
 namespace clp_s::indexer {
 /**
- * Class used to updates field names (e.g., JSON full paths) and data types for a specified archive
- * directory. It currently stores the results in a database. An archive directory consists of
- * multiple archives on the same topic, which can be queried using SQL. The directory name serves as
- * the table name, and its schema (field names and data types) is used by the SQL engine to resolve
- * column metadata.
+ * This class updates field names (e.g., JSON full paths) and data types for a specified archive.
+ * It traverses the schema tree from root to leaf, concatenating escaped key names using the
+ * delimiter `.` at each level.
+ *
+ * Currently, only leaf nodes with primitive types are indexed. Object nodes and subtrees of
+ * structured arrays are not indexed. The results are stored in a database to enable efficient
+ * querying.
+ *
+ * Multiple archives related to the same topic can form a table that can be queried using a SQL
+ * query engine. When indexing, a table name must be specified. This table is then used by the SQL
+ * engine to resolve column metadata.
  */
 class IndexManager {
 public:
@@ -36,10 +45,10 @@ class IndexManager {
     // Methods
     /**
      * Updates the metadata for a given archive
-     * @param archive_dir used as the table name
-     * @param archive_id
+     * @param table_name
+     * @param archive_path
      */
-    void update_metadata(std::string const& archive_dir, std::string const& archive_id);
+    void update_metadata(std::string const& table_name, Path const& archive_path);
 
 private:
     /**
@@ -50,15 +59,13 @@ class IndexManager {
     static std::string escape_key_name(std::string_view const key_name);
 
     /**
-     * Traverses the schema tree and returns a list of path names and their types
+     * Traverses the schema tree and updates the metadata
      * @param schema_tree
-     * @return a list of path names and their types
      */
-    std::vector<std::pair<std::string, clp_s::NodeType>> traverse_schema_tree(
-            std::shared_ptr<SchemaTree> const& schema_tree
-    );
+    void traverse_schema_tree_and_update_metadata(std::shared_ptr<SchemaTree> const& schema_tree);
 
-    std::shared_ptr<MySQLIndexStorage> m_table_metadata_db;
+    std::function<void(std::string&, NodeType)> m_field_update_callback;
+    std::shared_ptr<MySQLIndexStorage> m_mysql_index_storage;
     OutputType m_output_type{OutputType::Database};
 };
 }  // namespace clp_s::indexer
diff --git a/components/core/src/clp_s/indexer/indexer.cpp b/components/core/src/clp_s/indexer/indexer.cpp
@@ -34,8 +34,8 @@ int main(int argc, char const* argv[]) {
     try {
         clp_s::indexer::IndexManager index_manager(command_line_arguments.get_db_config());
         index_manager.update_metadata(
-                command_line_arguments.get_archive_dir(),
-                command_line_arguments.get_archive_id()
+                command_line_arguments.get_table_name(),
+                command_line_arguments.get_archive_path()
         );
     } catch (std::exception& e) {
         SPDLOG_ERROR("Failed to update metadata: {}", e.what());
