feat(clp-s): Add an indexer process to populate column names and types of each table #672
Conversation
WalkthroughThis pull request introduces a new Changes
Sequence DiagramsequenceDiagram
participant CLI as Command Line
participant Args as CommandLineArguments
participant IndexMgr as IndexManager
participant DBStorage as MySQLIndexStorage
participant Archive as ArchiveReader
CLI->>Args: Parse arguments
Args-->>CLI: Return parsing result
CLI->>IndexMgr: Create with DB config
IndexMgr->>DBStorage: Open connection
IndexMgr->>Archive: Read archive
IndexMgr->>IndexMgr: Traverse schema tree
IndexMgr->>DBStorage: Store metadata
Possibly Related PRs
Suggested Reviewers
✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (8)
components/core/src/clp_s/metadata_uploader/MySQLTableMetadataDB.cpp (1)
87-89: Add error logging for failed statement executionWhen the
execute()method fails, it would be helpful to log the error to aid in troubleshooting.Apply this diff to add error logging:
if (false == m_insert_field_statement->execute()) { + SPDLOG_ERROR("Failed to execute insert field statement"); throw OperationFailed(ErrorCodeFailure, __FILENAME__, __LINE__); }components/core/src/clp_s/metadata_uploader/TableMetadataManager.cpp (1)
39-41: Log error code before throwing exceptionIncluding the error code
ecin the log before throwing the exception can help diagnose filesystem errors.Apply this diff to add error logging:
if (false == std::filesystem::exists(archive_path, ec) || ec) { + SPDLOG_ERROR("Filesystem error while checking archive path {}: {}", archive_path, ec.message()); throw OperationFailed(ErrorCodeBadParam, __FILENAME__, __LINE__); }components/core/src/clp_s/metadata_uploader/CommandLineArguments.cpp (1)
111-114: Adjust log level for user input errorsLogging user input errors at the error level might be too severe for expected input mistakes. Consider using
SPDLOG_WARNinstead ofSPDLOG_ERRORfor these messages.Apply this diff to adjust the log level:
} catch (std::exception& e) { - SPDLOG_ERROR("{}", e.what()); + SPDLOG_WARN("{}", e.what()); print_basic_usage(); return ParsingResult::Failure; }components/core/src/clp_s/metadata_uploader/CommandLineArguments.hpp (1)
27-35: Add documentation for getter methods.Please add documentation comments for these getter methods to improve code maintainability. Include descriptions of return values and any potential exceptions.
components/core/src/clp_s/metadata_uploader/TableMetadataManager.hpp (1)
35-40: Enhance method documentation.The documentation for
update_metadatashould include:
- What the method does with the archive_dir and archive_id
- Any exceptions that might be thrown
- Any preconditions or postconditions
components/core/src/clp_s/metadata_uploader/MySQLTableMetadataDB.hpp (2)
57-64: Consider secure credential handling.Storing database credentials as plain text member variables poses a security risk. Consider:
- Using environment variables or a secure configuration system
- Clearing sensitive data after connection is established
50-53: Add documentation for public methods.Please add documentation for these public methods:
open(): When to call it, what it does, possible exceptionsinit(): Purpose of table_name parameter, initialization stepsclose(): Cleanup behaviour, when to call itadd_field(): Parameter requirements, behaviour on duplicate fieldscomponents/core/src/clp_s/metadata_uploader/CMakeLists.txt (1)
3-70: Consider organizing source files into logical groupsThe source file list is quite extensive and could benefit from better organization. Consider grouping related files into separate variables for improved maintainability.
Example organization:
set(AWS_SOURCES ../../clp/aws/AwsAuthenticationSigner.cpp ../../clp/aws/AwsAuthenticationSigner.hpp ) set(CORE_SOURCES ../../clp/BoundedReader.cpp ../../clp/BoundedReader.hpp # ... other core files ) set(DATABASE_SOURCES ../../clp/database_utils.cpp ../../clp/database_utils.hpp # ... other database files ) set(METADATA_UPLOADER_SOURCES ${AWS_SOURCES} ${CORE_SOURCES} ${DATABASE_SOURCES} )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
components/core/src/clp_s/CMakeLists.txt(1 hunks)components/core/src/clp_s/metadata_uploader/CMakeLists.txt(1 hunks)components/core/src/clp_s/metadata_uploader/CommandLineArguments.cpp(1 hunks)components/core/src/clp_s/metadata_uploader/CommandLineArguments.hpp(1 hunks)components/core/src/clp_s/metadata_uploader/MySQLTableMetadataDB.cpp(1 hunks)components/core/src/clp_s/metadata_uploader/MySQLTableMetadataDB.hpp(1 hunks)components/core/src/clp_s/metadata_uploader/TableMetadataManager.cpp(1 hunks)components/core/src/clp_s/metadata_uploader/TableMetadataManager.hpp(1 hunks)components/core/src/clp_s/metadata_uploader/metadata_uploader.cpp(1 hunks)
🧰 Additional context used
📓 Path-based instructions (7)
components/core/src/clp_s/metadata_uploader/metadata_uploader.cpp (1)
Pattern **/*.{cpp,hpp,java,js,jsx,ts,tsx}: - Prefer false == <expression> rather than !<expression>.
components/core/src/clp_s/metadata_uploader/CommandLineArguments.cpp (1)
Pattern **/*.{cpp,hpp,java,js,jsx,ts,tsx}: - Prefer false == <expression> rather than !<expression>.
components/core/src/clp_s/metadata_uploader/TableMetadataManager.cpp (1)
Pattern **/*.{cpp,hpp,java,js,jsx,ts,tsx}: - Prefer false == <expression> rather than !<expression>.
components/core/src/clp_s/metadata_uploader/TableMetadataManager.hpp (1)
Pattern **/*.{cpp,hpp,java,js,jsx,ts,tsx}: - Prefer false == <expression> rather than !<expression>.
components/core/src/clp_s/metadata_uploader/MySQLTableMetadataDB.cpp (1)
Pattern **/*.{cpp,hpp,java,js,jsx,ts,tsx}: - Prefer false == <expression> rather than !<expression>.
components/core/src/clp_s/metadata_uploader/CommandLineArguments.hpp (1)
Pattern **/*.{cpp,hpp,java,js,jsx,ts,tsx}: - Prefer false == <expression> rather than !<expression>.
components/core/src/clp_s/metadata_uploader/MySQLTableMetadataDB.hpp (1)
Pattern **/*.{cpp,hpp,java,js,jsx,ts,tsx}: - Prefer false == <expression> rather than !<expression>.
🪛 GitHub Actions: clp-lint
components/core/src/clp_s/metadata_uploader/CommandLineArguments.cpp
[error] 6-6: Code formatting violation: incorrect formatting in include statement
components/core/src/clp_s/metadata_uploader/TableMetadataManager.cpp
[error] 69-69: Code formatting violation: incorrect formatting in for loop statement
components/core/src/clp_s/metadata_uploader/TableMetadataManager.hpp
[error] 22-22: Code formatting violation: incorrect spacing around colon in enum class declaration
components/core/src/clp_s/metadata_uploader/MySQLTableMetadataDB.cpp
[error] 68-68: Code formatting violation: incorrect line alignment or spacing in function declaration
⏰ Context from checks skipped due to timeout of 90000ms (11)
- GitHub Check: ubuntu-jammy-static-linked-bins
- GitHub Check: ubuntu-jammy-dynamic-linked-bins
- GitHub Check: ubuntu-focal-static-linked-bins
- GitHub Check: ubuntu-focal-dynamic-linked-bins
- GitHub Check: centos-stream-9-static-linked-bins
- GitHub Check: centos-stream-9-dynamic-linked-bins
- GitHub Check: build-macos (macos-14, false)
- GitHub Check: build-macos (macos-14, true)
- GitHub Check: build-macos (macos-13, false)
- GitHub Check: build (macos-latest)
- GitHub Check: build-macos (macos-13, true)
🔇 Additional comments (7)
components/core/src/clp_s/metadata_uploader/CommandLineArguments.hpp (1)
1-50: Well-designed class structure!The class follows SOLID principles with clear separation of concerns and const-correctness. The use of enum class for parsing results is a good practice.
components/core/src/clp_s/metadata_uploader/MySQLTableMetadataDB.hpp (1)
66-69: Good use of RAII pattern!The use of
std::unique_ptrfor managing the prepared statement lifecycle is a good practice.components/core/src/clp_s/metadata_uploader/CMakeLists.txt (3)
96-101: LGTM: Output directory configurationThe configuration to place the executable at the root of the build directory is clear and follows common practices.
3-78: Verify relative paths to source filesThe source files are referenced using relative paths (../../clp/). This could be fragile if the directory structure changes.
Run this script to verify all source files exist:
80-95: Review dependency versions and compatibilityThe target links against multiple external libraries. Ensure all dependencies are compatible with C++20 and each other.
Run this script to check for potential version conflicts:
✅ Verification successful
Dependencies are properly versioned and compatible
All dependencies have explicit minimum version requirements that are compatible with C++20 and with each other. The project properly manages version dependencies, including the noted synchronization between spdlog (1.9.2+) and fmt (8.0.1+) libraries.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check versions of required libraries in package files # Check for version specifications in package files echo "Checking package versions..." fd -e json -e yaml -e txt -e cmake package || true fd CMakeLists.txt -x grep -l "find_package" {} \; || trueLength of output: 637
Script:
#!/bin/bash # Check version requirements in core CMakeLists.txt echo "=== Core CMakeLists.txt Contents ===" cat components/core/CMakeLists.txt | grep -A 1 "find_package" echo -e "\n=== Checking for specific version requirements ===" cat components/core/CMakeLists.txt | grep -E "VERSION|version|REQUIRED"Length of output: 2795
components/core/src/clp_s/CMakeLists.txt (2)
2-2: LGTM: Subdirectory integrationThe addition of the metadata_uploader subdirectory is properly integrated into the build system.
1-2: Verify build order dependenciesThe metadata_uploader subdirectory is added after search/kql. Ensure there are no build dependencies between these components that would require a specific order.
Run this script to check for potential dependencies:
✅ Verification successful
Build order is correct - no dependencies found
The components are independent modules with no cross-dependencies. The metadata-uploader executable and kql library have separate build targets with distinct purposes and dependency sets. The current order in the root CMakeLists.txt is acceptable.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for dependencies between metadata_uploader and search/kql # Look for includes or references between the components echo "Checking cross-component dependencies..." rg -l "search/kql" components/core/src/clp_s/metadata_uploader/ || true rg -l "metadata_uploader" components/core/src/clp_s/search/kql/ || trueLength of output: 232
Script:
#!/bin/bash # Check CMake files for build dependencies echo "Checking CMake dependencies..." echo "metadata_uploader CMakeLists.txt:" cat components/core/src/clp_s/metadata_uploader/CMakeLists.txt 2>/dev/null || echo "File not found" echo -e "\nsearch/kql CMakeLists.txt:" cat components/core/src/clp_s/search/kql/CMakeLists.txt 2>/dev/null || echo "File not found"Length of output: 4344
| m_db.execute_query(fmt::format( | ||
| "CREATE TABLE IF NOT EXISTS {}{} (" | ||
| "name VARCHAR(512) NOT NULL, " | ||
| "type BIGINT NOT NULL," | ||
| "PRIMARY KEY (name, type)" | ||
| ")", | ||
| m_table_prefix, | ||
| table_name | ||
| )); |
There was a problem hiding this comment.
Potential SQL injection vulnerability in table creation
Using unescaped or unparameterized user input in SQL statements can lead to SQL injection vulnerabilities. The table_name and m_table_prefix variables should be properly escaped to prevent injection attacks.
Consider using functions provided by the database library to escape table and column names.
| void MySQLTableMetadataDB::add_field(std::string const& field_name, | ||
| NodeType field_type) { | ||
| if (false == m_is_init) { |
There was a problem hiding this comment.
Fix code formatting in function declaration
The function declaration at line 68 has incorrect line alignment or spacing, as indicated by the pipeline failure. Please adjust the formatting to comply with the code style guidelines.
Apply this diff to correct the formatting:
void MySQLTableMetadataDB::add_field(std::string const& field_name,
- NodeType field_type) {
+ NodeType field_type) {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| void MySQLTableMetadataDB::add_field(std::string const& field_name, | |
| NodeType field_type) { | |
| if (false == m_is_init) { | |
| void MySQLTableMetadataDB::add_field(std::string const& field_name, | |
| NodeType field_type) { | |
| if (false == m_is_init) { |
🧰 Tools
🪛 GitHub Actions: clp-lint
[error] 68-68: Code formatting violation: incorrect line alignment or spacing in function declaration
| m_table_metadata_db->init(archive_dir); | ||
|
|
||
| auto archive_path = std::filesystem::path(archive_dir) / archive_id; |
There was a problem hiding this comment.
Risk of SQL injection through unescaped table names
Passing archive_dir as the table name to init may lead to SQL injection vulnerabilities if archive_dir contains malicious input. Ensure that table names are properly validated and escaped before use in SQL statements.
Consider sanitizing or validating archive_dir to prevent injection attacks.
| std::string path_buffer; | ||
| // Stack of pairs of node_id and path_length | ||
| std::stack<std::pair<int32_t, uint64_t>> s; | ||
| for (auto &node : schema_tree->get_nodes()) { |
There was a problem hiding this comment.
Fix code formatting in for loop statement
The for loop at line 69 has incorrect formatting, as indicated by the pipeline failure. Please adjust the spacing around the ampersand.
Apply this diff to correct the formatting:
-for (auto &node : schema_tree->get_nodes()) {
+for (auto& node : schema_tree->get_nodes()) {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| for (auto &node : schema_tree->get_nodes()) { | |
| for (auto& node : schema_tree->get_nodes()) { |
🧰 Tools
🪛 GitHub Actions: clp-lint
[error] 69-69: Code formatting violation: incorrect formatting in for loop statement
| #include <iostream> | ||
|
|
||
| #include <boost/program_options.hpp> | ||
| #include <spdlog/spdlog.h> |
There was a problem hiding this comment.
Fix code formatting in include statement
The include statement at line 6 has incorrect formatting, as indicated by the pipeline failure. Please ensure the include directives are properly grouped and formatted according to the code style guidelines.
🧰 Tools
🪛 GitHub Actions: clp-lint
[error] 6-6: Code formatting violation: incorrect formatting in include statement
| int main(int argc, char const* argv[]) { | ||
| try { | ||
| auto stderr_logger = spdlog::stderr_logger_st("stderr"); | ||
| spdlog::set_default_logger(stderr_logger); | ||
| spdlog::set_pattern("%Y-%m-%dT%H:%M:%S.%e%z [%l] %v"); | ||
| } catch (std::exception& e) { | ||
| return 1; | ||
| } | ||
|
|
||
| CommandLineArguments command_line_arguments("metadata-uploader"); | ||
| auto parsing_result = command_line_arguments.parse_arguments(argc, argv); | ||
| switch (parsing_result) { | ||
| case CommandLineArguments::ParsingResult::Failure: | ||
| return 1; | ||
| case CommandLineArguments::ParsingResult::InfoCommand: | ||
| return 0; | ||
| case CommandLineArguments::ParsingResult::Success: | ||
| // Continue processing | ||
| break; | ||
| } | ||
|
|
||
| clp_s::metadata_uploader::TableMetadataManager table_metadata_manager( | ||
| command_line_arguments.get_db_config() | ||
| ); | ||
| table_metadata_manager.update_metadata( | ||
| command_line_arguments.get_archive_dir(), | ||
| command_line_arguments.get_archive_id() | ||
| ); | ||
| } |
There was a problem hiding this comment.
Enhance error handling and add missing return statement.
Several improvements needed for robustness:
- Log the exception details in the catch block
- Add a try-catch block around TableMetadataManager operations
- Add a return statement for the success path
Apply this diff:
int main(int argc, char const* argv[]) {
try {
auto stderr_logger = spdlog::stderr_logger_st("stderr");
spdlog::set_default_logger(stderr_logger);
spdlog::set_pattern("%Y-%m-%dT%H:%M:%S.%e%z [%l] %v");
} catch (std::exception& e) {
+ std::cerr << "Failed to initialize logger: " << e.what() << std::endl;
return 1;
}
CommandLineArguments command_line_arguments("metadata-uploader");
auto parsing_result = command_line_arguments.parse_arguments(argc, argv);
switch (parsing_result) {
case CommandLineArguments::ParsingResult::Failure:
return 1;
case CommandLineArguments::ParsingResult::InfoCommand:
return 0;
case CommandLineArguments::ParsingResult::Success:
// Continue processing
break;
}
- clp_s::metadata_uploader::TableMetadataManager table_metadata_manager(
- command_line_arguments.get_db_config()
- );
- table_metadata_manager.update_metadata(
- command_line_arguments.get_archive_dir(),
- command_line_arguments.get_archive_id()
- );
+ try {
+ clp_s::metadata_uploader::TableMetadataManager table_metadata_manager(
+ command_line_arguments.get_db_config()
+ );
+ table_metadata_manager.update_metadata(
+ command_line_arguments.get_archive_dir(),
+ command_line_arguments.get_archive_id()
+ );
+ return 0;
+ } catch (std::exception& e) {
+ spdlog::error("Failed to update metadata: {}", e.what());
+ return 1;
+ }
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| int main(int argc, char const* argv[]) { | |
| try { | |
| auto stderr_logger = spdlog::stderr_logger_st("stderr"); | |
| spdlog::set_default_logger(stderr_logger); | |
| spdlog::set_pattern("%Y-%m-%dT%H:%M:%S.%e%z [%l] %v"); | |
| } catch (std::exception& e) { | |
| return 1; | |
| } | |
| CommandLineArguments command_line_arguments("metadata-uploader"); | |
| auto parsing_result = command_line_arguments.parse_arguments(argc, argv); | |
| switch (parsing_result) { | |
| case CommandLineArguments::ParsingResult::Failure: | |
| return 1; | |
| case CommandLineArguments::ParsingResult::InfoCommand: | |
| return 0; | |
| case CommandLineArguments::ParsingResult::Success: | |
| // Continue processing | |
| break; | |
| } | |
| clp_s::metadata_uploader::TableMetadataManager table_metadata_manager( | |
| command_line_arguments.get_db_config() | |
| ); | |
| table_metadata_manager.update_metadata( | |
| command_line_arguments.get_archive_dir(), | |
| command_line_arguments.get_archive_id() | |
| ); | |
| } | |
| int main(int argc, char const* argv[]) { | |
| try { | |
| auto stderr_logger = spdlog::stderr_logger_st("stderr"); | |
| spdlog::set_default_logger(stderr_logger); | |
| spdlog::set_pattern("%Y-%m-%dT%H:%M:%S.%e%z [%l] %v"); | |
| } catch (std::exception& e) { | |
| std::cerr << "Failed to initialize logger: " << e.what() << std::endl; | |
| return 1; | |
| } | |
| CommandLineArguments command_line_arguments("metadata-uploader"); | |
| auto parsing_result = command_line_arguments.parse_arguments(argc, argv); | |
| switch (parsing_result) { | |
| case CommandLineArguments::ParsingResult::Failure: | |
| return 1; | |
| case CommandLineArguments::ParsingResult::InfoCommand: | |
| return 0; | |
| case CommandLineArguments::ParsingResult::Success: | |
| // Continue processing | |
| break; | |
| } | |
| try { | |
| clp_s::metadata_uploader::TableMetadataManager table_metadata_manager( | |
| command_line_arguments.get_db_config() | |
| ); | |
| table_metadata_manager.update_metadata( | |
| command_line_arguments.get_archive_dir(), | |
| command_line_arguments.get_archive_id() | |
| ); | |
| return 0; | |
| } catch (std::exception& e) { | |
| spdlog::error("Failed to update metadata: {}", e.what()); | |
| return 1; | |
| } | |
| } |
| enum class OutputType: uint8_t { | ||
| Database | ||
| }; |
There was a problem hiding this comment.
Fix enum class formatting.
Add a space after the colon as per the linter error.
- enum class OutputType: uint8_t {
+ enum class OutputType : uint8_t {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| enum class OutputType: uint8_t { | |
| Database | |
| }; | |
| enum class OutputType : uint8_t { | |
| Database | |
| }; |
🧰 Tools
🪛 GitHub Actions: clp-lint
[error] 22-22: Code formatting violation: incorrect spacing around colon in enum class declaration
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
components/core/src/clp_s/metadata_uploader/MySQLTableMetadataDB.cpp (1)
30-38:⚠️ Potential issueSQL injection vulnerability in table creation.
The table_name and m_table_prefix variables should be properly escaped to prevent SQL injection attacks.
Consider using a method to escape identifiers:
+ std::string escaped_table_name = m_db.escape_identifier(table_name); + std::string escaped_prefix = m_db.escape_identifier(m_table_prefix); m_db.execute_query(fmt::format( "CREATE TABLE IF NOT EXISTS {}{} (" "name VARCHAR(512) NOT NULL, " "type BIGINT NOT NULL," "PRIMARY KEY (name, type)" ")", - m_table_prefix, - table_name + escaped_prefix, + escaped_table_name ));
🧹 Nitpick comments (6)
components/core/src/clp_s/metadata_uploader/MySQLTableMetadataDB.cpp (2)
16-23: Add parameter validation before opening the connection.Consider validating connection parameters (m_host, m_port, etc.) before attempting to open the database connection to provide better error messages.
void MySQLTableMetadataDB::open() { if (m_is_open) { throw OperationFailed(ErrorCodeNotReady, __FILENAME__, __LINE__); } + if (m_host.empty() || m_username.empty() || m_database_name.empty()) { + throw OperationFailed(ErrorCodeInvalidArgument, __FILENAME__, __LINE__); + } + m_db.open(m_host, m_port, m_username, m_password, m_database_name); m_is_open = true; }
25-66: Consider wrapping table creation in a transaction.For better atomicity, consider wrapping the table creation and prepared statement initialization in a transaction.
void MySQLTableMetadataDB::init(std::string const& table_name) { if (false == m_is_open) { throw OperationFailed(ErrorCodeNotReady, __FILENAME__, __LINE__); } + m_db.execute_query("START TRANSACTION"); + try { // ... existing code ... m_is_init = true; + m_db.execute_query("COMMIT"); + } catch (...) { + m_db.execute_query("ROLLBACK"); + throw; + } }components/core/src/clp_s/metadata_uploader/CommandLineArguments.cpp (4)
14-17: Consider usingargc < 2for better readability.The condition
1 == argcfollows the Yoda condition style, but it's not required by the coding guidelines for this specific comparison.- if (1 == argc) { + if (argc < 2) {
26-33: Remove redundant clang-format directives.The clang-format directives are unnecessary as the formatting is already correct and consistent.
Also applies to: 42-46
88-108: Enhance error handling for database configuration.The database configuration validation could be improved:
- Consider extracting the database configuration validation into a separate method for better readability.
- Add more specific error messages for different failure scenarios.
if (false == metadata_db_config_file_path.empty()) { - clp::GlobalMetadataDBConfig metadata_db_config; - try { - metadata_db_config.parse_config_file(metadata_db_config_file_path); - } catch (std::exception& e) { - SPDLOG_ERROR("Failed to validate metadata database config - {}.", e.what()); - return ParsingResult::Failure; - } - - if (clp::GlobalMetadataDBConfig::MetadataDBType::MySQL - != metadata_db_config.get_metadata_db_type()) - { - SPDLOG_ERROR( - "Invalid metadata database type for {}; only supported type is MySQL.", - m_program_name - ); - return ParsingResult::Failure; - } - - m_metadata_db_config = std::move(metadata_db_config); + if (auto result = validate_and_load_db_config(metadata_db_config_file_path); + result != ParsingResult::Success) { + return result; + } }Add this new method to the class:
ParsingResult validate_and_load_db_config(const std::string& config_path) { clp::GlobalMetadataDBConfig metadata_db_config; try { metadata_db_config.parse_config_file(config_path); } catch (const std::runtime_error& e) { SPDLOG_ERROR("Failed to read metadata database config file - {}.", e.what()); return ParsingResult::Failure; } catch (const std::exception& e) { SPDLOG_ERROR("Failed to validate metadata database config - {}.", e.what()); return ParsingResult::Failure; } if (clp::GlobalMetadataDBConfig::MetadataDBType::MySQL != metadata_db_config.get_metadata_db_type()) { SPDLOG_ERROR( "Invalid metadata database type for {}; only MySQL is supported.", m_program_name ); return ParsingResult::Failure; } m_metadata_db_config = std::move(metadata_db_config); return ParsingResult::Success; }
118-121: Enhance usage message with examples and descriptions.Consider adding:
- A brief description of what the program does
- Examples of common use cases
- Description of what ARCHIVE_DIR and ARCHIVE_ID represent
void CommandLineArguments::print_basic_usage() const { std::cerr << "Usage: " << get_program_name() << " [OPTIONS] ARCHIVE_DIR ARCHIVE_ID" - << std::endl; + << std::endl + << std::endl + << "Populates column names and types for MySQL tables from archive metadata." + << std::endl + << std::endl + << "Arguments:" << std::endl + << " ARCHIVE_DIR Directory containing the archive files" << std::endl + << " ARCHIVE_ID Unique identifier for the archive" << std::endl + << std::endl + << "Example:" << std::endl + << " " << get_program_name() << " --db-config-file=config.yaml /path/to/archive archive123" + << std::endl; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
components/core/src/clp_s/metadata_uploader/CommandLineArguments.cpp(1 hunks)components/core/src/clp_s/metadata_uploader/MySQLTableMetadataDB.cpp(1 hunks)components/core/src/clp_s/metadata_uploader/TableMetadataManager.cpp(1 hunks)components/core/src/clp_s/metadata_uploader/TableMetadataManager.hpp(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- components/core/src/clp_s/metadata_uploader/TableMetadataManager.cpp
- components/core/src/clp_s/metadata_uploader/TableMetadataManager.hpp
🧰 Additional context used
📓 Path-based instructions (2)
components/core/src/clp_s/metadata_uploader/CommandLineArguments.cpp (1)
Pattern **/*.{cpp,hpp,java,js,jsx,ts,tsx}: - Prefer false == <expression> rather than !<expression>.
components/core/src/clp_s/metadata_uploader/MySQLTableMetadataDB.cpp (1)
Pattern **/*.{cpp,hpp,java,js,jsx,ts,tsx}: - Prefer false == <expression> rather than !<expression>.
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: build-macos (macos-14, false)
- GitHub Check: build-macos (macos-13, false)
- GitHub Check: lint-check (ubuntu-latest)
- GitHub Check: build (macos-latest)
- GitHub Check: build-macos (macos-13, true)
- GitHub Check: lint-check (macos-latest)
🔇 Additional comments (4)
components/core/src/clp_s/metadata_uploader/MySQLTableMetadataDB.cpp (3)
1-14: LGTM! Well-structured file organization.The includes are appropriate, and the enum class provides type-safe field indexing.
68-89: LGTM! Secure implementation using prepared statements.The method properly uses prepared statements for parameter binding and has appropriate error handling.
91-96: LGTM! Proper resource cleanup.The method properly cleans up resources and resets state flags in the correct order.
components/core/src/clp_s/metadata_uploader/CommandLineArguments.cpp (1)
1-8: Reorder include statements according to standard practice.The include statements should be grouped in the following order:
- Related header file
- C++ standard library headers
- Third-party library headers
#include "CommandLineArguments.hpp" #include <iostream> - #include <boost/program_options.hpp> #include <spdlog/spdlog.h>
There was a problem hiding this comment.
Nice work! Putting up an initial set of comments for everything but MySQlTableMetadataDB.{hpp,cpp}. Will come back to review those later.
One other high level thing is I'm not sure if "TableMetadata" is the best name since its pretty generic. Not sure what would be better though. Alternatively we could just have more descriptive docstrings for the classes to describe the current and potential future usecases of TableMetadata.
components/core/src/clp_s/metadata_uploader/CommandLineArguments.cpp
Outdated
Show resolved
Hide resolved
| positional_options.add_options() | ||
| ("archive-dir", po::value<std::string>(&m_archive_dir)) | ||
| ("archive-id", po::value<std::string>(&m_archive_id)); |
There was a problem hiding this comment.
It might be best to refactor these arguments/the CommandLineArguments class in general to match the current style in clp-s. That way the rest of the code can transparently support both the local fs and s3 flow.
We could do it as one positional "archive-path" option which we convert to a Path object using get_path_object_for_raw_path + a --auth option.
There was a problem hiding this comment.
Yes, in general, get_path_object_for_raw_path is more suitable. However, the indexer is specifically used for the package. Within the package, the archive exists in the local file system before being uploaded to S3. A complicating thing is that we use the archive-dir as the table name for PrestoDB. If we pass the path directly, we would need to first concatenate archive_output_dir and archive id within the package, then manually extract the directory name in the indexer process, which would add unnecessary complexity to the code.
There was a problem hiding this comment.
Just realized all archives are stored in the same directory in the package. I will change it to archive-path, and probably add another command line option table_name with a default value.
components/core/src/clp_s/metadata_uploader/TableMetadataManager.cpp
Outdated
Show resolved
Hide resolved
| if (false == path_buffer.empty()) { | ||
| path_buffer += "."; | ||
| } | ||
| path_buffer += node.get_key_name(); |
There was a problem hiding this comment.
We need to escape the result of node.get_key_name() here since the key name can contain ".". We should add escaping rules to an official spec for this exposed schema if we haven't already.
| NetworkAuthOption{} | ||
| ); | ||
|
|
||
| auto schema_tree = archive_reader.get_schema_tree(); |
There was a problem hiding this comment.
We might want to close the archive after this point in order to cancel downloads when we're reading archives from s3. Unfortunately it looks like the current implementation will complain if we call close before reading the entire archive, but we could change that behaviour.
Up to you -- don't want to expand the scope of this PR too much.
There was a problem hiding this comment.
What's the problem if we call close before reading the entire archive?
There was a problem hiding this comment.
For now, I think it only targets archives stored in local file system.
There was a problem hiding this comment.
I think the current close implementation for the DictionaryReader's will throw if the dictionary wasn't previously opened. Since we call close on all of the dictionaries in ArchiveReader's close method close will throw if the archive is closed before dictionaries are read. Probably want to change that behaviour since reading only the first few sections of an archive is a valid use-case.
There was a problem hiding this comment.
But yes, if we only have to support the local filesystem flow this is a non-issue for now.
There was a problem hiding this comment.
Yeah, but we call DictionaryReader::open in ArchiveReader::open. I think it's fine if we don't call read_entries?
|
|
||
| auto schema_tree = archive_reader.get_schema_tree(); | ||
| auto field_pairs = traverse_schema_tree(schema_tree); | ||
| if (OutputType::Database == m_output_type) { |
There was a problem hiding this comment.
Do we want to throw on unknown OutputType?
There was a problem hiding this comment.
The constructor should throw an exception
components/core/src/clp_s/metadata_uploader/MySQLTableMetadataDB.hpp
Outdated
Show resolved
Hide resolved
components/core/src/clp_s/metadata_uploader/TableMetadataManager.hpp
Outdated
Show resolved
Hide resolved
components/core/src/clp_s/metadata_uploader/MySQLTableMetadataDB.hpp
Outdated
Show resolved
Hide resolved
Yes, I can add more details to the class description. I agree that the current name isn't ideal, but I haven't come up with a better alternative yet. Do you have any ideas to name the class (and also the program name)? @kirkrodrigues |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
components/core/src/clp_s/metadata_uploader/MySQLTableMetadataDB.hpp (2)
32-48: Ensure consistent member initialization style.Some members use uniform initialization
{}while others don't. For consistency and to prevent potential issues:
- Use uniform initialization for all string members
- Add validation for the port parameter
Apply this diff:
MySQLTableMetadataDB( std::string const& host, int port, std::string const& username, std::string const& password, std::string const& database_name, std::string const& table_prefix ) : m_is_open(false), m_is_init(false), m_host(host), m_port(port), m_username(username), m_password(password), m_database_name(database_name), m_table_prefix(table_prefix) { + if (port <= 0 || port > 65535) { + throw std::invalid_argument("Invalid port number"); + } } // ... std::string m_host{}; int m_port{}; - std::string m_username; - std::string m_password; - std::string m_database_name; - std::string m_table_prefix; + std::string m_username{}; + std::string m_password{}; + std::string m_database_name{}; + std::string m_table_prefix{};Also applies to: 78-83
73-87: Document class invariants for private members.Consider adding documentation that describes the relationships and invariants between private members, especially regarding the connection state flags and prepared statement.
Example documentation to add above the private section:
private: // Class invariants: // - m_is_open indicates if m_db has an active connection // - m_is_init indicates if m_insert_field_statement is prepared // - m_insert_field_statement is only valid when m_is_init is true
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
components/core/src/clp_s/metadata_uploader/CommandLineArguments.hpp(1 hunks)components/core/src/clp_s/metadata_uploader/MySQLTableMetadataDB.hpp(1 hunks)components/core/src/clp_s/metadata_uploader/TableMetadataManager.cpp(1 hunks)components/core/src/clp_s/metadata_uploader/TableMetadataManager.hpp(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- components/core/src/clp_s/metadata_uploader/TableMetadataManager.cpp
- components/core/src/clp_s/metadata_uploader/TableMetadataManager.hpp
- components/core/src/clp_s/metadata_uploader/CommandLineArguments.hpp
🧰 Additional context used
📓 Path-based instructions (1)
components/core/src/clp_s/metadata_uploader/MySQLTableMetadataDB.hpp (1)
Pattern **/*.{cpp,hpp,java,js,jsx,ts,tsx}: - Prefer false == <expression> rather than !<expression>.
🪛 GitHub Actions: clp-lint
components/core/src/clp_s/metadata_uploader/MySQLTableMetadataDB.hpp
[error] 66-69: Code formatting violation detected in documentation comments. Comments need to be formatted according to clang-format rules.
⏰ Context from checks skipped due to timeout of 90000ms (10)
- GitHub Check: ubuntu-jammy-static-linked-bins
- GitHub Check: ubuntu-focal-static-linked-bins
- GitHub Check: ubuntu-jammy-dynamic-linked-bins
- GitHub Check: ubuntu-focal-dynamic-linked-bins
- GitHub Check: centos-stream-9-static-linked-bins
- GitHub Check: centos-stream-9-dynamic-linked-bins
- GitHub Check: build-macos (macos-14, false)
- GitHub Check: build-macos (macos-14, true)
- GitHub Check: build-macos (macos-13, false)
- GitHub Check: build-macos (macos-13, true)
🔇 Additional comments (1)
components/core/src/clp_s/metadata_uploader/MySQLTableMetadataDB.hpp (1)
1-11: LGTM! Header organization is clean and follows best practices.The header guards and includes are well-organized, with appropriate using declarations.
| /** | ||
| * Class representing a MySQL table metadata database | ||
| */ |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance class documentation to better describe its purpose and usage.
Given this class's role in populating column names and types for MySQL tables (as per PR objectives), the documentation should include:
- Purpose and responsibilities
- Relationship with PrestoDB metadata resolution
- Usage examples or typical workflow
Example documentation:
/**
* Class representing a MySQL table metadata database that manages column names
* and types for each table. This metadata is used by SQL engines (e.g., PrestoDB)
* to resolve table schemas effectively.
*
* Usage:
* ```
* MySQLTableMetadataDB db("localhost", 3306, "user", "pass", "db", "prefix_");
* db.open();
* db.init("table_name");
* db.add_field("column1", NodeType::String);
* ```
*/| /** | ||
| * Adds a field to the table | ||
| * @param field_name | ||
| * @param field_type | ||
| */ | ||
| void add_field(std::string const& field_name, NodeType field_type); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Fix documentation formatting and enhance parameter descriptions.
The documentation needs formatting fixes to comply with clang-format rules and could benefit from more detailed parameter descriptions.
Apply this diff:
/**
- * Adds a field to the table
- * @param field_name
- * @param field_type
- */
+ * Adds a field (column) to the table's metadata
+ * @param field_name Name of the column in the table
+ * @param field_type Data type of the column (e.g., String, Integer)
+ */📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| /** | |
| * Adds a field to the table | |
| * @param field_name | |
| * @param field_type | |
| */ | |
| void add_field(std::string const& field_name, NodeType field_type); | |
| /** | |
| * Adds a field (column) to the table's metadata | |
| * @param field_name Name of the column in the table | |
| * @param field_type Data type of the column (e.g., String, Integer) | |
| */ | |
| void add_field(std::string const& field_name, NodeType field_type); |
🧰 Tools
🪛 GitHub Actions: clp-lint
[error] 66-69: Code formatting violation detected in documentation comments. Comments need to be formatted according to clang-format rules.
How about |
Thanks. What about the name of the db class? @kirkrodrigues. I'm also a bit concerned if we add index to clp-s later. |
How about
What's the concern? If it's that we might have a local index later in clp-s, we can revisit the names then. Shouldn't be an issue to rename the classes. |
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Nitpick comments (5)
components/core/src/clp_s/indexer/IndexManager.cpp (1)
76-76: Follow coding guidelines by preferring 'false == ' over '!'In line 76, the condition in the
whileloop uses!s.empty(). To adhere to the coding guidelines, please usefalse == s.empty()instead of!s.empty().Apply this diff to align with the coding standards:
- while (!s.empty()) { + while (false == s.empty()) {components/core/src/clp_s/indexer/CommandLineArguments.hpp (3)
10-12: Enhance class documentation with more details.The current documentation is too brief. Consider adding:
- Expected command-line arguments and their format
- Example usage
- Description of parsing behaviour and validation rules
26-26: Add documentation for parse_arguments method.Document:
- Expected format of arguments
- Return value conditions
- Possible exceptions
23-23: Consider adding move semantics support.Add move constructor and move assignment operator to optimize string handling:
+ CommandLineArguments(CommandLineArguments&&) noexcept = default; + CommandLineArguments& operator=(CommandLineArguments&&) noexcept = default;components/core/src/clp_s/indexer/CMakeLists.txt (1)
1-79: Consider reorganizing source files and reducing dependencies.A few concerns with the current source files organization:
- Duplicate file names exist in different paths (e.g., FileReader.cpp/hpp in both ../../clp/ and ../)
- The inclusion of AWS-related files suggests tight coupling with AWS services, which might not be necessary for basic indexing functionality.
Consider:
- Resolving the duplicate file names by consolidating them into a single location
- Moving AWS-related functionality into a separate module that can be optionally included
- Organizing files into logical subdirectories based on functionality
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
components/core/src/clp_s/CMakeLists.txt(1 hunks)components/core/src/clp_s/indexer/CMakeLists.txt(1 hunks)components/core/src/clp_s/indexer/CommandLineArguments.cpp(1 hunks)components/core/src/clp_s/indexer/CommandLineArguments.hpp(1 hunks)components/core/src/clp_s/indexer/IndexManager.cpp(1 hunks)components/core/src/clp_s/indexer/IndexManager.hpp(1 hunks)components/core/src/clp_s/indexer/MySQLIndexStorage.cpp(1 hunks)components/core/src/clp_s/indexer/MySQLIndexStorage.hpp(1 hunks)components/core/src/clp_s/indexer/indexer.cpp(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- components/core/src/clp_s/CMakeLists.txt
🧰 Additional context used
📓 Path-based instructions (7)
components/core/src/clp_s/indexer/indexer.cpp (1)
Pattern **/*.{cpp,hpp,java,js,jsx,ts,tsx}: - Prefer false == <expression> rather than !<expression>.
components/core/src/clp_s/indexer/CommandLineArguments.cpp (1)
Pattern **/*.{cpp,hpp,java,js,jsx,ts,tsx}: - Prefer false == <expression> rather than !<expression>.
components/core/src/clp_s/indexer/IndexManager.hpp (1)
Pattern **/*.{cpp,hpp,java,js,jsx,ts,tsx}: - Prefer false == <expression> rather than !<expression>.
components/core/src/clp_s/indexer/CommandLineArguments.hpp (1)
Pattern **/*.{cpp,hpp,java,js,jsx,ts,tsx}: - Prefer false == <expression> rather than !<expression>.
components/core/src/clp_s/indexer/IndexManager.cpp (1)
Pattern **/*.{cpp,hpp,java,js,jsx,ts,tsx}: - Prefer false == <expression> rather than !<expression>.
components/core/src/clp_s/indexer/MySQLIndexStorage.cpp (1)
Pattern **/*.{cpp,hpp,java,js,jsx,ts,tsx}: - Prefer false == <expression> rather than !<expression>.
components/core/src/clp_s/indexer/MySQLIndexStorage.hpp (1)
Pattern **/*.{cpp,hpp,java,js,jsx,ts,tsx}: - Prefer false == <expression> rather than !<expression>.
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: build-macos (macos-14, false)
- GitHub Check: build-macos (macos-14, true)
- GitHub Check: build-macos (macos-13, false)
- GitHub Check: build-macos (macos-13, true)
- GitHub Check: lint-check (macos-latest)
🔇 Additional comments (4)
components/core/src/clp_s/indexer/CommandLineArguments.cpp (1)
11-116: LGTMThe command-line argument parsing and error handling are implemented correctly. The code adheres to the coding guidelines.
components/core/src/clp_s/indexer/CMakeLists.txt (3)
81-83: LGTM! Good use of modern C++ standards.The executable configuration is well-structured and follows CMake best practices.
97-102: LGTM! Clear and conventional output directory configuration.The configuration follows CMake best practices and is well-documented.
84-96: Add version constraints for external dependencies.While the library dependencies are comprehensive, they lack version constraints which could lead to compatibility issues.
Run this script to check the minimum required versions of the dependencies:
| m_db.execute_query(fmt::format( | ||
| "CREATE TABLE IF NOT EXISTS {}{} (" | ||
| "name VARCHAR(512) NOT NULL, " | ||
| "type BIGINT NOT NULL," | ||
| "PRIMARY KEY (name, type)" | ||
| ")", | ||
| m_table_prefix, | ||
| table_name | ||
| )); |
There was a problem hiding this comment.
Mitigate potential SQL injection vulnerabilities in dynamic SQL statements
The init() method constructs SQL queries by directly interpolating m_table_prefix and table_name into the SQL statement using fmt::format. If these variables are derived from untrusted sources, this could introduce SQL injection vulnerabilities. Although parameterized queries do not support table names, it's important to validate or sanitize m_table_prefix and table_name to ensure they contain only expected characters (e.g., alphanumeric characters and underscores).
Consider adding validation for table_name and m_table_prefix. For example:
+ #include <regex>
...
+ // Validate table name
+ if (!std::regex_match(table_name, std::regex("^[A-Za-z_][A-Za-z0-9_]*$"))) {
+ throw OperationFailed(ErrorCodeBadParam, __FILENAME__, __LINE__);
+ }Committable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
I think for our purposes the table prefix/table name are trusted input, but maybe we should create a github issue to systematically sanitize input for our sql queries? There are a few places in the codebase where we dump trusted data like table names into sql queries without sanitizing it, but it could lead to tricky bugs so its probably worth sanitizing anyway.
There was a problem hiding this comment.
@gibber9809 You're right about the current inputs being trusted. I've created GitHub issue #688 to track the systematic implementation of SQL input sanitization across the codebase. This will help prevent potential bugs and make the code more robust for future changes.
🔗 Analysis chain
@gibber9809 You're right about the current inputs being trusted. I've created a GitHub issue to track the systematic implementation of SQL input sanitization across the codebase. This will help prevent potential bugs and make the code more robust for future changes.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Create a GitHub issue for SQL input sanitization
gh issue create \
--title "Systematically sanitize SQL inputs across the codebase" \
--body "## Background
Currently, there are several places in the codebase where trusted data (like table names) is directly interpolated into SQL queries without sanitization. While these inputs are currently from trusted sources, this practice could lead to bugs and potential security vulnerabilities if the trust assumptions change in the future.
## Objectives
1. Review all SQL query construction points in the codebase
2. Implement proper input sanitization for:
- Table names
- Column names
- Other SQL identifiers
3. Consider creating utility functions for SQL identifier sanitization
## References
- Discussion: ${GITHUB_SERVER_URL}/${GITHUB_REPOSITORY}/pull/672#discussion_r1923866363
## Impact
This improvement will:
- Prevent potential bugs from malformed identifiers
- Add an extra layer of safety against SQL injection
- Make the code more robust against future changes in trust assumptions
/cc @gibber9809" \
--assignee "gibber9809"
Length of output: 1044
✏️ Learnings added
Learnt from: gibber9809
PR: y-scope/clp#672
File: components/core/src/clp_s/indexer/MySQLIndexStorage.cpp:30-38
Timestamp: 2025-01-23T17:08:55.419Z
Learning: SQL identifiers (table names, column names) in the codebase are currently from trusted sources and directly interpolated into queries. However, the team prefers to implement sanitization for robustness, even for trusted inputs.
Learnt from: gibber9809
PR: y-scope/clp#672
File: components/core/src/clp_s/indexer/MySQLIndexStorage.cpp:30-38
Timestamp: 2025-01-23T17:08:55.419Z
Learning: SQL identifiers (table names, column names) in the codebase are currently from trusted sources and directly interpolated into queries. However, the team prefers to implement sanitization for robustness, even for trusted inputs.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
| std::string m_host; | ||
| int m_port{}; | ||
| std::string m_username; | ||
| std::string m_password; |
There was a problem hiding this comment.
Security: Avoid storing database password as plain string.
The password is currently stored as a plain string member variable. Consider:
- Using a secure string class that zeros memory on destruction
- Clearing the password after connection is established
| * Creates the table if it does not exist and prepares the insert statement | ||
| * @param table_name | ||
| */ | ||
| void init(std::string const& table_name); |
There was a problem hiding this comment.
Security: Add input validation for table_name parameter.
The init method should validate the table_name parameter to prevent SQL injection:
- Validate characters (alphanumeric and underscores only)
- Check length limits
- Consider using prepared statements for table creation
| class MySQLIndexStorage { | ||
| public: | ||
| static constexpr char cColumnMetadataPrefix[] = "column_metadata_"; | ||
|
|
||
| // Types | ||
| class OperationFailed : public TraceableException { | ||
| public: | ||
| // Constructors | ||
| OperationFailed(ErrorCode error_code, char const* const filename, int line_number) | ||
| : TraceableException(error_code, filename, line_number) {} | ||
|
|
||
| // Methods | ||
| [[nodiscard]] char const* what() const noexcept override { | ||
| return "MySQLIndexStorage operation failed"; | ||
| } | ||
| }; | ||
|
|
||
| // Constructors | ||
| MySQLIndexStorage( | ||
| std::string const& host, | ||
| int port, | ||
| std::string const& username, | ||
| std::string const& password, | ||
| std::string const& database_name, | ||
| std::string const& table_prefix | ||
| ) | ||
| : m_is_open(false), | ||
| m_is_init(false), | ||
| m_host(host), | ||
| m_port(port), | ||
| m_username(username), | ||
| m_password(password), | ||
| m_database_name(database_name), | ||
| m_table_prefix(table_prefix + cColumnMetadataPrefix) {} | ||
|
|
||
| // Methods | ||
| /** | ||
| * Opens the database connection | ||
| */ | ||
| void open(); | ||
|
|
||
| /** | ||
| * Creates the table if it does not exist and prepares the insert statement | ||
| * @param table_name | ||
| */ | ||
| void init(std::string const& table_name); | ||
|
|
||
| /** | ||
| * Closes the database connection | ||
| */ | ||
| void close(); | ||
|
|
||
| /** | ||
| * Adds a field (column) to the table | ||
| * @param field_name | ||
| * @param field_type | ||
| */ | ||
| void add_field(std::string const& field_name, NodeType field_type); | ||
|
|
||
| private: | ||
| // Variables | ||
| bool m_is_open{}; | ||
| bool m_is_init{}; | ||
| std::string m_host; | ||
| int m_port{}; | ||
| std::string m_username; | ||
| std::string m_password; | ||
| std::string m_database_name; | ||
| std::string m_table_prefix; | ||
|
|
||
| MySQLDB m_db; | ||
|
|
||
| std::unique_ptr<MySQLPreparedStatement> m_insert_field_statement; | ||
| }; |
There was a problem hiding this comment.
💡 Codebase verification
Document thread safety limitations: Not thread-safe
This class maintains mutable state and manages database connections without synchronization mechanisms. Since it's stored as a std::shared_ptr in IndexManager, concurrent access is possible. Please add documentation to specify:
- The class is not thread-safe
- Concurrent calls to
open(),init(),add_field(), andclose()must be externally synchronized - Each instance should be accessed by a single thread at a time
🔗 Analysis chain
Document thread safety guarantees.
The class handles database connections and prepared statements but doesn't specify thread safety guarantees. Please document:
- Whether methods are thread-safe
- Any synchronization requirements
- Concurrent access limitations
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for concurrent usage of MySQLIndexStorage
rg -A 5 "MySQLIndexStorage.*thread|std::mutex.*MySQLIndexStorage"
Length of output: 67
Script:
#!/bin/bash
# Check MySQLDB implementation and documentation
ast-grep --pattern 'class MySQLDB {
$$$
}'
# Look for documentation mentioning thread safety
rg -i "thread.*safe|concurrent|synchroniz" --type cpp
# Check how MySQLIndexStorage is used
rg "MySQLIndexStorage" -B 2 -A 2
Length of output: 5414
| class OperationFailed : public TraceableException { | ||
| public: | ||
| // Constructors | ||
| OperationFailed(ErrorCode error_code, char const* const filename, int line_number) | ||
| : TraceableException(error_code, filename, line_number) {} | ||
|
|
||
| // Methods | ||
| [[nodiscard]] char const* what() const noexcept override { | ||
| return "IndexManager operation failed"; | ||
| } | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add virtual destructor to exception class.
The OperationFailed exception class should have a virtual destructor since it inherits from TraceableException:
public:
// Constructors
OperationFailed(ErrorCode error_code, char const* const filename, int line_number)
: TraceableException(error_code, filename, line_number) {}
+ // Destructor
+ ~OperationFailed() override = default;
+
// Methods
[[nodiscard]] char const* what() const noexcept override {
return "IndexManager operation failed";
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| class OperationFailed : public TraceableException { | |
| public: | |
| // Constructors | |
| OperationFailed(ErrorCode error_code, char const* const filename, int line_number) | |
| : TraceableException(error_code, filename, line_number) {} | |
| // Methods | |
| [[nodiscard]] char const* what() const noexcept override { | |
| return "IndexManager operation failed"; | |
| } | |
| }; | |
| class OperationFailed : public TraceableException { | |
| public: | |
| // Constructors | |
| OperationFailed(ErrorCode error_code, char const* const filename, int line_number) | |
| : TraceableException(error_code, filename, line_number) {} | |
| // Destructor | |
| ~OperationFailed() override = default; | |
| // Methods | |
| [[nodiscard]] char const* what() const noexcept override { | |
| return "IndexManager operation failed"; | |
| } | |
| }; |
| try { | ||
| auto stderr_logger = spdlog::stderr_logger_st("stderr"); | ||
| spdlog::set_default_logger(stderr_logger); | ||
| spdlog::set_pattern("%Y-%m-%dT%H:%M:%S.%e%z [%l] %v"); | ||
| } catch (std::exception& e) { | ||
| return 1; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve error handling for logger initialization.
The current implementation silently fails if logger initialization fails. Consider:
- Adding error message to stderr
- Using RAII for logger management
try {
auto stderr_logger = spdlog::stderr_logger_st("stderr");
spdlog::set_default_logger(stderr_logger);
spdlog::set_pattern("%Y-%m-%dT%H:%M:%S.%e%z [%l] %v");
} catch (std::exception& e) {
+ std::cerr << "Failed to initialize logger: " << e.what() << std::endl;
return 1;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| try { | |
| auto stderr_logger = spdlog::stderr_logger_st("stderr"); | |
| spdlog::set_default_logger(stderr_logger); | |
| spdlog::set_pattern("%Y-%m-%dT%H:%M:%S.%e%z [%l] %v"); | |
| } catch (std::exception& e) { | |
| return 1; | |
| } | |
| try { | |
| auto stderr_logger = spdlog::stderr_logger_st("stderr"); | |
| spdlog::set_default_logger(stderr_logger); | |
| spdlog::set_pattern("%Y-%m-%dT%H:%M:%S.%e%z [%l] %v"); | |
| } catch (std::exception& e) { | |
| std::cerr << "Failed to initialize logger: " << e.what() << std::endl; | |
| return 1; | |
| } |
| switch (parsing_result) { | ||
| case CommandLineArguments::ParsingResult::Failure: | ||
| return 1; | ||
| case CommandLineArguments::ParsingResult::InfoCommand: | ||
| return 0; | ||
| case CommandLineArguments::ParsingResult::Success: | ||
| // Continue processing | ||
| break; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add logging for command line parsing failures.
The switch statement silently returns on parsing failure. Add logging to help diagnose issues:
switch (parsing_result) {
case CommandLineArguments::ParsingResult::Failure:
+ SPDLOG_ERROR("Failed to parse command line arguments");
return 1;
case CommandLineArguments::ParsingResult::InfoCommand:
return 0;
case CommandLineArguments::ParsingResult::Success:
// Continue processing
break;
}Committable suggestion skipped: line range outside the PR's diff.
| int main(int argc, char const* argv[]) { | ||
| try { | ||
| auto stderr_logger = spdlog::stderr_logger_st("stderr"); | ||
| spdlog::set_default_logger(stderr_logger); | ||
| spdlog::set_pattern("%Y-%m-%dT%H:%M:%S.%e%z [%l] %v"); | ||
| } catch (std::exception& e) { | ||
| return 1; | ||
| } | ||
|
|
||
| CommandLineArguments command_line_arguments("metadata-uploader"); | ||
| auto parsing_result = command_line_arguments.parse_arguments(argc, argv); | ||
| switch (parsing_result) { | ||
| case CommandLineArguments::ParsingResult::Failure: | ||
| return 1; | ||
| case CommandLineArguments::ParsingResult::InfoCommand: | ||
| return 0; | ||
| case CommandLineArguments::ParsingResult::Success: | ||
| // Continue processing | ||
| break; | ||
| } | ||
|
|
||
| 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() | ||
| ); | ||
| } catch (std::exception& e) { | ||
| SPDLOG_ERROR("Failed to update metadata: {}", e.what()); | ||
| return 1; | ||
| } | ||
| } |
There was a problem hiding this comment.
Add return value to indicate successful execution.
The main function should return 0 on successful execution:
} catch (std::exception& e) {
SPDLOG_ERROR("Failed to update metadata: {}", e.what());
return 1;
}
+ return 0;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| int main(int argc, char const* argv[]) { | |
| try { | |
| auto stderr_logger = spdlog::stderr_logger_st("stderr"); | |
| spdlog::set_default_logger(stderr_logger); | |
| spdlog::set_pattern("%Y-%m-%dT%H:%M:%S.%e%z [%l] %v"); | |
| } catch (std::exception& e) { | |
| return 1; | |
| } | |
| CommandLineArguments command_line_arguments("metadata-uploader"); | |
| auto parsing_result = command_line_arguments.parse_arguments(argc, argv); | |
| switch (parsing_result) { | |
| case CommandLineArguments::ParsingResult::Failure: | |
| return 1; | |
| case CommandLineArguments::ParsingResult::InfoCommand: | |
| return 0; | |
| case CommandLineArguments::ParsingResult::Success: | |
| // Continue processing | |
| break; | |
| } | |
| 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() | |
| ); | |
| } catch (std::exception& e) { | |
| SPDLOG_ERROR("Failed to update metadata: {}", e.what()); | |
| return 1; | |
| } | |
| } | |
| int main(int argc, char const* argv[]) { | |
| try { | |
| auto stderr_logger = spdlog::stderr_logger_st("stderr"); | |
| spdlog::set_default_logger(stderr_logger); | |
| spdlog::set_pattern("%Y-%m-%dT%H:%M:%S.%e%z [%l] %v"); | |
| } catch (std::exception& e) { | |
| return 1; | |
| } | |
| CommandLineArguments command_line_arguments("metadata-uploader"); | |
| auto parsing_result = command_line_arguments.parse_arguments(argc, argv); | |
| switch (parsing_result) { | |
| case CommandLineArguments::ParsingResult::Failure: | |
| return 1; | |
| case CommandLineArguments::ParsingResult::InfoCommand: | |
| return 0; | |
| case CommandLineArguments::ParsingResult::Success: | |
| // Continue processing | |
| break; | |
| } | |
| 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() | |
| ); | |
| } catch (std::exception& e) { | |
| SPDLOG_ERROR("Failed to update metadata: {}", e.what()); | |
| return 1; | |
| } | |
| return 0; | |
| } |
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (9)
components/core/src/clp_s/indexer/IndexManager.hpp (7)
9-15: Enhance class documentation.Consider adding:
- @SInCE tag for version tracking
- List of supported SQL engines (e.g., PrestoDB)
- Example of how field names are mapped to column names
31-33: Document OutputType enum and consider future extensibility.Add documentation for the enum class explaining:
- Purpose of each value
- When to use different output types
- Plans for future output types (if any)
35-37: Document constructor parameters and behaviour.Add documentation for the constructor explaining:
- Purpose of the optional db_config parameter
- Default behaviour when db_config is not provided
- Any exceptions that might be thrown
42-47: Enhance update_metadata documentation.Add documentation for:
- @throws section listing possible exceptions
- Parameter constraints (e.g., format requirements for archive_dir and archive_id)
- Side effects and state changes
- Performance characteristics
55-55: Consider moving escape_key_name to a utility class.This static method could be useful in other contexts. Consider moving it to a utility class for better code organization and reusability.
62-64: Use a struct instead of std::pair for schema tree traversal results.Replace the pair with a named struct to improve code readability and maintainability:
struct SchemaNode { std::string path_name; clp_s::NodeType type; };Then update the return type:
std::vector<SchemaNode> traverse_schema_tree(std::shared_ptr<SchemaTree> const& schema_tree);
66-66: Rename member variable to match class naming.The member variable
m_table_metadata_dbshould be renamed tom_index_storageto maintain consistency with the class name change from TableMetadata to Index.components/core/src/clp_s/indexer/IndexManager.cpp (2)
26-30: Add error handling for database closure.The destructor should handle potential exceptions during database closure to prevent termination.
IndexManager::~IndexManager() { if (m_output_type == OutputType::Database) { - m_table_metadata_db->close(); + try { + m_table_metadata_db->close(); + } catch (...) { + // Log error but don't throw from destructor + } } }
56-101: Optimize string escaping implementation.Consider these improvements:
- Adjust reserve size to account for escape sequences
- Add input validation
- Use a lookup table for better performance with common escape sequences
std::string IndexManager::escape_key_name(std::string_view const key_name) { + if (key_name.empty()) { + return {}; + } + std::string escaped_key_name; - escaped_key_name.reserve(key_name.size()); + // Reserve twice the size to accommodate potential escape sequences + escaped_key_name.reserve(key_name.size() * 2); + + // Static lookup table for common escape sequences + static const std::unordered_map<char, std::string_view> escape_sequences{ + {'\"', "\\\""}, {'\\', "\\\\"}, {'\n', "\\n"}, + {'\t', "\\t"}, {'\r', "\\r"}, {'\b', "\\b"}, + {'\f', "\\f"}, {'.', "\\."} + }; for (auto c : key_name) { - switch (c) { - case '\"': - escaped_key_name += "\\\""; - break; - // ... existing cases ... - default: - if (std::isprint(c)) { - escaped_key_name += c; - } else { - char buffer[7]; - std::snprintf(buffer, sizeof(buffer), "\\u00%02x", - static_cast<unsigned char>(c)); - escaped_key_name += buffer; - } + if (auto it = escape_sequences.find(c); it != escape_sequences.end()) { + escaped_key_name += it->second; + } else if (std::isprint(c)) { + escaped_key_name += c; + } else { + char buffer[7]; + std::snprintf(buffer, sizeof(buffer), "\\u00%02x", + static_cast<unsigned char>(c)); + escaped_key_name += buffer; } } return escaped_key_name; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
components/core/src/clp_s/indexer/IndexManager.cpp(1 hunks)components/core/src/clp_s/indexer/IndexManager.hpp(1 hunks)components/core/src/clp_s/indexer/MySQLIndexStorage.hpp(1 hunks)components/core/src/clp_s/indexer/indexer.cpp(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- components/core/src/clp_s/indexer/indexer.cpp
- components/core/src/clp_s/indexer/MySQLIndexStorage.hpp
🧰 Additional context used
📓 Path-based instructions (2)
components/core/src/clp_s/indexer/IndexManager.hpp (1)
Pattern **/*.{cpp,hpp,java,js,jsx,ts,tsx}: - Prefer false == <expression> rather than !<expression>.
components/core/src/clp_s/indexer/IndexManager.cpp (1)
Pattern **/*.{cpp,hpp,java,js,jsx,ts,tsx}: - Prefer false == <expression> rather than !<expression>.
⏰ Context from checks skipped due to timeout of 90000ms (12)
- GitHub Check: ubuntu-focal-static-linked-bins
- GitHub Check: centos-stream-9-static-linked-bins
- GitHub Check: ubuntu-focal-dynamic-linked-bins
- GitHub Check: ubuntu-jammy-static-linked-bins
- GitHub Check: centos-stream-9-dynamic-linked-bins
- GitHub Check: ubuntu-jammy-dynamic-linked-bins
- GitHub Check: build-macos (macos-14, false)
- GitHub Check: build-macos (macos-14, true)
- GitHub Check: build-macos (macos-13, false)
- GitHub Check: lint-check (ubuntu-latest)
- GitHub Check: build (macos-latest)
- GitHub Check: build-macos (macos-13, true)
🔇 Additional comments (1)
components/core/src/clp_s/indexer/IndexManager.hpp (1)
19-29: Add virtual destructor to exception class.The
OperationFailedexception class should have a virtual destructor since it inherits fromTraceableException.
| std::vector<std::pair<std::string, clp_s::NodeType>> IndexManager::traverse_schema_tree( | ||
| std::shared_ptr<SchemaTree> const& schema_tree | ||
| ) { | ||
| std::vector<std::pair<std::string, clp_s::NodeType>> fields; | ||
| if (nullptr == schema_tree) { | ||
| return fields; | ||
| } | ||
|
|
||
| std::string path_buffer; | ||
| // Stack of pairs of node_id and path_length | ||
| std::stack<std::pair<int32_t, uint64_t>> s; | ||
| for (auto& node : schema_tree->get_nodes()) { | ||
| if (constants::cRootNodeId == node.get_parent_id() | ||
| && clp_s::NodeType::Metadata != node.get_type()) | ||
| { | ||
| s.push({node.get_id(), 0}); | ||
| break; | ||
| } | ||
| } | ||
|
|
||
| while (false == s.empty()) { | ||
| auto [node_id, path_length] = s.top(); | ||
| s.pop(); | ||
|
|
||
| auto const& node = schema_tree->get_node(node_id); | ||
| auto& children_ids = node.get_children_ids(); | ||
| auto node_type = node.get_type(); | ||
| 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) | ||
| { | ||
| fields.push_back({path_buffer, node_type}); | ||
| } | ||
|
|
||
| for (auto child_id : children_ids) { | ||
| s.push({child_id, path_buffer.size()}); | ||
| } | ||
| } | ||
|
|
||
| return fields; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve documentation and optimize schema tree traversal.
The method needs several improvements:
- Add documentation explaining the traversal algorithm
- Optimize string operations
- Extract magic numbers into named constants
- Simplify complex conditions
+/**
+ * Traverses the schema tree in depth-first order, collecting field names and types.
+ * For each non-object leaf node, constructs a dot-separated path from root to node
+ * and adds it to the result along with its type.
+ *
+ * @param schema_tree The schema tree to traverse
+ * @return Vector of pairs containing field paths and their types
+ */
std::vector<std::pair<std::string, clp_s::NodeType>> IndexManager::traverse_schema_tree(
std::shared_ptr<SchemaTree> const& schema_tree
) {
std::vector<std::pair<std::string, clp_s::NodeType>> fields;
if (nullptr == schema_tree) {
return fields;
}
+ // Estimate initial path buffer size to reduce reallocations
std::string path_buffer;
+ path_buffer.reserve(256); // Adjust based on typical path length
+
+ static constexpr char PATH_SEPARATOR = '.';
// Stack of pairs of node_id and path_length
std::stack<std::pair<int32_t, uint64_t>> s;
+ // Find the first non-metadata root child
for (auto& node : schema_tree->get_nodes()) {
- if (constants::cRootNodeId == node.get_parent_id()
- && clp_s::NodeType::Metadata != node.get_type())
- {
+ if (is_valid_root_child(node)) {
s.push({node.get_id(), 0});
break;
}
}
while (false == s.empty()) {
auto [node_id, path_length] = s.top();
s.pop();
auto const& node = schema_tree->get_node(node_id);
auto& children_ids = node.get_children_ids();
auto node_type = node.get_type();
path_buffer.resize(path_length);
if (false == path_buffer.empty()) {
- path_buffer += ".";
+ path_buffer += PATH_SEPARATOR;
}
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 (is_valid_leaf_node(node)) {
fields.push_back({path_buffer, node_type});
}
for (auto child_id : children_ids) {
s.push({child_id, path_buffer.size()});
}
}
return fields;
}
+
+/**
+ * Checks if a node is a valid root child (non-metadata node with root parent).
+ */
+bool IndexManager::is_valid_root_child(const SchemaTreeNode& node) const {
+ return constants::cRootNodeId == node.get_parent_id()
+ && clp_s::NodeType::Metadata != node.get_type();
+}
+
+/**
+ * Checks if a node is a valid leaf node (no children and has a known, non-object type).
+ */
+bool IndexManager::is_valid_leaf_node(const SchemaTreeNode& node) const {
+ return node.get_children_ids().empty()
+ && clp_s::NodeType::Object != node.get_type()
+ && clp_s::NodeType::Unknown != node.get_type();
+}Committable suggestion skipped: line range outside the PR's diff.
| IndexManager::IndexManager(std::optional<clp::GlobalMetadataDBConfig> const& db_config) { | ||
| if (db_config.has_value()) { | ||
| m_table_metadata_db = 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_output_type = OutputType::Database; | ||
| } else { | ||
| throw OperationFailed(ErrorCodeBadParam, __FILENAME__, __LINE__); | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling and documentation for database initialization.
The constructor should include:
- Validation of database configuration parameters
- Error handling for database connection failures in
open() - Documentation explaining the purpose of
OutputType::Database
IndexManager::IndexManager(std::optional<clp::GlobalMetadataDBConfig> const& db_config) {
if (db_config.has_value()) {
+ // Validate database configuration parameters
+ if (db_config->get_metadata_db_host().empty() ||
+ db_config->get_metadata_db_username().empty() ||
+ db_config->get_metadata_db_name().empty()) {
+ throw OperationFailed(ErrorCodeBadParam, __FILENAME__, __LINE__);
+ }
+
m_table_metadata_db = 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();
+ try {
+ m_table_metadata_db->open();
+ } catch (const std::exception& e) {
+ throw OperationFailed(ErrorCodeDatabaseError, __FILENAME__, __LINE__);
+ }
m_output_type = OutputType::Database;
} else {
throw OperationFailed(ErrorCodeBadParam, __FILENAME__, __LINE__);
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| IndexManager::IndexManager(std::optional<clp::GlobalMetadataDBConfig> const& db_config) { | |
| if (db_config.has_value()) { | |
| m_table_metadata_db = 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_output_type = OutputType::Database; | |
| } else { | |
| throw OperationFailed(ErrorCodeBadParam, __FILENAME__, __LINE__); | |
| } | |
| } | |
| IndexManager::IndexManager(std::optional<clp::GlobalMetadataDBConfig> const& db_config) { | |
| if (db_config.has_value()) { | |
| // Validate database configuration parameters | |
| if (db_config->get_metadata_db_host().empty() || | |
| db_config->get_metadata_db_username().empty() || | |
| db_config->get_metadata_db_name().empty()) { | |
| throw OperationFailed(ErrorCodeBadParam, __FILENAME__, __LINE__); | |
| } | |
| m_table_metadata_db = 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() | |
| ); | |
| try { | |
| m_table_metadata_db->open(); | |
| } catch (const std::exception& e) { | |
| throw OperationFailed(ErrorCodeDatabaseError, __FILENAME__, __LINE__); | |
| } | |
| m_output_type = OutputType::Database; | |
| } else { | |
| throw OperationFailed(ErrorCodeBadParam, __FILENAME__, __LINE__); | |
| } | |
| } |
| 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__); | ||
| } | ||
|
|
||
| ArchiveReader archive_reader; | ||
| archive_reader.open( | ||
| clp_s::Path{.source = clp_s::InputSource::Filesystem, .path = archive_path.string()}, | ||
| 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); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Add input validation and comprehensive error handling.
The method needs several improvements:
- Input parameter validation
- Error handling for archive operations
- RAII pattern for resource management
void IndexManager::update_metadata(std::string const& archive_dir, std::string const& archive_id) {
+ if (archive_dir.empty() || archive_id.empty()) {
+ throw OperationFailed(ErrorCodeBadParam, __FILENAME__, __LINE__);
+ }
+
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__);
}
- ArchiveReader archive_reader;
- archive_reader.open(
- clp_s::Path{.source = clp_s::InputSource::Filesystem, .path = archive_path.string()},
- NetworkAuthOption{}
- );
+ try {
+ ArchiveReader archive_reader;
+ archive_reader.open(
+ clp_s::Path{.source = clp_s::InputSource::Filesystem, .path = archive_path.string()},
+ 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);
+ auto schema_tree = archive_reader.get_schema_tree();
+ if (nullptr == schema_tree) {
+ throw OperationFailed(ErrorCodeSchemaError, __FILENAME__, __LINE__);
+ }
+
+ 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);
+ }
}
+ } catch (const std::exception& e) {
+ throw OperationFailed(ErrorCodeArchiveError, __FILENAME__, __LINE__);
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| 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__); | |
| } | |
| ArchiveReader archive_reader; | |
| archive_reader.open( | |
| clp_s::Path{.source = clp_s::InputSource::Filesystem, .path = archive_path.string()}, | |
| 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); | |
| } | |
| } | |
| } | |
| void IndexManager::update_metadata(std::string const& archive_dir, std::string const& archive_id) { | |
| if (archive_dir.empty() || archive_id.empty()) { | |
| throw OperationFailed(ErrorCodeBadParam, __FILENAME__, __LINE__); | |
| } | |
| 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__); | |
| } | |
| try { | |
| ArchiveReader archive_reader; | |
| archive_reader.open( | |
| clp_s::Path{.source = clp_s::InputSource::Filesystem, .path = archive_path.string()}, | |
| NetworkAuthOption{} | |
| ); | |
| auto schema_tree = archive_reader.get_schema_tree(); | |
| if (nullptr == schema_tree) { | |
| throw OperationFailed(ErrorCodeSchemaError, __FILENAME__, __LINE__); | |
| } | |
| 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); | |
| } | |
| } | |
| } catch (const std::exception& e) { | |
| throw OperationFailed(ErrorCodeArchiveError, __FILENAME__, __LINE__); | |
| } | |
| } |
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
components/core/src/clp_s/indexer/IndexManager.hpp (1)
19-24:⚠️ Potential issueAdd what() method override and virtual destructor.
The exception class needs:
- A virtual destructor (as previously mentioned)
- An override of the what() method to provide specific error details
Apply this diff:
class OperationFailed : public TraceableException { public: // Constructors OperationFailed(ErrorCode error_code, char const* const filename, int line_number) : TraceableException(error_code, filename, line_number) {} + + // Destructor + ~OperationFailed() override = default; + + // Methods + [[nodiscard]] char const* what() const noexcept override { + return "IndexManager operation failed"; + } };
🧹 Nitpick comments (3)
components/core/src/clp_s/indexer/IndexManager.hpp (3)
9-15: Enhance class documentation for better clarity.Consider adding more details about:
- The relationship with MySQLIndexStorage and database operations
- The purpose and potential future expansion of OutputType
- Error handling expectations and failure scenarios
26-28: Document OutputType enum for future extensibility.Consider adding documentation to explain:
- The purpose of this enum
- Plans for future output types
- Why uint8_t was chosen as the underlying type
37-42: Enhance update_metadata documentation.Please add more details about:
- Expected format and constraints for archive_dir and archive_id
- Possible error conditions and how they're handled
- Side effects on the database
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
components/core/src/clp_s/indexer/IndexManager.cpp(1 hunks)components/core/src/clp_s/indexer/IndexManager.hpp(1 hunks)components/core/src/clp_s/indexer/MySQLIndexStorage.hpp(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- components/core/src/clp_s/indexer/MySQLIndexStorage.hpp
- components/core/src/clp_s/indexer/IndexManager.cpp
🧰 Additional context used
📓 Path-based instructions (1)
components/core/src/clp_s/indexer/IndexManager.hpp (1)
Pattern **/*.{cpp,hpp,java,js,jsx,ts,tsx}: - Prefer false == <expression> rather than !<expression>.
⏰ Context from checks skipped due to timeout of 90000ms (9)
- GitHub Check: ubuntu-focal-static-linked-bins
- GitHub Check: ubuntu-jammy-static-linked-bins
- GitHub Check: ubuntu-focal-dynamic-linked-bins
- GitHub Check: ubuntu-jammy-dynamic-linked-bins
- GitHub Check: centos-stream-9-static-linked-bins
- GitHub Check: centos-stream-9-dynamic-linked-bins
- GitHub Check: build-macos (macos-14, false)
- GitHub Check: build-macos (macos-13, false)
- GitHub Check: build-macos (macos-13, true)
🔇 Additional comments (3)
components/core/src/clp_s/indexer/IndexManager.hpp (3)
1-7: LGTM! Header guards and includes are properly structured.
31-34: LGTM! Constructor and destructor are properly declared.
44-63: LGTM! Private section is well-structured and documented.
| 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; | ||
| } | ||
| } |
There was a problem hiding this comment.
Sorry, just remembered this, but I actually added a get_object_subtree_node_id method to SchemaTree. Maybe we could do s.emplace(schema_tree->get_object_subtree_root_node_id(), 0) instead of this loop?
| m_db.execute_query(fmt::format( | ||
| "CREATE TABLE IF NOT EXISTS {}{} (" | ||
| "name VARCHAR(512) NOT NULL, " | ||
| "type BIGINT NOT NULL," | ||
| "PRIMARY KEY (name, type)" | ||
| ")", | ||
| m_table_prefix, | ||
| table_name | ||
| )); |
There was a problem hiding this comment.
I think for our purposes the table prefix/table name are trusted input, but maybe we should create a github issue to systematically sanitize input for our sql queries? There are a few places in the codebase where we dump trusted data like table names into sql queries without sanitizing it, but it could lead to tricky bugs so its probably worth sanitizing anyway.
| throw OperationFailed(ErrorCodeNotReady, __FILENAME__, __LINE__); | ||
| } | ||
|
|
||
| m_db.execute_query(fmt::format( |
There was a problem hiding this comment.
Are we sure that we want to create this table from c++ code? I think most of our other table creation statements live in python scripts. It's probably fine, but I just want to make sure the rationale is solid.
There was a problem hiding this comment.
A good question. I did see CREATE TABLE in clp and glt code. Maybe let's ask @kirkrodrigues
| std::vector<std::pair<std::string, clp_s::NodeType>> IndexManager::traverse_schema_tree( | ||
| std::shared_ptr<SchemaTree> const& schema_tree | ||
| ) { | ||
| std::vector<std::pair<std::string, clp_s::NodeType>> fields; |
There was a problem hiding this comment.
I'm a bit worried about the memory usage of this approach for large schema trees (as compared to inserting each field into an index once we hit a leaf), but I guess we can change the approach later if it becomes an issue.
Also there are a few edge cases that might be worth documenting in a comment. Specifically, since we only index leaf nodes we can miss indexing an empty object if it sometimes contains children.
For example
{"a": {}}
{"a": {"b": 0}}
Will only index "a.b" : NodeType::Integer.
| path_buffer += "."; | ||
| } | ||
| path_buffer += escape_key_name(node.get_key_name()); | ||
| if (children_ids.empty() && clp_s::NodeType::Object != node_type |
There was a problem hiding this comment.
Need to add a case for structured arrays, since structured arrays can have children but should probably be treated as leaf nodes for the purposes of indexing.
Something like
if (clp_s::NodeType::StructuredArray == node_type) {
s.emplace_back(path_buffer, NodeType::UnstructuredArray);
continue;
} else if (children_ids.empty() && ...)) {
s.emplace...;
continue;
}
Assuming that NodeType::UnstructuredArray is what we're treating as the fundamental array type in the index data.
Description
This PR introduces an indexer process to clp-s that populates column names and types for each table in MySQL. It enables SQL engines (e.g., PrestoDB) to resolve metadata effectively. The indexer will be integrated into the clp-package in a future update.
Validation performed
Summary by CodeRabbit
New Features
New Components
IndexManagerfor handling metadata updates.MySQLIndexStoragefor database interactions.CommandLineArgumentsfor processing application inputs.Infrastructure