Copilot commented on code in PR #873:
URL:
https://github.com/apache/skywalking-banyandb/pull/873#discussion_r2623630272
##########
banyand/measure/topn.go:
##########
@@ -577,9 +579,10 @@ func (manager *topNProcessorManager) buildMapper(fieldName
string, groupByNames
}
}, nil
}
- groupLocator, err := newGroupLocator(manager.m, groupByNames)
- if err != nil {
- return nil, err
+ groupLocator, removedTags := newGroupLocator(manager.m, groupByNames)
+ for _, removedTag := range removedTags {
+ manager.l.Warn().Str("tagName", removedTag).Str("measure",
manager.m.Metadata.GetName()).
+ Msg("TopNAggregation references removed tag which no
longer exists in schema, ignoring this tag")
}
Review Comment:
The warning message is logged for each removed tag, but the TopNAggregation
may not function correctly if required groupBy tags are missing. Consider
logging a single warning that includes all removed tags and potentially
returning an error if all groupBy tags were removed, as this would make the
aggregation invalid.
```suggestion
if len(removedTags) > 0 {
manager.l.Warn().
Strs("removedTags", removedTags).
Str("measure", manager.m.Metadata.GetName()).
Msg("TopNAggregation references removed tag(s) which no
longer exist in schema, ignoring these tags")
}
if len(groupLocator) == 0 {
return nil, fmt.Errorf("all groupBy tags were removed for
TopNAggregation in measure %s, aggregation is invalid",
manager.m.Metadata.GetName())
}
```
##########
pkg/query/logical/stream/stream_plan_indexscan_local.go:
##########
@@ -147,15 +148,34 @@ func BuildElementsFromStreamResult(ctx context.Context,
result model.StreamQuery
ElementId:
hex.EncodeToString(convert.Uint64ToBytes(r.ElementIDs[i])),
}
- for _, tf := range r.TagFamilies {
+ for _, proj := range projectionTags {
tagFamily := &modelv1.TagFamily{
- Name: tf.Name,
+ Name: proj.Family,
}
e.TagFamilies = append(e.TagFamilies, tagFamily)
- for _, t := range tf.Tags {
+ var resultTagFamily *model.TagFamily
+ for idx := range r.TagFamilies {
+ if r.TagFamilies[idx].Name == proj.Family {
+ resultTagFamily = &r.TagFamilies[idx]
+ break
+ }
+ }
+ for _, tagName := range proj.Names {
Review Comment:
The inner loop searching for resultTagFamily (lines 157-162) is executed for
every projection tag family on every iteration of the outer loop. Consider
building a map of tag family names to tag families once before the outer loop
to avoid O(n*m) complexity.
##########
pkg/query/logical/measure/measure_plan_indexscan_local.go:
##########
@@ -307,26 +311,60 @@ func (ei *resultMIterator) Next() bool {
Version: r.Versions[i],
}
- for _, tf := range r.TagFamilies {
+ for _, proj := range ei.projectionTags {
tagFamily := &modelv1.TagFamily{
- Name: tf.Name,
+ Name: proj.Family,
}
dp.TagFamilies = append(dp.TagFamilies, tagFamily)
- for _, t := range tf.Tags {
+ var resultTagFamily *model.TagFamily
+ for idx := range r.TagFamilies {
+ if r.TagFamilies[idx].Name == proj.Family {
+ resultTagFamily = &r.TagFamilies[idx]
+ break
+ }
+ }
+ for _, tagName := range proj.Names {
Review Comment:
The inner loop searching for resultTagFamily (lines 320-325) is executed for
every projection tag family on every data point iteration. Consider building a
map of tag family names to tag families once before the outer loop to avoid
O(n*m) complexity.
--
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]
