You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@skywalking.apache.org by "hanahmily (via GitHub)" <gi...@apache.org> on 2026/03/21 01:48:51 UTC
[PR] feat(sidx): add optional timestamp support for part selection and validation [skywalking-banyandb]
hanahmily opened a new pull request, #1019:
URL: https://github.com/apache/skywalking-banyandb/pull/1019
## Summary
This PR enhances SIDX (secondary index) with optional timestamp support for efficient part selection and validation:
- Add timestamp-based part selection to improve query performance by pruning irrelevant parts early
- Add timestamp validation to ensure data freshness and correctness
- Fix nil pointer panic in segment collectMetrics during shutdown (race condition fix)
## Test plan
- [x] Run CI checks locally (`make check`, `make test-ci`)
- [x] Integration tests pass (standalone and distributed)
## Related Issue
Milestone 0.10.0
🤖 Generated with [Claude Code](https://claude.com/claude-code)
--
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-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(sidx): add optional timestamp support for part selection and validation [skywalking-banyandb]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #1019:
URL: https://github.com/apache/skywalking-banyandb/pull/1019#discussion_r2972320083
##########
.github/workflows/test-fodc-e2e.yml:
##########
@@ -60,10 +60,11 @@ jobs:
download-artifacts: 'true'
setup-docker: 'true'
- - name: Setup kubectl
- uses: azure/setup-kubectl@v4
- with:
- version: v1.30.0
+ - name: Install kubectl
+ run: |
+ mkdir -p "${HOME}/.local/bin"
+ bash test/e2e-v2/script/prepare/setup-e2e-shell/install-kubectl.sh /tmp "${HOME}/.local/bin" v1.30.0
Review Comment:
The workflow passes `v1.30.0` as a third argument to `install-kubectl.sh`, but the script currently ignores positional arg #3 and only reads `K8SVERSION` from the environment. As a result, this step will still install the script’s default kubectl version. Set `K8SVERSION=v1.30.0` (or update the script to accept the version argument) so the intended version is installed.
```suggestion
K8SVERSION=v1.30.0 bash test/e2e-v2/script/prepare/setup-e2e-shell/install-kubectl.sh /tmp "${HOME}/.local/bin"
```
##########
banyand/internal/sidx/query.go:
##########
@@ -484,15 +483,17 @@ func extractOrdering(req QueryRequest) bool {
return req.Order.Sort != modelv1.Sort_SORT_DESC
}
-// selectPartsForQuery selects relevant parts from snapshot based on key range.
-func selectPartsForQuery(snap *Snapshot, minKey, maxKey int64) []*part {
+// selectPartsForQuery selects relevant parts from snapshot based on key range and optional timestamp range.
+func selectPartsForQuery(snap *Snapshot, minKey, maxKey int64, minTS, maxTS *int64) []*part {
var selectedParts []*part
-
for _, pw := range snap.parts {
- if pw.overlapsKeyRange(minKey, maxKey) {
- selectedParts = append(selectedParts, pw.p)
+ if !pw.overlapsKeyRange(minKey, maxKey) {
+ continue
}
+ if minTS != nil && maxTS != nil && !pw.overlapsTimestampRange(*minTS, *maxTS) {
+ continue
Review Comment:
Timestamp pruning is only applied when *both* `minTS` and `maxTS` are non-nil. If a caller supplies only one bound (open-ended range), part selection will ignore timestamps entirely and may scan/query irrelevant parts. Consider supporting one-sided ranges (e.g., if `minTS != nil` then require `part.MaxTimestamp >= *minTS`; if `maxTS != nil` then require `part.MinTimestamp <= *maxTS`).
```suggestion
switch {
case minTS != nil && maxTS != nil:
if !pw.overlapsTimestampRange(*minTS, *maxTS) {
continue
}
case minTS != nil:
if !pw.overlapsTimestampRange(*minTS, math.MaxInt64) {
continue
}
case maxTS != nil:
if !pw.overlapsTimestampRange(math.MinInt64, *maxTS) {
continue
}
```
##########
banyand/internal/sidx/sidx.go:
##########
@@ -100,6 +100,8 @@ func (s *sidx) ConvertToMemPart(reqs []WriteRequest, segmentID int64) (*MemPart,
mp := GenerateMemPart()
mp.mustInitFromElements(es)
mp.partMetadata.SegmentID = segmentID
+ mp.partMetadata.MinTimestamp = minTimestamp
+ mp.partMetadata.MaxTimestamp = maxTimestamp
return mp, nil
Review Comment:
`ConvertToMemPart` now accepts optional `minTimestamp/maxTimestamp` but doesn’t validate their relationship. If callers accidentally pass `minTimestamp > maxTimestamp`, the invalid manifest can be written (marshal doesn’t call `partMetadata.validate()`), and later opens/reads will fail validation. Add a check here (or in `mustWriteMetadata`/`marshal`) to reject invalid timestamp ranges early.
##########
banyand/internal/sidx/scan_query.go:
##########
@@ -102,6 +105,21 @@ func (s *sidx) ScanQuery(ctx context.Context, req ScanQueryRequest) ([]*QueryRes
return results, nil
}
+// selectPartsForScan selects parts that overlap key and optional timestamp range for scan queries.
+func selectPartsForScan(snap *Snapshot, minKey, maxKey int64, minTS, maxTS *int64) []*partWrapper {
+ var selected []*partWrapper
+ for _, pw := range snap.parts {
+ if !pw.overlapsKeyRange(minKey, maxKey) {
+ continue
+ }
+ if minTS != nil && maxTS != nil && !pw.overlapsTimestampRange(*minTS, *maxTS) {
+ continue
Review Comment:
Timestamp pruning is only applied when *both* `minTS` and `maxTS` are non-nil. If a caller supplies only one bound (open-ended range), scans will ignore timestamps entirely. Consider supporting one-sided timestamp ranges in `selectPartsForScan` similarly to key range handling.
```suggestion
// Apply timestamp pruning when either bound is provided; treat missing bounds as open-ended.
if minTS != nil || maxTS != nil {
from := int64(math.MinInt64)
to := int64(math.MaxInt64)
if minTS != nil {
from = *minTS
}
if maxTS != nil {
to = *maxTS
}
if !pw.overlapsTimestampRange(from, to) {
continue
}
```
##########
banyand/internal/sidx/timestamp_test.go:
##########
@@ -0,0 +1,394 @@
+// Licensed to Apache Software Foundation (ASF) under one or more contributor
+// license agreements. See the NOTICE file distributed with
+// this work for additional information regarding copyright
+// ownership. Apache Software Foundation (ASF) licenses this file to you under
+// the Apache License, Version 2.0 (the "License"); you may
+// not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+package sidx
+
+import (
+ "context"
+ "encoding/json"
+ "path/filepath"
+ "testing"
+
+ "github.com/stretchr/testify/assert"
+ "github.com/stretchr/testify/require"
+
+ "github.com/apache/skywalking-banyandb/api/common"
+ "github.com/apache/skywalking-banyandb/banyand/observability"
+ "github.com/apache/skywalking-banyandb/banyand/protector"
+ "github.com/apache/skywalking-banyandb/pkg/fs"
+)
+
+// writeTestDataWithTimeRange writes test data with optional min/max timestamps (back-compat: nil means no timestamps).
+func writeTestDataWithTimeRange(t *testing.T, sidx SIDX, reqs []WriteRequest, segmentID int64, partID uint64, minTS, maxTS *int64) {
+ t.Helper()
+ memPart, err := sidx.ConvertToMemPart(reqs, segmentID, minTS, maxTS)
+ require.NoError(t, err)
+ require.NotNil(t, memPart)
+ sidx.IntroduceMemPart(partID, memPart)
+}
+
+func ptrInt64Ts(v int64) *int64 {
+ val := v
+ return &val
+}
+
+// TestConvertToMemPart_Timestamps verifies ConvertToMemPart with and without timestamps.
+func TestConvertToMemPart_Timestamps(t *testing.T) {
+ sidx := createTestSIDX(t)
+ defer func() {
+ assert.NoError(t, sidx.Close())
+ }()
+
+ reqs := []WriteRequest{
+ createTestWriteRequest(1, 1000, "data1"),
+ createTestWriteRequest(1, 2000, "data2"),
+ }
+
+ t.Run("nil_timestamps_back_compat", func(t *testing.T) {
+ mp, err := sidx.ConvertToMemPart(reqs, 1, nil, nil)
+ require.NoError(t, err)
+ require.NotNil(t, mp)
+ require.Nil(t, mp.partMetadata.MinTimestamp)
+ require.Nil(t, mp.partMetadata.MaxTimestamp)
+ ReleaseMemPart(mp)
+ })
+
+ t.Run("with_timestamps", func(t *testing.T) {
+ minTS := int64(500)
+ maxTS := int64(2500)
+ mp, err := sidx.ConvertToMemPart(reqs, 1, &minTS, &maxTS)
+ require.NoError(t, err)
+ require.NotNil(t, mp)
+ require.NotNil(t, mp.partMetadata.MinTimestamp)
+ require.NotNil(t, mp.partMetadata.MaxTimestamp)
+ assert.Equal(t, int64(500), *mp.partMetadata.MinTimestamp)
+ assert.Equal(t, int64(2500), *mp.partMetadata.MaxTimestamp)
+ ReleaseMemPart(mp)
+ })
+}
+
+// TestManifest_BackCompat verifies manifest.json omits timestamps when nil (back-compat).
+func TestManifest_BackCompat(t *testing.T) {
+ dir := t.TempDir()
+ fileSystem := fs.NewLocalFileSystem()
+ opts := NewDefaultOptions()
+ opts.Memory = protector.NewMemory(observability.NewBypassRegistry())
+ opts.Path = dir
+
+ sidxIface, err := NewSIDX(fileSystem, opts)
+ require.NoError(t, err)
+ raw := sidxIface.(*sidx)
+ defer func() {
+ assert.NoError(t, raw.Close())
+ }()
+
+ reqs := []WriteRequest{
+ createTestWriteRequest(1, 100, "data"),
+ }
+
+ t.Run("manifest_without_timestamps", func(t *testing.T) {
+ writeTestDataWithTimeRange(t, raw, reqs, 1, 1, nil, nil)
+ flushIntro, err := raw.Flush(map[uint64]struct{}{1: {}})
+ require.NoError(t, err)
+ raw.IntroduceFlushed(flushIntro)
+ flushIntro.Release()
+
+ manifestPath := filepath.Join(dir, "0000000000000001", manifestFilename)
+ data, readErr := fileSystem.Read(manifestPath)
+ require.NoError(t, readErr)
+
+ var m struct {
+ MinTimestamp *int64 `json:"minTimestamp,omitempty"`
+ MaxTimestamp *int64 `json:"maxTimestamp,omitempty"`
+ MinKey int64 `json:"minKey"`
+ MaxKey int64 `json:"maxKey"`
+ }
+ require.NoError(t, json.Unmarshal(data, &m))
+ require.Nil(t, m.MinTimestamp)
+ require.Nil(t, m.MaxTimestamp)
+ assert.Equal(t, int64(100), m.MinKey)
+ assert.Equal(t, int64(100), m.MaxKey)
+ })
+
+ t.Run("manifest_with_timestamps", func(t *testing.T) {
+ minTS := int64(1000)
+ maxTS := int64(2000)
+ writeTestDataWithTimeRange(t, raw, reqs, 2, 2, &minTS, &maxTS)
+ flushIntro, err := raw.Flush(map[uint64]struct{}{2: {}})
+ require.NoError(t, err)
+ raw.IntroduceFlushed(flushIntro)
+ flushIntro.Release()
+
+ manifestPath := filepath.Join(dir, "0000000000000002", manifestFilename)
+ data, readErr := fileSystem.Read(manifestPath)
+ require.NoError(t, readErr)
+
+ var m struct {
+ MinTimestamp *int64 `json:"minTimestamp,omitempty"`
+ MaxTimestamp *int64 `json:"maxTimestamp,omitempty"`
+ }
+ require.NoError(t, json.Unmarshal(data, &m))
+ require.NotNil(t, m.MinTimestamp)
+ require.NotNil(t, m.MaxTimestamp)
+ assert.Equal(t, int64(1000), *m.MinTimestamp)
+ assert.Equal(t, int64(2000), *m.MaxTimestamp)
+ })
+}
+
+// TestQuery_TimestampSelection verifies timestamp-aware part selection and key-range fallback.
+func TestQuery_TimestampSelection(t *testing.T) {
+ reqs := []WriteRequest{
+ createTestWriteRequest(1, 1000, "data1000"),
+ createTestWriteRequest(1, 2000, "data2000"),
+ }
+
+ t.Run("back_compat_key_range_only", func(t *testing.T) {
+ sidx := createTestSIDX(t)
+ defer func() {
+ assert.NoError(t, sidx.Close())
+ }()
+ writeTestDataWithTimeRange(t, sidx, reqs, 1, 1, nil, nil)
+ waitForIntroducerLoop()
+
+ minKey := int64(500)
+ maxKey := int64(2500)
+ qr := QueryRequest{
+ SeriesIDs: []common.SeriesID{1},
+ MinKey: &minKey,
+ MaxKey: &maxKey,
+ }
+ resultsCh, errCh := sidx.StreamingQuery(context.Background(), qr)
+ var keys []int64
+ for res := range resultsCh {
+ require.NoError(t, res.Error)
+ keys = append(keys, res.Keys...)
+ }
+ select {
+ case err, ok := <-errCh:
+ if ok {
+ require.NoError(t, err)
+ }
+ default:
+ }
+ assert.GreaterOrEqual(t, len(keys), 0, "key-range filter should work without timestamps")
+ })
+
+ t.Run("timestamp_filter_overlaps", func(t *testing.T) {
+ sidx := createTestSIDX(t)
+ defer func() {
+ assert.NoError(t, sidx.Close())
+ }()
+ minTS := ptrInt64Ts(1000)
+ maxTS := ptrInt64Ts(3000)
+ writeTestDataWithTimeRange(t, sidx, reqs, 1, 1, minTS, maxTS)
+ waitForIntroducerLoop()
+
+ qMinTS := ptrInt64Ts(1500)
+ qMaxTS := ptrInt64Ts(2500)
+ qr := QueryRequest{
+ SeriesIDs: []common.SeriesID{1},
+ MinTimestamp: qMinTS,
+ MaxTimestamp: qMaxTS,
+ }
+ resultsCh, errCh := sidx.StreamingQuery(context.Background(), qr)
+ var count int
+ for res := range resultsCh {
+ require.NoError(t, res.Error)
+ count += res.Len()
+ }
+ select {
+ case err, ok := <-errCh:
+ if ok {
+ require.NoError(t, err)
+ }
+ default:
+ }
+ assert.GreaterOrEqual(t, count, 0, "query with overlapping timestamp range")
+ })
+
+ t.Run("timestamp_filter_no_overlap_excludes_part", func(t *testing.T) {
+ sidx := createTestSIDX(t)
+ defer func() {
+ assert.NoError(t, sidx.Close())
+ }()
+ minTS := ptrInt64Ts(1000)
+ maxTS := ptrInt64Ts(2000)
+ writeTestDataWithTimeRange(t, sidx, reqs, 1, 1, minTS, maxTS)
+ waitForIntroducerLoop()
+
+ qMinTS := ptrInt64Ts(5000)
+ qMaxTS := ptrInt64Ts(6000)
+ qr := QueryRequest{
+ SeriesIDs: []common.SeriesID{1},
+ MinTimestamp: qMinTS,
+ MaxTimestamp: qMaxTS,
+ }
+ resultsCh, errCh := sidx.StreamingQuery(context.Background(), qr)
+ var count int
+ for res := range resultsCh {
+ require.NoError(t, res.Error)
+ count += res.Len()
+ }
+ select {
+ case err, ok := <-errCh:
+ if ok {
+ require.NoError(t, err)
+ }
+ default:
+ }
+ assert.Equal(t, 0, count, "parts outside timestamp range should be excluded")
+ })
+}
+
+// TestScanQuery_TimestampSelection verifies ScanQuery timestamp-aware part selection.
+func TestScanQuery_TimestampSelection(t *testing.T) {
+ reqs := []WriteRequest{
+ createTestWriteRequest(1, 100, "data"),
+ }
+
+ t.Run("back_compat_no_timestamp_filter", func(t *testing.T) {
+ sidx := createTestSIDX(t)
+ defer func() {
+ assert.NoError(t, sidx.Close())
+ }()
+ writeTestDataWithTimeRange(t, sidx, reqs, 1, 1, nil, nil)
+ waitForIntroducerLoop()
+
+ sqr := ScanQueryRequest{}
+ res, err := sidx.ScanQuery(context.Background(), sqr)
+ require.NoError(t, err)
+ assert.GreaterOrEqual(t, len(res), 0)
Review Comment:
This scan test assertion `assert.GreaterOrEqual(t, len(res), 0)` is always true and doesn’t verify correctness. Please assert expected non-empty results (or expected row counts) for the back-compat scan case so the new part-selection logic is actually exercised.
```suggestion
total := 0
for _, r := range res {
total += r.Len()
}
assert.Equal(t, 1, total, "expected one row from back-compat scan without timestamp filter")
```
##########
banyand/internal/sidx/timestamp_test.go:
##########
@@ -0,0 +1,394 @@
+// Licensed to Apache Software Foundation (ASF) under one or more contributor
+// license agreements. See the NOTICE file distributed with
+// this work for additional information regarding copyright
+// ownership. Apache Software Foundation (ASF) licenses this file to you under
+// the Apache License, Version 2.0 (the "License"); you may
+// not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+package sidx
+
+import (
+ "context"
+ "encoding/json"
+ "path/filepath"
+ "testing"
+
+ "github.com/stretchr/testify/assert"
+ "github.com/stretchr/testify/require"
+
+ "github.com/apache/skywalking-banyandb/api/common"
+ "github.com/apache/skywalking-banyandb/banyand/observability"
+ "github.com/apache/skywalking-banyandb/banyand/protector"
+ "github.com/apache/skywalking-banyandb/pkg/fs"
+)
+
+// writeTestDataWithTimeRange writes test data with optional min/max timestamps (back-compat: nil means no timestamps).
+func writeTestDataWithTimeRange(t *testing.T, sidx SIDX, reqs []WriteRequest, segmentID int64, partID uint64, minTS, maxTS *int64) {
+ t.Helper()
+ memPart, err := sidx.ConvertToMemPart(reqs, segmentID, minTS, maxTS)
+ require.NoError(t, err)
+ require.NotNil(t, memPart)
+ sidx.IntroduceMemPart(partID, memPart)
+}
+
+func ptrInt64Ts(v int64) *int64 {
+ val := v
+ return &val
+}
+
+// TestConvertToMemPart_Timestamps verifies ConvertToMemPart with and without timestamps.
+func TestConvertToMemPart_Timestamps(t *testing.T) {
+ sidx := createTestSIDX(t)
+ defer func() {
+ assert.NoError(t, sidx.Close())
+ }()
+
+ reqs := []WriteRequest{
+ createTestWriteRequest(1, 1000, "data1"),
+ createTestWriteRequest(1, 2000, "data2"),
+ }
+
+ t.Run("nil_timestamps_back_compat", func(t *testing.T) {
+ mp, err := sidx.ConvertToMemPart(reqs, 1, nil, nil)
+ require.NoError(t, err)
+ require.NotNil(t, mp)
+ require.Nil(t, mp.partMetadata.MinTimestamp)
+ require.Nil(t, mp.partMetadata.MaxTimestamp)
+ ReleaseMemPart(mp)
+ })
+
+ t.Run("with_timestamps", func(t *testing.T) {
+ minTS := int64(500)
+ maxTS := int64(2500)
+ mp, err := sidx.ConvertToMemPart(reqs, 1, &minTS, &maxTS)
+ require.NoError(t, err)
+ require.NotNil(t, mp)
+ require.NotNil(t, mp.partMetadata.MinTimestamp)
+ require.NotNil(t, mp.partMetadata.MaxTimestamp)
+ assert.Equal(t, int64(500), *mp.partMetadata.MinTimestamp)
+ assert.Equal(t, int64(2500), *mp.partMetadata.MaxTimestamp)
+ ReleaseMemPart(mp)
+ })
+}
+
+// TestManifest_BackCompat verifies manifest.json omits timestamps when nil (back-compat).
+func TestManifest_BackCompat(t *testing.T) {
+ dir := t.TempDir()
+ fileSystem := fs.NewLocalFileSystem()
+ opts := NewDefaultOptions()
+ opts.Memory = protector.NewMemory(observability.NewBypassRegistry())
+ opts.Path = dir
+
+ sidxIface, err := NewSIDX(fileSystem, opts)
+ require.NoError(t, err)
+ raw := sidxIface.(*sidx)
+ defer func() {
+ assert.NoError(t, raw.Close())
+ }()
+
+ reqs := []WriteRequest{
+ createTestWriteRequest(1, 100, "data"),
+ }
+
+ t.Run("manifest_without_timestamps", func(t *testing.T) {
+ writeTestDataWithTimeRange(t, raw, reqs, 1, 1, nil, nil)
+ flushIntro, err := raw.Flush(map[uint64]struct{}{1: {}})
+ require.NoError(t, err)
+ raw.IntroduceFlushed(flushIntro)
+ flushIntro.Release()
+
+ manifestPath := filepath.Join(dir, "0000000000000001", manifestFilename)
+ data, readErr := fileSystem.Read(manifestPath)
+ require.NoError(t, readErr)
+
+ var m struct {
+ MinTimestamp *int64 `json:"minTimestamp,omitempty"`
+ MaxTimestamp *int64 `json:"maxTimestamp,omitempty"`
+ MinKey int64 `json:"minKey"`
+ MaxKey int64 `json:"maxKey"`
+ }
+ require.NoError(t, json.Unmarshal(data, &m))
+ require.Nil(t, m.MinTimestamp)
+ require.Nil(t, m.MaxTimestamp)
+ assert.Equal(t, int64(100), m.MinKey)
+ assert.Equal(t, int64(100), m.MaxKey)
+ })
+
+ t.Run("manifest_with_timestamps", func(t *testing.T) {
+ minTS := int64(1000)
+ maxTS := int64(2000)
+ writeTestDataWithTimeRange(t, raw, reqs, 2, 2, &minTS, &maxTS)
+ flushIntro, err := raw.Flush(map[uint64]struct{}{2: {}})
+ require.NoError(t, err)
+ raw.IntroduceFlushed(flushIntro)
+ flushIntro.Release()
+
+ manifestPath := filepath.Join(dir, "0000000000000002", manifestFilename)
+ data, readErr := fileSystem.Read(manifestPath)
+ require.NoError(t, readErr)
+
+ var m struct {
+ MinTimestamp *int64 `json:"minTimestamp,omitempty"`
+ MaxTimestamp *int64 `json:"maxTimestamp,omitempty"`
+ }
+ require.NoError(t, json.Unmarshal(data, &m))
+ require.NotNil(t, m.MinTimestamp)
+ require.NotNil(t, m.MaxTimestamp)
+ assert.Equal(t, int64(1000), *m.MinTimestamp)
+ assert.Equal(t, int64(2000), *m.MaxTimestamp)
+ })
+}
+
+// TestQuery_TimestampSelection verifies timestamp-aware part selection and key-range fallback.
+func TestQuery_TimestampSelection(t *testing.T) {
+ reqs := []WriteRequest{
+ createTestWriteRequest(1, 1000, "data1000"),
+ createTestWriteRequest(1, 2000, "data2000"),
+ }
+
+ t.Run("back_compat_key_range_only", func(t *testing.T) {
+ sidx := createTestSIDX(t)
+ defer func() {
+ assert.NoError(t, sidx.Close())
+ }()
+ writeTestDataWithTimeRange(t, sidx, reqs, 1, 1, nil, nil)
+ waitForIntroducerLoop()
+
+ minKey := int64(500)
+ maxKey := int64(2500)
+ qr := QueryRequest{
+ SeriesIDs: []common.SeriesID{1},
+ MinKey: &minKey,
+ MaxKey: &maxKey,
+ }
+ resultsCh, errCh := sidx.StreamingQuery(context.Background(), qr)
+ var keys []int64
+ for res := range resultsCh {
+ require.NoError(t, res.Error)
+ keys = append(keys, res.Keys...)
+ }
+ select {
+ case err, ok := <-errCh:
+ if ok {
+ require.NoError(t, err)
+ }
+ default:
+ }
+ assert.GreaterOrEqual(t, len(keys), 0, "key-range filter should work without timestamps")
+ })
+
+ t.Run("timestamp_filter_overlaps", func(t *testing.T) {
+ sidx := createTestSIDX(t)
+ defer func() {
+ assert.NoError(t, sidx.Close())
+ }()
+ minTS := ptrInt64Ts(1000)
+ maxTS := ptrInt64Ts(3000)
+ writeTestDataWithTimeRange(t, sidx, reqs, 1, 1, minTS, maxTS)
+ waitForIntroducerLoop()
+
+ qMinTS := ptrInt64Ts(1500)
+ qMaxTS := ptrInt64Ts(2500)
+ qr := QueryRequest{
+ SeriesIDs: []common.SeriesID{1},
+ MinTimestamp: qMinTS,
+ MaxTimestamp: qMaxTS,
+ }
+ resultsCh, errCh := sidx.StreamingQuery(context.Background(), qr)
+ var count int
+ for res := range resultsCh {
+ require.NoError(t, res.Error)
+ count += res.Len()
+ }
+ select {
+ case err, ok := <-errCh:
+ if ok {
+ require.NoError(t, err)
+ }
+ default:
+ }
+ assert.GreaterOrEqual(t, count, 0, "query with overlapping timestamp range")
Review Comment:
These assertions don’t validate behavior: `assert.GreaterOrEqual(t, count, 0)` (and similar) will always pass. To ensure the timestamp-based selection works, assert on concrete expected results (e.g., `count > 0` for the overlap case, and specific key/data values where possible).
```suggestion
assert.Greater(t, count, 0, "query with overlapping timestamp range")
```
--
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-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(sidx): add optional timestamp support for part selection and validation [skywalking-banyandb]
Posted by "hanahmily (via GitHub)" <gi...@apache.org>.
hanahmily merged PR #1019:
URL: https://github.com/apache/skywalking-banyandb/pull/1019
--
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-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org