wu-sheng commented on code in PR #10448:
URL: https://github.com/apache/skywalking/pull/10448#discussion_r1119459445
##########
oap-server/server-storage-plugin/storage-banyandb-plugin/src/main/java/org/apache/skywalking/oap/server/storage/plugin/banyandb/BanyanDBAggregationQueryDAO.java:
##########
@@ -85,24 +60,45 @@ protected void apply(MeasureQuery query) {
throw new IOException("field spec is not registered");
}
+ if (schema.getTopNSpec() == null) {
+ throw new IOException("TopN spec is registered");
+ }
+
+ TopNQueryResponse resp = null;
+ if (condition.getOrder() == Order.DES) {
+ resp = topN(schema, timestampRange, condition.getTopN(),
additionalConditions);
+ } else {
+ resp = bottomN(schema, timestampRange, condition.getTopN(),
additionalConditions);
+ }
Review Comment:
@lujiajing1126 I did a discussion with @hanahmily
I think the server side is clear, and the thing is you seem mixed query
groupBy condition and topN pre-aggregation GroupBy condition.
In the current case, we should have two TopN definition per metric
1. Global TopN
2. Service-ID oriented groupBy TopN
Notice, the entityID should never be involved pre-aggregation TopN. That ID
is the groupBy condition in the query stage only about how to merge data cross
bucket. But pre-aggregation is bucket-based.
I mentioned to @hanahmily, in theory <1> could be built through all <2>'s
TopN by costing some extra CPU. Considering <1> oriented query is supported but
not widely used, we could enhance the server side to build <1> in the query
stage only. You could discuss with Gao to see whether we could do this.
--
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]
