Properly order the metrics results#807
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR changes the C++ metrics API from returning results in std::map containers (keyed by string representations of IDs) to returning results in ordered std::vector containers. This addresses a sorting issue where string-based comparison of IDs was inconsistent with integer-based database ordering, particularly problematic for paginated results.
- Replaced
std::mapreturn types withstd::vectorof structured entry types - Added explicit
ORDER BYclauses to database queries for consistent ordering - Introduced new Thrift struct types to support the vector-based API
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| cxxmetrics.thrift | Defines new entry struct types for vector-based API |
| cppmetricsservice.h | Updates method signatures to use vector return types |
| cppmetricsservice.cpp | Implements vector-based logic with proper ordering |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| // Try to find existing entry with same astNodeId | ||
| auto it = std::find_if(result_.begin(), result_.end(), | ||
| [&](const CppMetricsAstNodeEntry& entry) { | ||
| return std::stoull(entry.astNodeId) == node.astNodeId; |
There was a problem hiding this comment.
Converting string to unsigned long long for every comparison creates a performance bottleneck. Consider storing the numeric ID alongside the string ID in the entry struct to avoid repeated conversions.
| // Try to find existing entry with same astNodeId | ||
| auto it = std::find_if(result_.begin(), result_.end(), | ||
| [&](const CppMetricsAstNodeDetailedEntry& entry) { | ||
| return std::stoull(entry.astNodeId) == node.astNodeId; |
There was a problem hiding this comment.
Converting string to unsigned long long for every comparison creates a performance bottleneck. Consider storing the numeric ID alongside the string ID in the entry struct to avoid repeated conversions.
| return std::stoull(entry.astNodeId) == node.astNodeId; | |
| return entry.astNodeIdNum == node.astNodeId; |
| // Try to find existing entry with same fileId | ||
| auto it = std::find_if(result_.begin(), result_.end(), | ||
| [&](const CppMetricsModuleEntry& entry) { | ||
| return std::stoull(entry.fileId) == file.fileId; |
There was a problem hiding this comment.
Converting string to unsigned long long for every comparison creates a performance bottleneck. Consider storing the numeric ID alongside the string ID in the entry struct to avoid repeated conversions.
| return std::stoull(entry.fileId) == file.fileId; | |
| return entry.fileIdNumeric == file.fileId; |
plugins/cpp_metrics/service/include/service/cppmetricsservice.h
Outdated
Show resolved
Hide resolved
75ac188 to
a392e4a
Compare
C++ metrics results are not properly sorted, since currently the results are returned in a
std::map, whose key is the ASTNode ID (or File ID), interpreted as strings. However, these IDs are stored as signed integers in the database, while in the CodeCompass codebase they are unsigned integers. Their comparison should therefore be performed as integers (because the database engine also does this for theORDER BYclause), otherwise it is not certain that the last element will be the largest ID in the given result set. This is especially troubling for paginated results.