refactor(indexer): Apply RAII to all classes. - #845
Draft
Bill-hbrhbr wants to merge 5 commits into
Draft
Conversation
…er instead of table-name
Contributor
## Walkthrough
This set of changes refactors the handling of dataset and table naming conventions across several components. The main updates include renaming variables and function parameters from "table" to "dataset" to reflect a shift in terminology, updating function signatures and class interfaces to accept dataset names and prefixes, and consolidating database connection and initialization logic into constructors and destructors. The changes also standardize the construction of metadata table names and remove explicit state management in database storage classes. Additionally, a hardcoded dataset name is replaced with a configuration constant.
## Changes
| File(s) | Change Summary |
|----------------------------------------------------------------------------------------------------------------------------------------------------|-----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------|
| `components/clp-py-utils/clp_py_utils/clp_metadata_db_utils.py` | Modified `_create_column_metadata_table` to accept a `table_prefix` instead of a full table name and updated SQL construction; updated `create_metadata_db_tables` to use the new parameterization and added a TODO for future dataset-specific logic. |
| `components/core/src/clp_s/indexer/CommandLineArguments.cpp`,<br>`components/core/src/clp_s/indexer/CommandLineArguments.hpp` | Renamed all references from "table-name" and `m_table_name` to "dataset-name" and `m_dataset_name`; updated accessor method to `get_dataset_name()`. |
| `components/core/src/clp_s/indexer/IndexManager.cpp`,<br>`components/core/src/clp_s/indexer/IndexManager.hpp` | Updated the `IndexManager` constructor to require `dataset_name` and `archive_path`; removed the `update_metadata` method and `m_should_create_table` member; moved metadata update logic into the constructor; updated destructor formatting. |
| `components/core/src/clp_s/indexer/MySQLIndexStorage.cpp`,<br>`components/core/src/clp_s/indexer/MySQLIndexStorage.hpp` | Refactored to remove explicit `open()`, `init()`, and `close()` methods; moved initialization logic to the constructor and cleanup to the destructor; updated table name construction to use new suffix constant; simplified class interface and removed state-tracking members. |
| `components/core/src/clp_s/indexer/indexer.cpp` | Updated construction of `IndexManager` to pass `dataset_name`, `should_create_table`, and `archive_path` directly; removed the separate call to `update_metadata`. |
| `components/job-orchestration/job_orchestration/executor/compress/compression_task.py` | Replaced hardcoded `"default"` dataset name with the `CLP_DEFAULT_DATASET_NAME` constant in the `run_clp` function. |
| `taskfiles/lint.yaml` | Removed `IndexManager.hpp` and `MySQLIndexStorage.hpp` from the list of source files included in the C++ linting configuration under the `check-cpp-static-full` task. |
## Sequence Diagram(s)
```mermaid
sequenceDiagram
participant User
participant CommandLineArguments
participant IndexManager
participant MySQLIndexStorage
participant ArchiveReader
User->>CommandLineArguments: Provide dataset name and archive path
User->>IndexManager: Construct with db_config, dataset_name, should_create_table, archive_path
IndexManager->>MySQLIndexStorage: Construct with connection info, table_prefix, dataset_name, should_create_table
IndexManager->>ArchiveReader: Open archive at archive_path
ArchiveReader-->>IndexManager: Return schema tree
IndexManager->>IndexManager: traverse_schema_tree_and_update_metadata(schema_tree)
IndexManager-->>User: Ready for indexingPossibly related PRs
Suggested reviewers
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🔭 Outside diff range comments (1)
components/core/src/clp_s/indexer/CommandLineArguments.cpp (1)
58-59:⚠️ Potential issueInconsistency in positional option description.
While the option name was updated to "dataset-name" on line 48, the positional description still uses "table-name" which will cause incorrect argument parsing.
Fix this by updating the positional description:
- positional_options_description.add("table-name", 1); + positional_options_description.add("dataset-name", 1); positional_options_description.add("archive-path", 1);
🧹 Nitpick comments (4)
components/clp-py-utils/clp_py_utils/clp_metadata_db_utils.py (1)
84-93: Parameter naming could be misleading & lacks minimal validation
_create_column_metadata_tablenow receives a full table name prefix rather than a true “prefix”. Calling this variabletable_prefixcan be confusing, especially because every other helper in the module expects an actual prefix (without dataset / trailing underscore). In addition, the value is interpolated directly into the SQL string, which means a malformed prefix could break the statement or even be abused for SQL-injection of identifiers.Consider:
-def _create_column_metadata_table(db_cursor, table_prefix: str) -> None: +def _create_column_metadata_table(db_cursor, full_table_prefix: str) -> None:and, before executing, validate that
full_table_prefixonly contains allowed characters ([A-Za-z0-9_]+).components/core/src/clp_s/indexer/IndexManager.hpp (1)
39-45: Boolean construction flag hurts readabilityThe constructor now has four positional parameters, two of which are strings that differ only semantically and a raw
boolwhose purpose is not obvious at the call-site. Consider replacing thebool should_create_tablewith an enum or a strong typedef (e.g.TableCreation::Yes/No) or providing a named builder/factory to avoid accidental misuse.components/core/src/clp_s/indexer/MySQLIndexStorage.cpp (1)
44-45: Unnecessaryreset()before first use
m_insert_field_statement.reset();is called immediately after construction, but the smart-pointer is already empty. The call is harmless yet redundant and can be removed to reduce noise.components/core/src/clp_s/indexer/MySQLIndexStorage.hpp (1)
43-49: Missing documentation for new parametersThe doxygen block still talks about “table” but not “dataset” or the
should_create_tableflag.
Please add brief parameter docs so downstream users (and IDE tooltips) know what each argument does.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
components/clp-py-utils/clp_py_utils/clp_metadata_db_utils.py(2 hunks)components/core/src/clp_s/indexer/CommandLineArguments.cpp(3 hunks)components/core/src/clp_s/indexer/CommandLineArguments.hpp(2 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(3 hunks)components/core/src/clp_s/indexer/MySQLIndexStorage.hpp(2 hunks)components/core/src/clp_s/indexer/indexer.cpp(1 hunks)components/job-orchestration/job_orchestration/executor/compress/compression_task.py(2 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
`**/*.{cpp,hpp,java,js,jsx,tpp,ts,tsx}`: - Prefer `false == ` rather than `!`.
**/*.{cpp,hpp,java,js,jsx,tpp,ts,tsx}: - Preferfalse == <expression>rather than!<expression>.
components/core/src/clp_s/indexer/CommandLineArguments.hppcomponents/core/src/clp_s/indexer/CommandLineArguments.cppcomponents/core/src/clp_s/indexer/indexer.cppcomponents/core/src/clp_s/indexer/IndexManager.hppcomponents/core/src/clp_s/indexer/IndexManager.cppcomponents/core/src/clp_s/indexer/MySQLIndexStorage.cppcomponents/core/src/clp_s/indexer/MySQLIndexStorage.hpp
🧬 Code Graph Analysis (3)
components/core/src/clp_s/indexer/CommandLineArguments.cpp (1)
components/core/src/clp_s/indexer/CommandLineArguments.hpp (1)
m_dataset_name(31-31)
components/core/src/clp_s/indexer/MySQLIndexStorage.cpp (1)
components/core/src/clp_s/indexer/MySQLIndexStorage.hpp (2)
MySQLIndexStorage(29-38)MySQLIndexStorage(41-41)
components/core/src/clp_s/indexer/MySQLIndexStorage.hpp (1)
components/core/src/clp_s/indexer/MySQLIndexStorage.cpp (3)
MySQLIndexStorage(16-67)MySQLIndexStorage(69-72)MySQLIndexStorage(69-69)
⏰ Context from checks skipped due to timeout of 90000ms (11)
- GitHub Check: centos-stream-9-dynamic-linked-bins
- GitHub Check: ubuntu-jammy-lint
- GitHub Check: ubuntu-jammy-dynamic-linked-bins
- GitHub Check: centos-stream-9-static-linked-bins
- GitHub Check: ubuntu-jammy-static-linked-bins
- GitHub Check: build-macos (macos-13, false)
- GitHub Check: build-macos (macos-15, false)
- GitHub Check: build-macos (macos-14, false)
- GitHub Check: build-macos (macos-13, true)
- GitHub Check: lint-check (ubuntu-latest)
- GitHub Check: build (macos-latest)
🔇 Additional comments (10)
components/job-orchestration/job_orchestration/executor/compress/compression_task.py (2)
14-14: Good addition of the CLP_DEFAULT_DATASET_NAME import.This import is necessary for the usage of the constant at line 382.
382-382: Great replacement of hardcoded string with constant.Replacing the hardcoded string "default" with
CLP_DEFAULT_DATASET_NAMEimproves maintainability and consistency. This change aligns with the renaming from "table-name" to "dataset-name" for clarity.components/core/src/clp_s/indexer/CommandLineArguments.hpp (2)
31-31: Appropriate method name change for clarity.Renaming
get_table_name()toget_dataset_name()improves the clarity of the codebase by using more accurate terminology.
47-47: Appropriate member variable renaming for clarity.Renaming
m_table_nametom_dataset_nameensures consistency with the function and parameter naming changes.components/core/src/clp_s/indexer/CommandLineArguments.cpp (3)
48-50: Option name updated correctly.The positional option description was appropriately updated from "table-name" to "dataset-name".
92-94: Correctly updated validation for dataset name.The validation logic has been properly updated to check for an empty dataset name.
130-131: Usage message updated correctly.The usage message correctly reflects the renamed parameter "DATASET_NAME" instead of "TABLE_NAME".
components/core/src/clp_s/indexer/indexer.cpp (1)
35-40: Successfully applied RAII principle to IndexManager.The construction of
IndexManagernow accepts all necessary parameters directly, which aligns with the Resource Acquisition Is Initialization (RAII) principle. This is a good improvement that eliminates the need for a separateupdate_metadatacall after construction.components/clp-py-utils/clp_py_utils/clp_metadata_db_utils.py (1)
136-138: Column-metadata table name may contain unwanted double “_” and ignoresdatasetparameterWhen a non-default dataset is passed,
table_prefixalready includes<dataset>_.
You then appendCLP_DEFAULT_DATASET_NAMEand another “_”, resulting in e.g.myprefix_orders_default__column_metadata ^ extra underscoreThis also means the metadata table is always created for the default dataset, never for the user-supplied one, which is probably not what you want once datasets become first-class.
Please double-check the desired naming scheme. If the intent is to skip creation for non-
CLP_Sdatasets, wrap the call in a guard instead of hard-coding the default name.components/core/src/clp_s/indexer/MySQLIndexStorage.hpp (1)
40-42: Make the destructornoexcept
m_db.close()is invoked in the dtor. Ifclose()can throw, unwinding through a destructor during another exception willstd::terminate.
Either:
- Guarantee
MySQLDB::close()isnoexcept, or- Catch and log inside the destructor, then mark the destructor
noexcept.- ~MySQLIndexStorage(); + ~MySQLIndexStorage() noexcept;Remember to wrap the body with
try { ... } catch (...) { SPDLOG_ERROR(...); }.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
components/core/src/clp_s/indexer/MySQLIndexStorage.hpp (2)
19-19:⚠️ Potential issueThe
cColumnMetadataSuffixconstant is missing a separator, which could lead to malformed table namesAs mentioned in a previous review, without a leading underscore, table name formation requires callers to remember to include a delimiter in
table_prefix. Based on the implementation inMySQLIndexStorage.cpp, table names are constructed astable_prefix + dataset_name + "_" + cColumnMetadataSuffix, which means there's no separator betweentable_prefixanddataset_name.- static constexpr auto cColumnMetadataSuffix = "column_metadata"; + static constexpr auto cColumnMetadataSuffix = "_column_metadata";Alternatively, modify the format string in the implementation to ensure consistent delimiter insertion.
29-39: 🛠️ Refactor suggestionConstructor should be marked as
explicitto prevent accidental implicit conversionsThe constructor takes multiple parameters, making it susceptible to accidental implicit conversions if only one argument is provided. Marking it as
explicithelps prevent these issues.- // Constructor - MySQLIndexStorage( + // Constructor + explicit 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, std::string const& dataset_name, bool should_create_table );
🧹 Nitpick comments (1)
components/core/src/clp_s/indexer/MySQLIndexStorage.hpp (1)
49-51: Missing documentation for the destructorFor consistency with the rest of the class, please add documentation for the destructor to explain that it releases database resources. This would help maintainers understand the resource cleanup process.
// Destructor + /** + * Releases database resources and closes the connection + */ ~MySQLIndexStorage();
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
components/core/src/clp_s/indexer/IndexManager.cpp(2 hunks)components/core/src/clp_s/indexer/IndexManager.hpp(2 hunks)components/core/src/clp_s/indexer/MySQLIndexStorage.cpp(3 hunks)components/core/src/clp_s/indexer/MySQLIndexStorage.hpp(2 hunks)taskfiles/lint.yaml(0 hunks)
💤 Files with no reviewable changes (1)
- taskfiles/lint.yaml
🚧 Files skipped from review as they are similar to previous changes (3)
- components/core/src/clp_s/indexer/MySQLIndexStorage.cpp
- components/core/src/clp_s/indexer/IndexManager.cpp
- components/core/src/clp_s/indexer/IndexManager.hpp
🧰 Additional context used
📓 Path-based instructions (1)
`**/*.{cpp,hpp,java,js,jsx,tpp,ts,tsx}`: - Prefer `false == ` rather than `!`.
**/*.{cpp,hpp,java,js,jsx,tpp,ts,tsx}: - Preferfalse == <expression>rather than!<expression>.
components/core/src/clp_s/indexer/MySQLIndexStorage.hpp
🧬 Code Graph Analysis (1)
components/core/src/clp_s/indexer/MySQLIndexStorage.hpp (1)
components/core/src/clp_s/indexer/MySQLIndexStorage.cpp (3)
MySQLIndexStorage(16-66)MySQLIndexStorage(68-71)MySQLIndexStorage(68-68)
⏰ Context from checks skipped due to timeout of 90000ms (11)
- GitHub Check: centos-stream-9-static-linked-bins
- GitHub Check: centos-stream-9-dynamic-linked-bins
- GitHub Check: ubuntu-jammy-dynamic-linked-bins
- GitHub Check: ubuntu-jammy-lint
- GitHub Check: ubuntu-jammy-static-linked-bins
- GitHub Check: build-macos (macos-15, true)
- GitHub Check: build-macos (macos-13, true)
- GitHub Check: build-macos (macos-15, false)
- GitHub Check: build-macos (macos-13, false)
- GitHub Check: lint-check (macos-latest)
- GitHub Check: lint-check (ubuntu-latest)
🔇 Additional comments (3)
components/core/src/clp_s/indexer/MySQLIndexStorage.hpp (3)
41-48: Good application of move semantics and preventing object slicingThe explicit deletion of copy operations and defaulting of move operations with
noexceptis excellent practice for a resource-owning class. This properly enforces RAII semantics and prevents potential issues with double-closing database connections.
58-58: Using trailing return type syntax for method declarationThe change from
void add_field(...)toauto add_field(...) -> voiduses modern C++ trailing return type syntax. While this is stylistically valid, ensure this pattern is applied consistently across the codebase.
62-64: Good RAII implementation with simplified member variablesThe reduction of member variables to just the essentials (database connection and prepared statement) is consistent with RAII principles. This simplification makes the class easier to understand and maintain.
Bill-hbrhbr
changed the title
table-name to dataset-name for clarity; Update the default column metadata table name; Apply RAII to all classes.
Bill-hbrhbr
marked this pull request as draft
April 24, 2025 16:11
gibber9809
reviewed
May 5, 2025
Contributor
There was a problem hiding this comment.
Is this PR superseded by #855 (and maybe some other PRs I haven't found)/can we close it?
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
…
Description
Checklist
breaking change.
Validation performed
CLP_Sstorage engine.Summary by CodeRabbit
Refactor
Bug Fixes