Copilot commented on code in PR #948:
URL:
https://github.com/apache/skywalking-banyandb/pull/948#discussion_r2706373281
##########
banyand/internal/sidx/query.go:
##########
@@ -161,7 +161,7 @@ func (s *sidx) prepareStreamingResources(
minKey, maxKey := extractKeyRange(req)
asc := extractOrdering(req)
- parts := selectPartsForQuery(snap, minKey, maxKey)
+ parts := selectPartsForQuery(snap, minKey, maxKey, nil, nil)
Review Comment:
Time range filtering is hardcoded to nil, nil which means timestamp-based
part selection optimization is not being utilized. To enable the time-based
query optimization that this PR introduces, QueryRequest needs MinTimestamp and
MaxTimestamp fields that can be passed to selectPartsForQuery.
```suggestion
parts := selectPartsForQuery(snap, minKey, maxKey, req.MinTimestamp,
req.MaxTimestamp)
```
##########
banyand/internal/sidx/part.go:
##########
@@ -754,11 +754,19 @@ func (mp *memPart) mustInitFromElements(es *elements) {
if (i-blockStart) > maxBlockLength || accumulatedSize >=
maxUncompressedBlockSize || i == len(es.seriesIDs) || seriesChanged {
// Extract elements for current series
seriesUserKeys := es.userKeys[blockStart:i]
+ // Ensure timestamps slice has the same length as other
slices
+ var seriesTimestamps []int64
+ if len(es.timestamps) >= i {
+ seriesTimestamps = es.timestamps[blockStart:i]
Review Comment:
The condition `len(es.timestamps) >= i` is incorrect. The variable `i` is
used as an exclusive end index for slicing (line 760), so the correct condition
should be `len(es.timestamps) > blockStart` or `len(es.timestamps) >= i` should
be changed to check against blockStart. As written, when timestamps are
present, the fallback path will be incorrectly taken in many cases. For
example, if there are 10 timestamps (indices 0-9) and i=10, the condition fails
even though timestamps[0:10] would be valid.
```suggestion
if len(es.timestamps) > blockStart {
// Use available timestamps starting from
blockStart, up to either i or the end of timestamps.
upper := i
if upper > len(es.timestamps) {
upper = len(es.timestamps)
}
seriesTimestamps =
es.timestamps[blockStart:upper]
// If we don't have enough timestamps to cover
the whole block, pad with zeros
if len(seriesTimestamps) < i-blockStart {
padded := make([]int64, i-blockStart)
copy(padded, seriesTimestamps)
seriesTimestamps = padded
}
```
##########
banyand/internal/sidx/scan_query.go:
##########
@@ -73,9 +73,21 @@ func (s *sidx) ScanQuery(ctx context.Context, req
ScanQueryRequest) ([]*QueryRes
Tags: tagsMap,
}
- // Scan all parts
- totalParts := len(snap.parts)
- for partIdx, pw := range snap.parts {
+ // Filter parts by key and time range
+ var filteredParts []*partWrapper
+ for _, pw := range snap.parts {
+ if !pw.overlapsKeyRange(minKey, maxKey) {
+ continue
+ }
+ if !pw.overlapsTimeRange(nil, nil) {
Review Comment:
The time range filtering is hardcoded to pass nil, nil which means no
timestamp filtering will be applied during scan queries. This defeats the
purpose of adding timestamp range support. The ScanQueryRequest should include
MinTimestamp and MaxTimestamp fields that can be passed here to enable
time-based filtering optimization.
```suggestion
if !pw.overlapsTimeRange(req.MinTimestamp, req.MaxTimestamp) {
```
--
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]
