Copilot commented on code in PR #13665:
URL: https://github.com/apache/skywalking/pull/13665#discussion_r2688668324
##########
docs/en/changes/changes.md:
##########
@@ -20,6 +20,7 @@
* Remove `bydb.dependencies.properties` and set the compatible BanyanDB API
version number in `${SW_STORAGE_BANYANDB_COMPATIBLE_SERVER_API_VERSIONS}`
* Fix trace profiling query time range condition.
* Fix BanyanDB time range overflow in profile thread snapshot query.
+* MQE: fix multiple labeled metric query and make it won't get result if no
label value compose is match.
Review Comment:
Grammar issue: "make it won't get result" should be "make it return no
results" or "ensure no results are returned".
```suggestion
* MQE: fix multiple labeled metric query and ensure no results are returned
if no label value combinations match.
```
##########
oap-server/server-core/src/main/java/org/apache/skywalking/oap/server/core/storage/query/IMetricsQueryDAO.java:
##########
@@ -191,6 +191,8 @@ public static List<String> composeLabelConditions(final
List<KeyValue> queryLabe
});
if (!keySet.isEmpty()) {
keySets.add(keySet);
+ } else { // only all query labels match, can get result
Review Comment:
The comment "only all query labels match, can get result" is unclear.
Consider rephrasing to better explain the logic, such as: "If any query label
has no matches, no results should be returned" or "Clear all keySets when a
query label finds no matches to ensure empty result".
```suggestion
} else { // If any query label has no matches, clear all
keySets so that no results are returned
```
##########
oap-server/server-core/src/main/java/org/apache/skywalking/oap/server/core/storage/query/IMetricsQueryDAO.java:
##########
@@ -191,6 +191,8 @@ public static List<String> composeLabelConditions(final
List<KeyValue> queryLabe
});
if (!keySet.isEmpty()) {
keySets.add(keySet);
+ } else { // only all query labels match, can get result
+ keySets.clear();
}
Review Comment:
This fix addresses multiple labeled metric queries (e.g., querying for
status=200 AND region=us), but the existing tests only cover single-label-key
scenarios (e.g., status=200 OR status=400). Consider adding test cases that
cover:
1. Multiple label keys where all match (e.g., status=200 AND region=us, both
present in data)
2. Multiple label keys where one doesn't match (e.g., status=200 AND
region=eu, where region=eu is not in data) - should return empty results
3. Multiple label keys where none match - should return empty results
This is especially important given the critical bug in the current
implementation (missing break statement after keySets.clear()).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
