Copilot commented on code in PR #813:
URL:
https://github.com/apache/skywalking-banyandb/pull/813#discussion_r2443372634
##########
banyand/trace/snapshot.go:
##########
@@ -59,14 +59,28 @@ type snapshot struct {
ref int32
}
-func (s *snapshot) getParts(dst []*part, minTimestamp int64, maxTimestamp
int64) ([]*part, int) {
+func (s *snapshot) getParts(dst []*part, minTimestamp int64, maxTimestamp
int64, traceIDs []string) ([]*part, int) {
+ shouldSkip := func(p *part) bool {
+ if p.traceIDFilter.filter == nil {
+ return false
+ }
+ for _, traceID := range traceIDs {
+ if p.traceIDFilter.filter.MightContain([]byte(traceID))
{
+ return false
+ }
+ }
+ return true
+ }
+
Review Comment:
When traceIDs is nil or empty, this logic will skip all parts that have a
non-nil filter, effectively filtering everything out. If no traceIDs are
provided, getParts should not apply filtering and must include all
timestamp-matching parts. Suggestion: treat len(traceIDs) == 0 as no filtering
by changing the guard to `if p.traceIDFilter.filter == nil || len(traceIDs) ==
0 { return false }`.
##########
banyand/trace/snapshot.go:
##########
@@ -59,14 +59,28 @@ type snapshot struct {
ref int32
}
-func (s *snapshot) getParts(dst []*part, minTimestamp int64, maxTimestamp
int64) ([]*part, int) {
+func (s *snapshot) getParts(dst []*part, minTimestamp int64, maxTimestamp
int64, traceIDs []string) ([]*part, int) {
+ shouldSkip := func(p *part) bool {
+ if p.traceIDFilter.filter == nil {
+ return false
+ }
+ for _, traceID := range traceIDs {
+ if p.traceIDFilter.filter.MightContain([]byte(traceID))
{
Review Comment:
[nitpick] Converting each traceID to []byte inside the per-part loop
allocates repeatedly and scales with (parts × traceIDs). Precompute a [][]byte
of traceIDs once before iterating parts (or before defining shouldSkip) and
reuse the byte slices to avoid redundant allocations.
```suggestion
// Precompute [][]byte of traceIDs to avoid repeated allocations
traceIDBytes := make([][]byte, len(traceIDs))
for i, traceID := range traceIDs {
traceIDBytes[i] = []byte(traceID)
}
shouldSkip := func(p *part) bool {
if p.traceIDFilter.filter == nil {
return false
}
for _, tid := range traceIDBytes {
if p.traceIDFilter.filter.MightContain(tid) {
```
--
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]
