wu-sheng commented on code in PR #10448:
URL: https://github.com/apache/skywalking/pull/10448#discussion_r1174687593
##########
oap-server/server-storage-plugin/storage-banyandb-plugin/src/main/java/org/apache/skywalking/oap/server/storage/plugin/banyandb/BanyanDBAggregationQueryDAO.java:
##########
@@ -64,6 +63,13 @@ public List<SelectedRecord> sortMetrics(TopNCondition
condition, String valueCol
// BanyanDB server-side TopN support for metrics pre-aggregation.
if (schema.getTopNSpec() != null) {
+ // check if additional conditions are registered
+ if (CollectionUtils.isNotEmpty(additionalConditions)) {
+ final Set<String> additionalConditionNames =
additionalConditions.stream().map(KeyValue::getKey).collect(Collectors.toSet());
+ if
(!ImmutableSet.copyOf(schema.getTopNSpec().getGroupByTagNames()).containsAll(additionalConditionNames))
{
+ throw new IOException("[" +
Strings.join(additionalConditionNames, ',') + "] conditions are not registered
as groupBy tags");
Review Comment:
Two questions,
1. I think group-by pre-aggregation should be `equal` or `no condition`,
rather than `contains`? I know that we have one condition, so, contains means
extractly the same. But I am trying to be precise.
2. When this pre-aggregation doesn't fit the current query, I think failing
back to on-demand group-by still works. We don't have to through exception,
right?
--
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: notifications-unsubscr...@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
