refactor: split datacell factory interfaces#2002
Open
LHT129 wants to merge 3 commits into
Open
Conversation
Move graph and flatten instance construction out of core interface headers so factory dependencies no longer fan out through widely included abstractions. This keeps datacell creation logic in dedicated factory units while preserving existing behavior and serialization paths. Signed-off-by: LHT129 <tianlan.lht@antgroup.com>
LHT129
added
kind/improvement
Contributor
Merge ProtectionsYour pull request matches the following merge protections and will not be merged until they are valid. 🟢 Require kind labelWonderful, this rule succeeded.
🟢 Require version labelWonderful, this rule succeeded.
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Code Review
This pull request refactors the instantiation of FlattenInterface and GraphInterface by moving the factory logic from the interface classes into dedicated factory files (flatten_factory.cpp and graph_factory.cpp). This involves replacing static MakeInstance methods with standalone MakeFlattenInstance and MakeGraphInstance functions across the codebase. Feedback identifies a potential null pointer dereference in the graph factory and suggests refactoring the IO type selection logic in the flatten factory using a map to improve maintainability.
Contributor
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Refactors datacell instance creation by extracting FlattenInterface/GraphInterface construction logic into dedicated factory headers/source files, reducing core header coupling and updating call sites accordingly.
Changes:
- Introduce
MakeFlattenInstanceandMakeGraphInstancein newflatten_factory.*andgraph_factory.*files. - Remove
MakeInstancestatic constructors fromflatten_interface.*andgraph_interface.*. - Update production and test code to include the new factory headers and call the new factory functions.
Reviewed changes
Copilot reviewed 26 out of 26 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| src/index/index_impl.h | Adds dataset_impl.h include after decoupling it from inner_index_interface.h. |
| src/index/hnsw_test.cpp | Switches test call sites to MakeFlattenInstance / MakeGraphInstance. |
| src/index/hnsw.h | Reduces header coupling by dropping logger include. |
| src/index/hnsw.cpp | Uses new factory functions and includes factory headers. |
| src/index/diskann.cpp | Uses MakeFlattenInstance and includes flatten factory headers. |
| src/impl/pruning_strategy_test.cpp | Updates tests to use new factory headers/functions. |
| src/impl/odescent/odescent_graph_builder_test.cpp | Updates tests to use new factory headers/functions. |
| src/datacell/sparse_vector_datacell_test.cpp | Updates tests to use MakeFlattenInstance. |
| src/datacell/sparse_graph_datacell_test.cpp | Updates tests to use MakeGraphInstance and adds factory include. |
| src/datacell/graph_interface.h | Removes static MakeInstance and related header dependencies. |
| src/datacell/graph_interface.cpp | Removes factory implementation from interface TU. |
| src/datacell/graph_factory.h | Adds new public graph factory API declaration. |
| src/datacell/graph_factory.cpp | Implements graph factory logic formerly in GraphInterface::MakeInstance. |
| src/datacell/graph_datacell_test.cpp | Updates tests to use MakeGraphInstance. |
| src/datacell/flatten_interface.h | Removes static MakeInstance and related header dependencies. |
| src/datacell/flatten_interface.cpp | Removes factory implementation from interface TU. |
| src/datacell/flatten_factory.h | Adds new public flatten factory API declaration. |
| src/datacell/flatten_factory.cpp | Implements flatten factory logic formerly in FlattenInterface::MakeInstance. |
| src/datacell/flatten_datacell_test.cpp | Updates tests to use MakeFlattenInstance. |
| src/datacell/compressed_graph_datacell_test.cpp | Updates tests to use MakeGraphInstance. |
| src/algorithm/warp.cpp | Uses MakeFlattenInstance for code storage creation. |
| src/algorithm/pyramid.h | Uses MakeFlattenInstance and includes factory headers. |
| src/algorithm/ivf.cpp | Uses MakeFlattenInstance and includes the flatten factory header. |
| src/algorithm/inner_index_interface.h | Removes dataset_impl.h to reduce coupling. |
| src/algorithm/hgraph.cpp | Uses both new factory functions for codes/graph creation. |
| src/algorithm/brute_force.cpp | Uses MakeFlattenInstance and includes factory header. |
Comment on lines
+23
to
+24
| FlattenInterfacePtr | ||
| MakeFlattenInstance(const FlattenInterfaceParamPtr& param, const IndexCommonParam& common_param); |
Comment on lines
+25
to
+34
| GraphInterfacePtr | ||
| MakeGraphInstance(const GraphInterfaceParamPtr& graph_param, const IndexCommonParam& common_param) { | ||
| switch (graph_param->graph_storage_type_) { | ||
| case GraphStorageTypes::GRAPH_STORAGE_TYPE_SPARSE: | ||
| return std::make_shared<SparseGraphDataCell>(graph_param, common_param); | ||
| case GraphStorageTypes::GRAPH_STORAGE_TYPE_VALUE_COMPRESSED: | ||
| return std::make_shared<CompressedGraphDataCell>(graph_param, common_param); | ||
| case GraphStorageTypes::GRAPH_STORAGE_TYPE_VALUE_FLAT: | ||
| auto io_string = std::dynamic_pointer_cast<GraphDataCellParameter>(graph_param) | ||
| ->io_parameter_->GetTypeName(); |
|
|
||
| #include <fmt/format.h> | ||
|
|
||
| #include "graph_factory.h" |
| @@ -13,204 +13,3 @@ | |||
| // limitations under the License. | |||
|
|
|||
Add the concrete parameter and common parameter headers at the translation units that still relied on transitive includes after the datacell factory split. This keeps the refactor intact while restoring CI builds across TSAN, CircleCI, and test targets. Signed-off-by: LHT129 <tianlan.lht@antgroup.com>
Run the repository formatter after adding the direct include dependencies introduced by the factory split follow-up. This keeps the CI-only include fixes aligned with the project include ordering rules. Signed-off-by: LHT129 <tianlan.lht@antgroup.com>
Comment on lines
29
to
+33
| #include "datacell/flatten_datacell.h" | ||
| #include "datacell/flatten_datacell_parameter.h" | ||
| #include "datacell/flatten_factory.h" | ||
| #include "datacell/graph_datacell_parameter.h" | ||
| #include "datacell/graph_factory.h" |
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
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.
Summary
Testing