Cherry pick2 before #11
Conversation
Signed-off-by: Wish <breezewish@outlook.com>
…pingcap#166) Signed-off-by: Wish <breezewish@outlook.com>
Signed-off-by: Wish <breezewish@outlook.com>
Signed-off-by: Lloyd-Pottiger <yan1579196623@gmail.com>
Signed-off-by: Lloyd-Pottiger <yan1579196623@gmail.com>
WalkthroughThis update introduces significant improvements to the handling of vector indices within the database management system. Key changes include the restructuring of vector index management into distinct builder and viewer classes, enhancing clarity and efficiency. Additionally, several files have been modified to improve cache management, performance tracking, and S3 integration, ensuring robust error handling and better resource management. Overall, the modifications streamline interactions with vector indices and improve the system's performance and maintainability. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Cache
participant S3
participant VectorIndexBuilder
participant VectorIndexViewer
User->>Cache: Request file
Cache->>S3: Check file existence
S3-->>Cache: File status
Cache->>VectorIndexBuilder: Prepare index
VectorIndexBuilder-->>Cache: Index ready
Cache-->>User: Return file with index
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 as PR comments)
Additionally, you can add CodeRabbit Configuration File (
|
|
@coderabbitai review |
Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Outside diff range, codebase verification and nitpick comments (1)
dbms/src/Storages/S3/FileCache.cpp (1)
68-107: Consider defining a constant for the timeout value.The use of a magic number (300 seconds) for the timeout can be improved by defining a named constant for better readability and maintainability.
constexpr double WAIT_TIMEOUT_SECONDS = 300.0; // Replace 300 with WAIT_TIMEOUT_SECONDS in the code.
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (36)
- contrib/tipb (1 hunks)
- dbms/src/Common/FailPoint.cpp (1 hunks)
- dbms/src/Common/LRUCache.h (1 hunks)
- dbms/src/Interpreters/Context.cpp (3 hunks)
- dbms/src/Interpreters/Context.h (1 hunks)
- dbms/src/Server/Server.cpp (1 hunks)
- dbms/src/Storages/DeltaMerge/DMContext.h (2 hunks)
- dbms/src/Storages/DeltaMerge/DeltaMergeDefines.h (1 hunks)
- dbms/src/Storages/DeltaMerge/File/ColumnStat.h (1 hunks)
- dbms/src/Storages/DeltaMerge/File/DMFileWithVectorIndexBlockInputStream.cpp (1 hunks)
- dbms/src/Storages/DeltaMerge/File/DMFileWithVectorIndexBlockInputStream.h (5 hunks)
- dbms/src/Storages/DeltaMerge/File/DMFileWriter.cpp (4 hunks)
- dbms/src/Storages/DeltaMerge/File/DMFileWriter.h (4 hunks)
- dbms/src/Storages/DeltaMerge/File/VectorColumnFromIndexReader.cpp (5 hunks)
- dbms/src/Storages/DeltaMerge/File/VectorColumnFromIndexReader.h (4 hunks)
- dbms/src/Storages/DeltaMerge/File/dtpb/dmfile.proto (3 hunks)
- dbms/src/Storages/DeltaMerge/Index/RSIndex.h (1 hunks)
- dbms/src/Storages/DeltaMerge/Index/VectorIndex.cpp (2 hunks)
- dbms/src/Storages/DeltaMerge/Index/VectorIndex.h (1 hunks)
- dbms/src/Storages/DeltaMerge/Index/VectorIndexCache.cpp (1 hunks)
- dbms/src/Storages/DeltaMerge/Index/VectorIndexCache.h (1 hunks)
- dbms/src/Storages/DeltaMerge/Index/VectorIndexHNSW/Index.cpp (5 hunks)
- dbms/src/Storages/DeltaMerge/Index/VectorIndexHNSW/Index.h (1 hunks)
- dbms/src/Storages/DeltaMerge/Index/VectorIndex_fwd.h (1 hunks)
- dbms/src/Storages/DeltaMerge/Index/VectorSearchPerf.cpp (1 hunks)
- dbms/src/Storages/DeltaMerge/Index/VectorSearchPerf.h (1 hunks)
- dbms/src/Storages/DeltaMerge/Remote/Serializer.h (2 hunks)
- dbms/src/Storages/DeltaMerge/ScanContext.h (6 hunks)
- dbms/src/Storages/DeltaMerge/tests/gtest_dm_vector_index.cpp (26 hunks)
- dbms/src/Storages/S3/FileCache.cpp (8 hunks)
- dbms/src/Storages/S3/FileCache.h (7 hunks)
- dbms/src/Storages/S3/FileCachePerf.cpp (1 hunks)
- dbms/src/Storages/S3/FileCachePerf.h (1 hunks)
- dbms/src/TiDB/Schema/TiDB.cpp (3 hunks)
- dbms/src/TiDB/Schema/TiDB.h (1 hunks)
- dbms/src/TiDB/Schema/VectorIndex.h (1 hunks)
Files skipped from review due to trivial changes (1)
- contrib/tipb
Additional comments not posted (85)
dbms/src/Storages/S3/FileCachePerf.cpp (1)
1-22: File Setup Looks Good.The licensing information and namespace declaration are correctly formatted and follow standard conventions.
dbms/src/Storages/DeltaMerge/Index/VectorSearchPerf.cpp (1)
1-22: File Setup Looks Good.The licensing information and namespace declaration are correctly formatted and follow standard conventions.
dbms/src/Storages/DeltaMerge/Index/RSIndex.h (1)
Line range hint
13-23:
Simplification ofRSIndexStructure Looks Good.The removal of
VectorIndexPtrand its constructor simplifies theRSIndexstructure, likely improving clarity and reducing complexity.Ensure that any dependencies on the removed
VectorIndexPtrare updated accordingly in the codebase.Verification successful
Removal of
VectorIndexPtris Complete.The search did not find any remaining references to
VectorIndexPtr, indicating that its removal from the codebase is thorough and no dependencies remain.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify dependencies on the removed `VectorIndexPtr`. # Test: Search for any remaining usage of `VectorIndexPtr` in the codebase. rg --type cpp 'VectorIndexPtr'Length of output: 30
dbms/src/Storages/DeltaMerge/Index/VectorIndex_fwd.h (1)
24-28: Restructuring of vector index handling.The introduction of
VectorIndexBuilderandVectorIndexViewerclasses, along with their respective smart pointer types, enhances the modularity of the vector index system. This separation of concerns can improve maintainability and potentially introduce new capabilities or optimizations in the indexing process.dbms/src/Storages/S3/FileCachePerf.h (1)
27-35: Introduction of performance tracking for S3 file caching.The
FileCachePerfContextstructure and the thread-local instancefile_cacheprovide a mechanism to track performance metrics related to S3 file caching. This can be beneficial for identifying bottlenecks and optimizing performance in multi-threaded environments.dbms/src/Storages/DeltaMerge/Index/VectorSearchPerf.h (1)
27-35: Introduction of performance tracking for vector search operations.The
VectorSearchPerfContextstructure and the thread-local instancevector_searchprovide a mechanism to track performance metrics related to vector search operations. This can aid in performance optimization and debugging in multi-threaded environments.dbms/src/Storages/DeltaMerge/Index/VectorIndexHNSW/Index.h (3)
24-25: Update inUSearchImplTypeusage.The change from
VectorIndex::KeytoVectorIndexBuilder::Keysuggests a shift in the design pattern, possibly to decouple responsibilities. Ensure that this change is consistent across the codebase and that all dependencies are updated accordingly.Verification successful
Consistent usage of
USearchImplTypewithVectorIndexBuilder::Key.The change from
VectorIndex::KeytoVectorIndexBuilder::Keyis consistently reflected across the codebase. No issues found with the implementation.
USearchImplTypeis used consistently withVectorIndexBuilder::Keyin bothIndex.handIndex.cpp.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the usage of `USearchImplType` with `VectorIndexBuilder::Key` across the codebase. # Test: Search for the usage of `USearchImplType`. Expect: Consistent usage with `VectorIndexBuilder::Key`. rg --type cpp -A 3 $'USearchImplType'Length of output: 2454
41-55: Addition ofVectorIndexHNSWViewer.The new class
VectorIndexHNSWViewerprovides functionality for viewing vector indices. Theviewmethod andsearchmethod are key components. Ensure that these methods are correctly implemented and handle all edge cases, especially regarding file properties and query information.Verification successful
Verification of
VectorIndexHNSWViewermethods successful.The
viewandsearchmethods inVectorIndexHNSWViewerare implemented correctly, handling file properties and query information appropriately. Theviewmethod validates the index kind and distance metric, while thesearchmethod ensures the query vector size matches the index dimensions. No issues found.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the implementation of `view` and `search` methods in `VectorIndexHNSWViewer`. # Test: Search for the implementation of `view` and `search` methods. Expect: Correct handling of file properties and query information. ast-grep --lang cpp --pattern $'class VectorIndexHNSWViewer { $$$ static $_ view($_, $_) { $$$ } $$$ std::vector<Key> search($_, $_) const { $$$ } $$$ }'Length of output: 521
Script:
#!/bin/bash # Search for the implementation of the `view` method in the `VectorIndexHNSWViewer` class. rg -A 10 'VectorIndexHNSWViewer::view' --type=cpp # Search for the implementation of the `search` method in the `VectorIndexHNSWViewer` class. rg -A 10 'VectorIndexHNSWViewer::search' --type=cppLength of output: 3154
27-39: Introduction ofVectorIndexHNSWBuilder.The new class
VectorIndexHNSWBuilderinherits fromVectorIndexBuilder, indicating a refactoring effort to separate concerns. The constructor now takes aVectorIndexDefinitionPtr, which aligns with the new structure changes. Ensure that the logic withinaddBlockandsavemethods aligns with the intended functionality and that all necessary validations are in place.dbms/src/Storages/DeltaMerge/Index/VectorIndexCache.h (1)
30-77: Introduction ofVectorIndexCache.The
VectorIndexCacheclass uses an LRU cache to manage vector index viewers. The implementation includes thread management for cleaning outdated cache entries. Ensure that the cache eviction logic is correctly implemented and that the thread management handles shutdown scenarios gracefully.Verification successful
Cache Eviction and Thread Management Verified in
VectorIndexCacheThe
VectorIndexCacheclass correctly implements cache eviction and thread management. ThecleanOutdatedCacheEntriesmethod efficiently removes outdated cache entries, and thecleanOutdatedLoopmethod manages thread execution and shutdown gracefully.
cleanOutdatedCacheEntries: Properly checks and removes outdated entries.cleanOutdatedLoop: Manages the cleaning loop with shutdown handling.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the implementation of cache eviction and thread management in `VectorIndexCache`. # Test: Search for the implementation of `cleanOutdatedCacheEntries` and `cleanOutdatedLoop` methods. Expect: Proper eviction logic and thread management. ast-grep --lang cpp --pattern $'class VectorIndexCache { $$$ size_t cleanOutdatedCacheEntries() { $$$ } $$$ void cleanOutdatedLoop() { $$$ } $$$ }'Length of output: 170
Script:
#!/bin/bash # Search for the definition of `cleanOutdatedCacheEntries` in the codebase. rg 'size_t cleanOutdatedCacheEntries\(\)' -A 10 # Search for the definition of `cleanOutdatedLoop` in the codebase. rg 'void cleanOutdatedLoop\(\)' -A 10Length of output: 1731
Script:
#!/bin/bash # Search for the implementation of `cleanOutdatedCacheEntries` in the codebase. rg 'size_t VectorIndexCache::cleanOutdatedCacheEntries\(\)' -A 20 # Search for the implementation of `cleanOutdatedLoop` in the codebase. rg 'void VectorIndexCache::cleanOutdatedLoop\(\)' -A 20Length of output: 3498
dbms/src/TiDB/Schema/VectorIndex.h (2)
27-40: Refactoring toVectorIndexDefinition.The refactoring consolidates vector index information into
VectorIndexDefinition, aligning with TiDB schema. Ensure that this change is consistently applied across the codebase and that all dependencies are updated.Verification successful
Refactoring to
VectorIndexDefinitionis consistent across the codebase.The refactoring of
VectorIndexDefinitionhas been applied consistently across the codebase. There are no remaining references toVectorIndexInfo, indicating a thorough update. The changes align the structure with TiDB's schema definitions as intended.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the usage of `VectorIndexDefinition` across the codebase. # Test: Search for the usage of `VectorIndexDefinition`. Expect: Consistent usage and removal of old `VectorIndexInfo` references. rg --type cpp -A 3 $'VectorIndexDefinition'Length of output: 9363
45-66: Formatter specialization forVectorIndexDefinition.The formatter specialization has been updated to reflect the new structure. Ensure that the formatting logic correctly represents the
VectorIndexDefinitionand handles null pointers appropriately.Verification successful
Formatter specialization for
VectorIndexDefinitionis correctly implemented.The formatter specialization for
VectorIndexDefinitionis present and, along withVectorIndexDefinitionPtr, appears to handle null pointers appropriately through the use of smart pointers. The code is consistent with the expected design.
dbms/src/TiDB/Schema/VectorIndex.h: Contains the formatter specializations for bothVectorIndexDefinitionandVectorIndexDefinitionPtr.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the formatter specialization for `VectorIndexDefinition`. # Test: Search for the implementation of formatter specialization. Expect: Correct handling of `VectorIndexDefinition` and null pointers. ast-grep --lang cpp --pattern $'template <> struct fmt::formatter<TiDB::VectorIndexDefinition> { $$$ auto format(const TiDB::VectorIndexDefinition & vi, $_) const { $$$ } $$$ }'Length of output: 190
Script:
#!/bin/bash # Search for any formatter specializations related to `VectorIndexDefinition`. rg --context 5 'formatter<.*VectorIndexDefinition.*>' --type cpp # Search for usages of `VectorIndexDefinition` to check for null pointer handling. rg 'VectorIndexDefinition' --type cppLength of output: 4892
dbms/src/Storages/DeltaMerge/Index/VectorIndex.cpp (3)
Line range hint
32-39:
LGTM!The implementation correctly checks for nullable types and ensures they are
DataTypeFloat32.
41-54: LGTM!The function is well-structured with appropriate runtime checks and error handling for unsupported vector index kinds.
58-71: LGTM!The function is correctly implemented with necessary checks and error handling for unsupported index kinds.
dbms/src/Storages/DeltaMerge/Index/VectorIndex.h (2)
31-55: LGTM!The
VectorIndexBuilderclass is well-structured, encapsulating the logic for building vector indices and ensuring type support.
58-90: LGTM!The
VectorIndexViewerclass is well-designed, focusing on viewing functionalities and maintaining consistency with the builder class.dbms/src/Storages/DeltaMerge/File/VectorColumnFromIndexReader.h (1)
Line range hint
47-71:
LGTM!The changes improve the design by aligning with the viewer-based approach, enhancing data retrieval efficiency.
dbms/src/Storages/DeltaMerge/Index/VectorIndexCache.cpp (4)
23-61: LGTM! Efficient cache cleaning logic.The function
cleanOutdatedCacheEntriesis well-implemented with appropriate use of locks and logging. The logic for checking file existence and removing outdated entries is sound.
63-83: LGTM! Proper loop for periodic cache cleaning.The
cleanOutdatedLoopfunction effectively manages periodic cache cleaning with a proper shutdown mechanism using a condition variable.
86-91: LGTM! Proper initialization and thread management.The constructor for
VectorIndexCachecorrectly initializes the cache and starts a thread for cleaning outdated entries.
93-98: LGTM! Safe shutdown procedure.The destructor for
VectorIndexCacheensures a safe shutdown by setting the shutdown flag and joining the cleaner thread.dbms/src/Storages/DeltaMerge/File/dtpb/dmfile.proto (2)
66-66: LGTM! UpdatedColumnStatwith new vector index properties.The change to use
VectorIndexFilePropsinColumnStatenhances clarity by separating construction and retrieval details of vector indices.
83-93: LGTM! NewVectorIndexFilePropsmessage.The introduction of
VectorIndexFilePropsprovides a well-structured representation of vector index properties, facilitating efficient index retrieval.dbms/src/Storages/DeltaMerge/Remote/Serializer.h (2)
96-96: LGTM! Maintained encapsulation forserializeColumnFileSet.The function
serializeColumnFileSetremains private, indicating it is intended for internal use only, which is appropriate.
96-96: Verify the change in access level forserializeSegment.The access level for
serializeSegmenthas been changed from private to public. Ensure this change is intentional and necessary for broader usage, and verify that it doesn't expose sensitive functionality.dbms/src/Storages/DeltaMerge/File/VectorColumnFromIndexReader.cpp (1)
37-48: Ensure the runtime check is necessary and efficient.The addition of a runtime check using
std::is_sortedensures thatsorted_resultsis in ascending order. This is a good practice for validating input assumptions. However, consider the performance implications if this check is run frequently in a production environment. If the input is guaranteed to be sorted by design, you might consider removing this check in release builds.dbms/src/Storages/DeltaMerge/DMContext.h (1)
136-137: Verify the integration of the new parameterscan_context.The addition of
scan_contextas a parameter to the constructor enhances flexibility. Ensure that this change is consistently applied throughout the codebase whereDMContextis instantiated. The default value ofnullptrmaintains backward compatibility.Verification successful
Integration of
scan_contextis consistent and backward compatible.The
scan_contextparameter is integrated into theDMContextconstructor with a default value ofnullptr, ensuring backward compatibility. Instantiations across the codebase either use this default or specify the parameter explicitly, confirming consistent integration. No issues found.
- Instances in
DeltaMergeStore.cppandDeltaMergeStore.hexplicitly usescan_context.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify all instantiations of `DMContext` use the new `scan_context` parameter. # Test: Search for instantiations of `DMContext`. Expect: Consistent use of `scan_context`. rg --type cpp -A 5 $'DMContext('Length of output: 104
Script:
#!/bin/bash # Correcting the regex pattern to search for instantiations of `DMContext`. rg --type cpp -A 5 'DMContext\('Length of output: 64207
dbms/src/Storages/DeltaMerge/File/ColumnStat.h (1)
44-44: Assess the impact of changingvector_indextype.Changing the type of
vector_indextostd::optional<dtpb::VectorIndexFileProps>may affect serialization and deserialization processes. Ensure that all related methods, such astoProtoandmergeFromProto, correctly handle the new type.Verification successful
Change to
std::optionalforvector_indexis correctly handled in serialization and deserialization.The
toProtoandmergeFromProtomethods inColumnStat.happropriately handle thestd::optional<dtpb::VectorIndexFileProps>type by checking for its presence and using the necessary protobuf methods. No further issues found.
- File:
dbms/src/Storages/DeltaMerge/File/ColumnStat.h- Methods:
toProto,mergeFromProtoScripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify correct handling of `vector_index` type change in serialization and deserialization methods. # Test: Search for usage of `vector_index` in related methods. Expect: Correct handling of new type. rg --type cpp -A 5 $'vector_index'Length of output: 30639
dbms/src/Storages/DeltaMerge/File/DMFileWriter.h (3)
57-57: LGTM: Updated parameter type inStreamconstructor.The change from
TiDB::VectorIndexInfoPtrtoTiDB::VectorIndexDefinitionPtrappears to align with a more structured approach to vector indexing.
75-75: LGTM: Updated member variable type inStream.Changing the
vector_indexmember variable toVectorIndexBuilderPtraligns with the use of a builder pattern, enhancing modularity and maintainability.
163-163: LGTM: Updated parameter type inaddStreamsmethod.The change from
TiDB::VectorIndexInfoPtrtoTiDB::VectorIndexDefinitionPtris consistent with the overall shift towards a more structured vector indexing approach.dbms/src/Storages/DeltaMerge/DeltaMergeDefines.h (2)
95-95: LGTM: Updated member variable type inColumnDefine.Changing the
vector_indexmember type toTiDB::VectorIndexDefinitionPtrindicates a refinement in vector index handling, potentially enhancing its definition and utilization.
102-102: LGTM: Updated constructor parameter type inColumnDefine.The update to
TiDB::VectorIndexDefinitionPtrfor thevector_index_parameter ensures consistency in theColumnDefinestructure's handling of vector indices.dbms/src/Storages/DeltaMerge/File/DMFileWithVectorIndexBlockInputStream.h (7)
94-94: LGTM: Constructor declaration only.The removal of the constructor implementation from the header file suggests a move towards better modularity by defining it in a separate source file.
96-96: LGTM: Destructor declaration only.The removal of the destructor implementation from the header file aligns with the approach of separating interface and implementation for improved modularity.
108-108: LGTM:readmethod declaration only.The change to only declare the
readmethod in the header file is consistent with the refactoring for better modularity and reduced compilation dependencies.
113-113: LGTM:readImplmethod declaration only.The change to only declare the
readImplmethod in the header file aligns with the strategy to separate interface and implementation for better modularity.
144-144: LGTM:readByIndexReadermethod declaration only.The change to only declare the
readByIndexReadermethod in the header file is consistent with the refactoring to separate interface and implementation.
148-148: LGTM:readByFollowingOtherColumnsmethod declaration only.The change to only declare the
readByFollowingOtherColumnsmethod in the header file aligns with the strategy to separate interface and implementation for better modularity.
182-182: LGTM: Updated member variable type.Changing the
vec_indexmember variable toVectorIndexViewerPtrsuggests a more efficient or flexible handling of vector indices, aligning with the design improvements.dbms/src/Storages/DeltaMerge/Index/VectorIndexHNSW/Index.cpp (7)
38-54: LGTM: Metric kind mapping is clear and robust.The
getUSearchMetricKindfunction effectively maps distance metrics and includes a runtime check for unsupported metrics.
58-64: LGTM: Constructor ensures correct initialization.The
VectorIndexHNSWBuilderconstructor correctly initializes the index with the specified metric kind and dimensions.
Line range hint
67-106: LGTM: Data addition to index is well-handled.The
addBlockmethod effectively manages data addition to the index, including checks for dimensions and nullable columns.
110-113: LGTM: Index saving is robust.The
savemethod correctly handles saving the index and includes a runtime check for errors.
116-129: LGTM: Viewer initialization is thorough.The
viewmethod properly initializes the index viewer and includes checks for index kind and distance metric.
Line range hint
134-199: LGTM: Search functionality is well-implemented.The
searchmethod includes comprehensive checks and exception handling, ensuring robust search operations.
202-205: LGTM: Data retrieval is straightforward.The
getmethod correctly retrieves data from the index, ensuring the output vector is appropriately sized.dbms/src/Common/LRUCache.h (1)
74-80: LGTM: Key existence check is efficient and thread-safe.The
containsmethod provides a non-intrusive way to verify key presence in the cache, ensuring thread safety.dbms/src/Storages/S3/FileCache.h (5)
75-76: LGTM: Synchronization for file readiness is well-managed.The
waitForNotEmptymethod effectively uses a condition variable to manage synchronization for file segment readiness.
228-234: LGTM: File download and eviction prevention are well-handled.The
downloadFileForLocalReadmethod ensures files are downloaded and guarded against eviction, enhancing cache reliability.
249-253: LGTM: Blocking file retrieval is effectively implemented.The
getOrWaitmethod enhances cache functionality by providing a blocking mechanism for file retrieval.
256-256: LGTM: Foreground download control is well-implemented.The
fgDownloadmethod provides controlled and synchronous file downloading, enhancing cache management.
273-273: LGTM: Enhanced locking control for file removal.The
removemethod's use of a unique lock provides more granular control over file removal operations.dbms/src/TiDB/Schema/TiDB.h (1)
209-209: Verify compatibility ofVectorIndexDefinitionPtr.The change from
VectorIndexInfoPtrtoVectorIndexDefinitionPtrindicates a shift in how vector indices are represented. Ensure that this change is compatible with other parts of the code that useColumnInfo.Verification successful
Change from
VectorIndexInfoPtrtoVectorIndexDefinitionPtris compatible.The
vector_indexmember inColumnInfohas been updated across various parts of the codebase, including runtime checks and JSON handling, ensuring compatibility with the new type. No issues were found in its integration.
- Files affected:
dbms/src/TiDB/Schema/TiDB.cppdbms/src/Storages/StorageDeltaMerge.cppdbms/src/Storages/DeltaMerge/DeltaMergeDefines.hScripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the usage of `ColumnInfo` struct to ensure compatibility with the new `vector_index` type. # Test: Search for all usages of `ColumnInfo` to check if they are compatible with `VectorIndexDefinitionPtr`. rg --type cpp 'ColumnInfo'Length of output: 52672
Script:
#!/bin/bash # Description: Search for interactions with the `vector_index` member in `ColumnInfo` to verify compatibility with `VectorIndexDefinitionPtr`. # Test: Look for usages of `vector_index` within the `ColumnInfo` struct. rg --type cpp 'vector_index'Length of output: 8746
dbms/src/Storages/DeltaMerge/File/DMFileWriter.cpp (2)
104-108: Verify logic withVectorIndexDefinitionPtr.The change in the parameter type from
VectorIndexInfoPtrtoVectorIndexDefinitionPtrsuggests a more structured approach to managing vector index definitions. Ensure that the logic within this method correctly handles this new type.Verification successful
Verified: Logic correctly handles
VectorIndexDefinitionPtr.The transition from
VectorIndexInfoPtrtoVectorIndexDefinitionPtrin theaddStreamsmethod is correctly handled. The logic aligns with the new parameter type, ensuring proper functionality.
- The
vector_indexis defined and used as aVectorIndexDefinitionPtr.- The
addStreamsmethod uses this type appropriately in its logic.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the logic in `addStreams` method to ensure it correctly handles `VectorIndexDefinitionPtr`. # Test: Search for all usages of `addStreams` to check if they are correctly updated with the new parameter type. rg --type cpp 'addStreams'Length of output: 387
Script:
#!/bin/bash # Search for the definition and usage of `cd.vector_index` to ensure it aligns with `VectorIndexDefinitionPtr`. rg --type cpp 'vector_index' -A 5Length of output: 30639
342-355: Verify vector index metadata handling.The logic for handling vector indices has been streamlined, improving clarity and efficiency. Ensure that the new logic correctly sets vector index metadata, especially with the use of
emplace.dbms/src/Storages/DeltaMerge/ScanContext.h (1)
93-93: Verify initialization oftotal_vector_idx_load_from_s3.The atomic variable
total_vector_idx_load_from_s3is initialized to zero. Ensure that this initialization aligns with its intended usage and that the change does not affect the logic.Verification successful
Initialization of
total_vector_idx_load_from_s3is appropriate.The atomic variable
total_vector_idx_load_from_s3is correctly initialized to zero, aligning with its usage in increment operations and test assertions. This initialization does not adversely affect the logic. The tests confirm the expected behavior.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the initialization and usage of `total_vector_idx_load_from_s3`. # Test: Search for all usages of `total_vector_idx_load_from_s3` to ensure the initialization aligns with its intended usage. rg --type cpp 'total_vector_idx_load_from_s3'Length of output: 2852
dbms/src/Storages/DeltaMerge/File/DMFileWithVectorIndexBlockInputStream.cpp (10)
34-81: Constructor is well-implemented.The constructor efficiently initializes member variables using move semantics and performs necessary runtime checks to ensure correct initialization.
83-103: Destructor is well-implemented.The destructor correctly logs performance metrics and updates the scan context only if
vec_column_readeris initialized.
106-123: Methodreadis well-implemented.The method efficiently handles filtering logic based on the
return_filterflag, ensuring data is processed correctly.
125-168: MethodreadImplis well-implemented.The method effectively uses filtering logic to optimize data retrieval, ensuring only relevant data is processed and returned.
170-215: MethodreadByIndexReaderis well-implemented.The method efficiently utilizes the pack filter to skip unnecessary data packs and reads data using the vector index, ensuring performance metrics are updated accurately.
217-255: MethodreadByFollowingOtherColumnsis well-implemented.The method ensures consistent data reading across columns and vector indices, using the header layout to assemble the block correctly.
258-271: Methodloadis well-implemented.The method efficiently manages the loading of vector index and search result data, ensuring it is only performed once and measuring the duration accurately.
273-384: MethodloadVectorIndexis well-implemented.The method handles vector index loading from both local and S3 storage with comprehensive error handling and performance tracking, ensuring robustness.
386-475: MethodloadVectorSearchResultis well-implemented.The method efficiently processes vector search results, updating performance metrics and optimizing data retrieval through the pack filter.
477-485: MethodgetPackIdFromBlockis well-implemented.The method correctly retrieves the pack ID by ensuring alignment with the block's start offset, using a map lookup efficiently.
dbms/src/Common/FailPoint.cpp (1)
109-109: Addition of fail pointfile_cache_fg_download_failis well-implemented.The new fail point is added consistently with the existing pattern, enhancing error simulation capabilities.
dbms/src/Interpreters/Context.h (1)
395-395: Verify the impact of parameter name change insetVectorIndexCache.The parameter name change from
cache_size_in_bytestocache_entitiessuggests a shift in semantics. Ensure that the method's usage throughout the codebase aligns with this change.Verification successful
Parameter Name Change Verified: The change in parameter name from
cache_size_in_bytestocache_entitiesin thesetVectorIndexCachemethod is consistently reflected across the codebase. The usage inServer.cppand the test filegtest_dm_vector_index.cppaligns with the new semantics. No further issues were found.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the usage of `setVectorIndexCache` to ensure it aligns with the new parameter semantics. # Test: Search for the method usage. Expect: Occurrences of the method with updated semantics. rg --type cpp --type hpp -A 2 $'setVectorIndexCache'Length of output: 79
Script:
#!/bin/bash # Description: Verify the usage of `setVectorIndexCache` to ensure it aligns with the new parameter semantics. # Test: Search for the method usage. Expect: Occurrences of the method with updated semantics. rg --type cpp --type h -A 2 $'setVectorIndexCache'Length of output: 1032
dbms/src/Storages/S3/FileCache.cpp (3)
160-176: LGTM!The logic for handling local file existence and cache inconsistencies is well implemented.
339-353: LGTM!The overloaded
removemethod with a unique lock parameter enhances thread safety effectively.
683-711: LGTM!The use of fail points for testing and the handling of exceptions are well implemented.
dbms/src/TiDB/Schema/TiDB.cpp (2)
410-416: LGTM!The use of
RUNTIME_CHECKandtipbnamespace functions for vector index serialization is well implemented.
474-496: LGTM!The parsing logic for vector index kinds and distance metrics is robust, and the dimension checks are appropriate.
dbms/src/Interpreters/Context.cpp (3)
57-57: Updated header inclusion toVectorIndexCache.h.The inclusion of
VectorIndex.hhas been replaced withVectorIndexCache.h, indicating a shift towards cache management for vector indices.
1409-1410: Simplified reset call indropVectorIndexCache.The reset call on the shared pointer has been simplified, enhancing code readability and consistency.
1391-1397: Parameter change insetVectorIndexCache.The parameter has been changed from
cache_size_in_bytestocache_entities, indicating a shift in focus from cache size to the number of entities. Ensure that this change aligns with the intended cache management strategy.dbms/src/Server/Server.cpp (3)
1442-1442: Switch to entity-based cache configuration.The configuration now uses
vec_index_cache_entitieswith a default of 1000, replacing the previous size-based configuration. This change might impact how memory is managed, potentially simplifying cache management by focusing on the number of entities rather than size.
1443-1443: Conditional check for cache configuration.The cache is configured only if
vec_index_cache_entitiesis non-zero, preventing unnecessary allocation. This is a standard practice to ensure efficient resource usage.
1444-1444: Set vector index cache based on entities.The
setVectorIndexCachemethod is called withvec_index_cache_entities, aligning with the new entity-based cache configuration. This change reflects the shift from size-based to entity-based cache management.dbms/src/Storages/DeltaMerge/tests/gtest_dm_vector_index.cpp (4)
1368-1385: Verify ANN Query Parameter Handling.The
computeNodeANNQuerymethod constructs an ANN query with provided parameters. Ensure that theref_vecandtop_kparameters are validated to prevent incorrect usage.
1427-1450: Verify Exception Handling for Disabled Cache.The
FileCacheNotEnabledtest should ensure that the system raises the correct exception when the file cache is disabled. Verify that the test covers all relevant scenarios.Verification successful
Verification Successful: Exception Handling in
FileCacheNotEnabledTest is Correct.The test correctly simulates the scenario where the file cache is disabled and verifies that the appropriate exception is raised. The exception message is checked to ensure it matches the expected output. No issues found.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify exception handling in `FileCacheNotEnabled` test. # Test: Search for exception handling in the test to ensure correctness. rg --type cpp 'TEST_F\(VectorIndexSegmentOnS3Test, FileCacheNotEnabled\)' -A 30Length of output: 2595
1818-1881: Ensure Correct Concurrency Simulation.The
ConcurrentDownloadFromS3test should accurately simulate concurrent downloads and verify correct behavior. Ensure that the sync points are correctly used to manage concurrency.Verification successful
Concurrency Simulation in
ConcurrentDownloadFromS3Test is Correct.The test accurately simulates concurrent S3 downloads using sync points to manage thread execution order, ensuring expected behavior is verified. The sync points "FileCache::fgDownload" and "before_FileSegment::waitForNotEmpty_wait" are correctly used to simulate concurrency. No issues found.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify concurrency simulation in `ConcurrentDownloadFromS3` test. # Test: Search for sync point usage to ensure concurrency is correctly simulated. rg --type cpp 'SyncPointCtl::enableInScope' -A 5Length of output: 32670
1207-1274: Ensure S3 Configuration and Cache Initialization.The
SetUpmethod initializes S3 configurations and sets up the file cache. Ensure that these configurations are correctly reset in theTearDownmethod to avoid side effects in other tests.Verification successful
S3 Configuration and Cache Cleanup Verified
The
TearDownmethod ingtest_dm_vector_index.cppcorrectly resets configurations and cleans up resources initialized in theSetUpmethod, ensuring no side effects on other tests.
- Disables fail points and shuts down caches.
- Resets global context configurations and deletes S3 resources.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that the `TearDown` method correctly resets configurations. # Test: Search for the `TearDown` method to ensure configurations are reset. rg --type cpp 'void TearDown\(\)' -A 30Length of output: 115864
| && sorted_results[results_it] >= pack_start // | ||
| && sorted_results[results_it] < pack_end) | ||
| { | ||
| offsets_in_pack.push_back(results[results_it] - pack_start); | ||
| offsets_in_pack.push_back(sorted_results[results_it] - pack_start); |
There was a problem hiding this comment.
Consider optimizing the loop logic.
The loop that processes sorted_results could potentially be optimized by using a binary search to find the starting index for each pack, rather than iterating through all elements. This would improve performance, especially for large datasets.
| // TODO: We could fill multiple rows if rowid is continuous. | ||
| VectorIndexViewer::Key rowid = pack_start_rowid[pack_id] + offset_in_pack; |
There was a problem hiding this comment.
Optimize row filling logic.
The TODO comment suggests optimizing the filling of multiple rows when rowid is continuous. Implementing this optimization can significantly improve performance by reducing the number of insert operations.
Would you like assistance in implementing this optimization?
| static void removeAllFileCache() | ||
| { | ||
| auto * file_cache = FileCache::instance(); | ||
| auto file_segments = file_cache->getAll(); | ||
| for (const auto & file_seg : file_cache->getAll()) | ||
| file_cache->remove(file_cache->toS3Key(file_seg->getLocalFileName()), true); | ||
|
|
||
| RUNTIME_CHECK(file_cache->getAll().empty()); | ||
| } |
There was a problem hiding this comment.
Ensure Robust Cache Clearing.
The removeAllFileCache method should handle potential errors during cache clearing. Consider adding error handling to ensure robustness.
void removeAllFileCache()
{
auto * file_cache = FileCache::instance();
try
{
for (const auto & file_seg : file_cache->getAll())
file_cache->remove(file_cache->toS3Key(file_seg->getLocalFileName()), true);
}
catch (const std::exception & e)
{
LOG_ERROR("Error clearing file cache: {}", e.what());
}
RUNTIME_CHECK(file_cache->getAll().empty());
}| FileSegmentPtr FileCache::getOrWait(const S3::S3FilenameView & s3_fname, const std::optional<UInt64> & filesize) | ||
| { | ||
| auto s3_key = s3_fname.toFullKey(); | ||
| auto file_type = getFileType(s3_key); | ||
| auto & table = tables[static_cast<UInt64>(file_type)]; | ||
|
|
||
| std::unique_lock lock(mtx); | ||
|
|
||
| auto f = table.get(s3_key); | ||
| if (f != nullptr) | ||
| { | ||
| lock.unlock(); | ||
| f->setLastAccessTime(std::chrono::system_clock::now()); | ||
| auto status = f->waitForNotEmpty(); | ||
| if (status == FileSegment::Status::Complete) | ||
| { | ||
| GET_METRIC(tiflash_storage_remote_cache, type_dtfile_hit).Increment(); | ||
| return f; | ||
| } | ||
| else | ||
| { | ||
| // On-going download failed, let the caller retry. | ||
| return nullptr; | ||
| } | ||
| } | ||
|
|
||
| GET_METRIC(tiflash_storage_remote_cache, type_dtfile_miss).Increment(); | ||
|
|
||
| auto estimzted_size = filesize ? *filesize : getEstimatedSizeOfFileType(file_type); | ||
| if (!reserveSpaceImpl(file_type, estimzted_size, /*try_evict*/ true)) | ||
| { | ||
| // Space not enough. | ||
| GET_METRIC(tiflash_storage_remote_cache, type_dtfile_full).Increment(); | ||
| LOG_DEBUG( | ||
| log, | ||
| "s3_key={} space not enough(capacity={} used={} estimzted_size={}), skip cache", | ||
| s3_key, | ||
| cache_capacity, | ||
| cache_used, | ||
| estimzted_size); | ||
|
|
||
| // Just throw, no need to let the caller retry. | ||
| throw Exception( // | ||
| ErrorCodes::S3_ERROR, | ||
| "Cannot reserve {} space for object {}", | ||
| estimzted_size, | ||
| s3_key); | ||
| return nullptr; | ||
| } | ||
|
|
||
| auto file_seg | ||
| = std::make_shared<FileSegment>(toLocalFilename(s3_key), FileSegment::Status::Empty, estimzted_size, file_type); | ||
| table.set(s3_key, file_seg); | ||
| lock.unlock(); | ||
|
|
||
| PerfContext::file_cache.fg_download_from_s3++; | ||
| fgDownload(lock, s3_key, file_seg); | ||
| if (!file_seg || !file_seg->isReadyToRead()) | ||
| throw Exception( // | ||
| ErrorCodes::S3_ERROR, | ||
| "Download object {} failed", | ||
| s3_key); | ||
|
|
||
| return file_seg; |
There was a problem hiding this comment.
Fix the typo in the variable name.
The variable name estimzted_size should be corrected to estimated_size.
- auto estimzted_size = filesize ? *filesize : getEstimatedSizeOfFileType(file_type);
+ auto estimated_size = filesize ? *filesize : getEstimatedSizeOfFileType(file_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.
| FileSegmentPtr FileCache::getOrWait(const S3::S3FilenameView & s3_fname, const std::optional<UInt64> & filesize) | |
| { | |
| auto s3_key = s3_fname.toFullKey(); | |
| auto file_type = getFileType(s3_key); | |
| auto & table = tables[static_cast<UInt64>(file_type)]; | |
| std::unique_lock lock(mtx); | |
| auto f = table.get(s3_key); | |
| if (f != nullptr) | |
| { | |
| lock.unlock(); | |
| f->setLastAccessTime(std::chrono::system_clock::now()); | |
| auto status = f->waitForNotEmpty(); | |
| if (status == FileSegment::Status::Complete) | |
| { | |
| GET_METRIC(tiflash_storage_remote_cache, type_dtfile_hit).Increment(); | |
| return f; | |
| } | |
| else | |
| { | |
| // On-going download failed, let the caller retry. | |
| return nullptr; | |
| } | |
| } | |
| GET_METRIC(tiflash_storage_remote_cache, type_dtfile_miss).Increment(); | |
| auto estimzted_size = filesize ? *filesize : getEstimatedSizeOfFileType(file_type); | |
| if (!reserveSpaceImpl(file_type, estimzted_size, /*try_evict*/ true)) | |
| { | |
| // Space not enough. | |
| GET_METRIC(tiflash_storage_remote_cache, type_dtfile_full).Increment(); | |
| LOG_DEBUG( | |
| log, | |
| "s3_key={} space not enough(capacity={} used={} estimzted_size={}), skip cache", | |
| s3_key, | |
| cache_capacity, | |
| cache_used, | |
| estimzted_size); | |
| // Just throw, no need to let the caller retry. | |
| throw Exception( // | |
| ErrorCodes::S3_ERROR, | |
| "Cannot reserve {} space for object {}", | |
| estimzted_size, | |
| s3_key); | |
| return nullptr; | |
| } | |
| auto file_seg | |
| = std::make_shared<FileSegment>(toLocalFilename(s3_key), FileSegment::Status::Empty, estimzted_size, file_type); | |
| table.set(s3_key, file_seg); | |
| lock.unlock(); | |
| PerfContext::file_cache.fg_download_from_s3++; | |
| fgDownload(lock, s3_key, file_seg); | |
| if (!file_seg || !file_seg->isReadyToRead()) | |
| throw Exception( // | |
| ErrorCodes::S3_ERROR, | |
| "Download object {} failed", | |
| s3_key); | |
| return file_seg; | |
| FileSegmentPtr FileCache::getOrWait(const S3::S3FilenameView & s3_fname, const std::optional<UInt64> & filesize) | |
| { | |
| auto s3_key = s3_fname.toFullKey(); | |
| auto file_type = getFileType(s3_key); | |
| auto & table = tables[static_cast<UInt64>(file_type)]; | |
| std::unique_lock lock(mtx); | |
| auto f = table.get(s3_key); | |
| if (f != nullptr) | |
| { | |
| lock.unlock(); | |
| f->setLastAccessTime(std::chrono::system_clock::now()); | |
| auto status = f->waitForNotEmpty(); | |
| if (status == FileSegment::Status::Complete) | |
| { | |
| GET_METRIC(tiflash_storage_remote_cache, type_dtfile_hit).Increment(); | |
| return f; | |
| } | |
| else | |
| { | |
| // On-going download failed, let the caller retry. | |
| return nullptr; | |
| } | |
| } | |
| GET_METRIC(tiflash_storage_remote_cache, type_dtfile_miss).Increment(); | |
| auto estimated_size = filesize ? *filesize : getEstimatedSizeOfFileType(file_type); | |
| if (!reserveSpaceImpl(file_type, estimated_size, /*try_evict*/ true)) | |
| { | |
| // Space not enough. | |
| GET_METRIC(tiflash_storage_remote_cache, type_dtfile_full).Increment(); | |
| LOG_DEBUG( | |
| log, | |
| "s3_key={} space not enough(capacity={} used={} estimated_size={}), skip cache", | |
| s3_key, | |
| cache_capacity, | |
| cache_used, | |
| estimated_size); | |
| // Just throw, no need to let the caller retry. | |
| throw Exception( // | |
| ErrorCodes::S3_ERROR, | |
| "Cannot reserve {} space for object {}", | |
| estimated_size, | |
| s3_key); | |
| return nullptr; | |
| } | |
| auto file_seg | |
| = std::make_shared<FileSegment>(toLocalFilename(s3_key), FileSegment::Status::Empty, estimated_size, file_type); | |
| table.set(s3_key, file_seg); | |
| lock.unlock(); | |
| PerfContext::file_cache.fg_download_from_s3++; | |
| fgDownload(lock, s3_key, file_seg); | |
| if (!file_seg || !file_seg->isReadyToRead()) | |
| throw Exception( // | |
| ErrorCodes::S3_ERROR, | |
| "Download object {} failed", | |
| s3_key); | |
| return file_seg; |
What problem does this PR solve?
Issue Number: close #xxx
Problem Summary:
What is changed and how it works?
Check List
Tests
Side effects
Documentation
Release note
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Refactor
Tests