feat(clp-s): Add command line options for stubbed out kv-pair-IR ingestion. #618
Conversation
WalkthroughThe changes in this pull request introduce a new command-line option Changes
Possibly related PRs
Suggested reviewers
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: 2
🧹 Outside diff range and nitpick comments (3)
components/core/src/clp_s/clp-s.cpp (1)
131-171: Consider enhancing error handling in database configuration.While the function effectively consolidates common compression setup logic, the database configuration section could benefit from additional error handling.
Consider wrapping the database configuration in a try-catch block:
if (db_config_container.has_value()) { + try { auto const& db_config = db_config_container.value(); option.metadata_db = std::make_shared<clp::GlobalMySQLMetadataDB>( 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() ); + } catch (const std::exception& e) { + SPDLOG_ERROR("Failed to configure metadata database - {}", e.what()); + return false; + } }components/core/src/clp_s/CommandLineArguments.cpp (1)
957-959: Consolidate usage printing functionsThe
print_ir_compression_usagefunction added in lines 957-959 is very similar toprint_compression_usage. It would be beneficial to parameterize these functions or create a common function to avoid code duplication.components/core/src/clp_s/CommandLineArguments.hpp (1)
29-30: Ensure consistency in enum documentationTo maintain consistency, consider adding documentation or comments for the new
IrCompresscommand in theCommandenum, similar to the existing commands.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
components/core/src/clp_s/CommandLineArguments.cpp(4 hunks)components/core/src/clp_s/CommandLineArguments.hpp(2 hunks)components/core/src/clp_s/clp-s.cpp(3 hunks)
🔇 Additional comments (2)
components/core/src/clp_s/clp-s.cpp (2)
53-58: LGTM! Function declaration is well-documented.
The declaration follows consistent style and includes proper documentation.
355-358: LGTM! Command handling is properly integrated.
The new command case follows the existing pattern and error handling approach consistently.
There was a problem hiding this comment.
Changing the approach to just adding a new file-type option to the c flow per side-discussion.
IR ingestion supports almost all of the same options now (except for --structurize-arrays), so it will be simpler and more maintainable to have IR and jsonl ingestion under the same top-level flow.
In the future we also plan to detect file-type automatically, at which point IR and jsonl ingestion will need to be under the same flow, and the --file-type option will no longer be required.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
components/core/src/clp_s/clp-s.cpp (2)
75-79: Enhance file type validation with enum and improved error messagingConsider using an enum class for file types instead of string literals to prevent typos and improve type safety. Also, enhance the error message to list valid options.
+enum class FileType { + IR, + Json +}; + +static auto string_to_file_type(std::string const& type) -> std::optional<FileType> { + if (type == "IR") return FileType::IR; + if (type == "Json") return FileType::Json; + return std::nullopt; +} + auto file_type = command_line_arguments.get_file_type(); -if ("IR" != file_type && "Json" != file_type) { - SPDLOG_ERROR("File Type specified is Invalid"); +auto parsed_type = string_to_file_type(file_type); +if (!parsed_type) { + SPDLOG_ERROR("Invalid file type '{}'. Valid options are: IR, Json", file_type); return false; }
80-83: Consider moving IR file type validation to CommandLineArgumentsThe validation of incompatible options should ideally be performed during argument parsing in the CommandLineArguments class. This would catch invalid combinations earlier and maintain better separation of concerns.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
components/core/src/clp_s/clp-s.cpp(2 hunks)
🔇 Additional comments (1)
components/core/src/clp_s/clp-s.cpp (1)
126-134:
Adjust interim behaviour until implementation is complete
The function currently succeeds silently without performing any actual IR file processing, which could be misleading to callers.
if ("IR" == file_type) {
- // Functionality Coming in later PR
- // -->Call new parsing function in Json Parser to parse IRv2 to archive
- // -->Check for error from parsing function
+ SPDLOG_ERROR("IR file processing not yet implemented");
+ return false;
} else {
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
components/core/src/clp_s/CommandLineArguments.cpp (1)
271-278: Consider enhancing the error messageWhile the error message is clear, it could be more helpful by explaining why these options are incompatible.
Apply this change to improve the error message:
- SPDLOG_ERROR( - "Invalid combination of arguments; --file-type {} and " - "--structurize-arrays can't be used together", - cKeyValueIrFileType - ); + SPDLOG_ERROR( + "Invalid combination of arguments; --file-type {} and " + "--structurize-arrays can't be used together as key-value IR " + "format does not support array structuring", + cKeyValueIrFileType + );
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
components/core/src/clp_s/CommandLineArguments.cpp(3 hunks)components/core/src/clp_s/CommandLineArguments.hpp(3 hunks)components/core/src/clp_s/JsonParser.hpp(2 hunks)components/core/src/clp_s/clp-s.cpp(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- components/core/src/clp_s/clp-s.cpp
🔇 Additional comments (8)
components/core/src/clp_s/JsonParser.hpp (2)
15-15: LGTM: Include directive is correctly placed
The addition of CommandLineArguments.hpp is necessary for using the FileType enum.
33-33: LGTM: Member variable is properly initialized
The input_file_type member is correctly initialized with FileType::Json as the default value, maintaining backward compatibility.
components/core/src/clp_s/CommandLineArguments.hpp (3)
39-42: LGTM: FileType enum is well-defined
The enum class is correctly scoped and uses uint8_t as the underlying type, which is memory efficient.
124-124: LGTM: get_file_type method is properly defined
The method is correctly marked with [[nodiscard]] and returns the FileType enum.
194-194: LGTM: Member variable is properly initialized
The m_file_type member is correctly initialized with FileType::Json as the default value.
components/core/src/clp_s/CommandLineArguments.cpp (3)
151-151: LGTM: Command-line option is well-defined
The file-type option is properly added with a clear description and appropriate value name.
Also applies to: 206-209
263-265: LGTM: Constants are properly defined
The file type string constants are correctly defined as constexpr string_view.
266-282: LGTM: Validation logic is thorough
The validation logic:
- Properly handles both supported file types
- Includes error checking for invalid combinations
- Provides clear error messages
- Throws appropriate exceptions for unknown file types
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (2)
components/core/src/clp_s/CommandLineArguments.cpp (2)
208-211: Enhance the help message for the --file-type option.Consider making the help message more informative by including the default value.
- "The type of file being compressed (json or kv-ir)" + "The type of file being compressed (json or kv-ir, defaults to json)"
265-281: Refactor the file type parsing logic to reduce nesting.The current implementation can be made more maintainable by reducing nesting and using early returns.
if (parsed_command_line_options.count("file-type") > 0) { - if (cJsonFileType == file_type) { - m_file_type = FileType::Json; - } else if (cKeyValueIrFileType == file_type) { - m_file_type = FileType::KeyValueIr; - if (m_structurize_arrays) { - SPDLOG_ERROR( - "Invalid combination of arguments; --file-type {} and " - "--structurize-arrays can't be used together", - cKeyValueIrFileType - ); - return ParsingResult::Failure; - } - } else { - throw std::invalid_argument("Unknown FILE_TYPE: " + file_type); - } + if (cJsonFileType == file_type) { + m_file_type = FileType::Json; + return ParsingResult::Success; + } + + if (cKeyValueIrFileType == file_type) { + if (m_structurize_arrays) { + SPDLOG_ERROR( + "Invalid combination of arguments; --file-type {} and " + "--structurize-arrays can't be used together", + cKeyValueIrFileType + ); + return ParsingResult::Failure; + } + m_file_type = FileType::KeyValueIr; + return ParsingResult::Success; + } + + throw std::invalid_argument("Unknown FILE_TYPE: " + file_type); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
components/core/src/clp_s/CommandLineArguments.cpp(3 hunks)components/core/src/clp_s/clp-s.cpp(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- components/core/src/clp_s/clp-s.cpp
🔇 Additional comments (1)
components/core/src/clp_s/CommandLineArguments.cpp (1)
151-153: LGTM! Constants and default value are well-defined.
The use of string_view for constants and the naming convention with 'c' prefix follow good practices.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
components/core/src/clp_s/CommandLineArguments.cpp (2)
208-211: Enhance the help text for the --file-type option.While the option is well-integrated, the help text could be more descriptive about the purpose and implications of each file type.
Consider this improved help text:
- "The type of file being compressed (json or kv-ir)" + "The type of file being compressed: json (JSON logs) or kv-ir (key-value IR format). Note: kv-ir is incompatible with --structurize-arrays"
265-279: LGTM! Consider extracting validation to a separate method.The validation logic is comprehensive and handles all cases appropriately. However, for better maintainability as more file types are added, consider extracting this validation into a separate method.
Consider this refactor:
+ FileType validate_and_get_file_type(std::string const& file_type, bool structurize_arrays) { + if (cJsonFileType == file_type) { + return FileType::Json; + } + if (cKeyValueIrFileType == file_type) { + if (structurize_arrays) { + SPDLOG_ERROR( + "Invalid combination of arguments; --file-type {} and " + "--structurize-arrays can't be used together", + cKeyValueIrFileType + ); + throw std::invalid_argument("Invalid combination of arguments"); + } + return FileType::KeyValueIr; + } + throw std::invalid_argument("Unknown FILE_TYPE: " + file_type); + } + - if (cJsonFileType == file_type) { - m_file_type = FileType::Json; - } else if (cKeyValueIrFileType == file_type) { - m_file_type = FileType::KeyValueIr; - if (m_structurize_arrays) { - SPDLOG_ERROR( - "Invalid combination of arguments; --file-type {} and " - "--structurize-arrays can't be used together", - cKeyValueIrFileType - ); - return ParsingResult::Failure; - } - } else { - throw std::invalid_argument("Unknown FILE_TYPE: " + file_type); - } + try { + m_file_type = validate_and_get_file_type(file_type, m_structurize_arrays); + } catch (std::invalid_argument const&) { + return ParsingResult::Failure; + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
components/core/src/clp_s/CommandLineArguments.cpp(3 hunks)components/core/src/clp_s/clp-s.cpp(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- components/core/src/clp_s/clp-s.cpp
🔇 Additional comments (1)
components/core/src/clp_s/CommandLineArguments.cpp (1)
151-153: LGTM! Well-defined constants and default value.
The constants are appropriately defined as string_view for compile-time optimization, and the default file type is sensibly set to "json".
How about |
AVMatthews
changed the title
Description
Added command line interface needed for compressing IRv2 to archive.
--file-typeflag to theccompression option of the clp-s command line interface--stucturize-arrraysisn't used in conjunction with incorrect file types.Validation performed
Called the corresponding command line options (e.g.
./clp-s c test-archive test_ir.ir --file-type IR)to see that the command line options called the correct function paths.Called
./clp-s c test-archive test_ir.ir --file-type IR --structurize-arraysto see the an error message is produced.Called
./clp-s c test-archive test_json.json&&./clp-s c test-archive test_json.json --structurize-arraysto make sure both of these command result in the expected normal opperation.Called
./clp-s c --helpto see the correct usage message was printed including the new flag option.Summary by CodeRabbit
Summary by CodeRabbit
New Features
JsonParserto specify the type of input file it processes.Bug Fixes
Documentation
file-typeoption and its usage in compression commands.