Copilot commented on code in PR #900:
URL:
https://github.com/apache/skywalking-banyandb/pull/900#discussion_r2625709621
##########
banyand/cmd/dump/measure_test.go:
##########
@@ -158,3 +160,74 @@ func TestDumpMeasurePartFormat(t *testing.T) {
t.Logf("Successfully parsed part with %d data points across %d primary
blocks (metadata BlocksCount=%d)",
totalDataPoints, len(p.primaryBlockMetadata),
p.partMetadata.BlocksCount)
}
+
+// TestDumpMeasurePartWithSeriesMetadata tests that the dump tool can parse
smeta file.
+// This test creates a part with series metadata and verifies the dump tool
can correctly parse it.
+func TestDumpMeasurePartWithSeriesMetadata(t *testing.T) {
+ tmpPath, defFn := test.Space(require.New(t))
+ defer defFn()
+
+ fileSystem := fs.NewLocalFileSystem()
+
+ // Create a part with series metadata
+ partPath, cleanup := createTestMeasurePartWithSeriesMetadata(tmpPath,
fileSystem)
+ defer cleanup()
+
+ // Extract part ID from path
+ partName := filepath.Base(partPath)
+ partID, err := strconv.ParseUint(partName, 16, 64)
+ require.NoError(t, err, "part directory should have valid hex name")
+
+ // Parse the part using dump tool functions
+ p, err := openMeasurePart(partID, filepath.Dir(partPath), fileSystem)
+ require.NoError(t, err, "should be able to open part with series
metadata")
+ defer closeMeasurePart(p)
+
+ // Verify series metadata reader is available
+ assert.NotNil(t, p.seriesMetadata, "series metadata reader should be
available")
+
+ // Create a dump context to test parsing
+ opts := measureDumpOptions{
+ shardPath: filepath.Dir(partPath),
+ segmentPath: tmpPath, // Not used for this test
+ verbose: false,
+ csvOutput: false,
+ }
+ ctx, err := newMeasureDumpContext(opts)
+ require.NoError(t, err)
+ if ctx != nil {
+ defer ctx.close()
+ }
+
+ // Test parsing series metadata
+ err = ctx.parseAndDisplaySeriesMetadata(partID, p)
+ require.NoError(t, err, "should be able to parse series metadata")
+}
+
+// createTestMeasurePartWithSeriesMetadata creates a test measure part with
series metadata.
+func createTestMeasurePartWithSeriesMetadata(tmpPath string, fileSystem
fs.FileSystem) (string, func()) {
+ // Use measure package to create a part
+ partPath, cleanup := measure.CreateTestPartForDump(tmpPath, fileSystem)
+
+ // Create sample series metadata file
+ seriesMetadataPath := filepath.Join(partPath, "smeta.bin")
+
+ // Create sample documents
+ docs := index.Documents{
+ {
+ DocID: 1,
+ EntityValues: []byte("service.name=test-service"),
+ },
+ {
+ DocID: 2,
+ EntityValues: []byte("service.name=another-service"),
+ },
+ }
+
+ seriesMetadataBytes, err := docs.Marshal()
+ if err == nil {
+ fs.MustFlush(fileSystem, seriesMetadataBytes,
seriesMetadataPath, storage.FilePerm)
+ }
Review Comment:
The error from `docs.Marshal()` is silently ignored. If marshaling fails,
the test will continue with an empty or invalid smeta.bin file, which may cause
the test to pass when it should fail. Consider either returning the error to
the caller or using a test assertion to ensure marshaling succeeds.
##########
banyand/cmd/dump/stream.go:
##########
@@ -1097,3 +1118,54 @@ func streamTagValueDecoder(valueType pbv1.ValueType,
value []byte, valueArr [][]
return pbv1.NullTagValue
}
}
+
+func (ctx *streamDumpContext) parseAndDisplaySeriesMetadata(partID uint64, p
*streamPart) error {
+ // Read all data from series metadata file
+ seqReader := p.seriesMetadata.SequentialRead()
+ defer seqReader.Close()
+
+ var readMetadataBytes []byte
+ buf := make([]byte, 1024)
+ for {
+ n, err := seqReader.Read(buf)
+ if n == 0 {
+ if err != nil && !errors.Is(err, io.EOF) {
+ return fmt.Errorf("failed to read series
metadata: %w", err)
+ }
+ break
+ }
+ if err != nil && !errors.Is(err, io.EOF) {
+ return fmt.Errorf("failed to read series metadata: %w",
err)
+ }
+ readMetadataBytes = append(readMetadataBytes, buf[:n]...)
Review Comment:
The manual buffer reading loop can be replaced with `io.ReadAll`, which is
already used elsewhere in this codebase (e.g., in
`readStreamPrimaryBlockMetadata` at line 596). Using `io.ReadAll` is simpler,
more idiomatic, and less error-prone than manually accumulating bytes with a
fixed-size buffer.
```suggestion
readMetadataBytes, err := io.ReadAll(seqReader)
if err != nil {
return fmt.Errorf("failed to read series metadata: %w", err)
```
##########
banyand/cmd/dump/trace.go:
##########
@@ -1297,3 +1318,54 @@ func writeTraceRowAsCSV(writer *csv.Writer, row
traceRowData, tagColumns []strin
return writer.Write(csvRow)
}
+
+func (ctx *traceDumpContext) parseAndDisplaySeriesMetadata(partID uint64, p
*part) error {
+ // Read all data from series metadata file
+ seqReader := p.seriesMetadata.SequentialRead()
+ defer seqReader.Close()
+
+ var readMetadataBytes []byte
+ buf := make([]byte, 1024)
+ for {
+ n, err := seqReader.Read(buf)
+ if n == 0 {
+ if err != nil && !errors.Is(err, io.EOF) {
+ return fmt.Errorf("failed to read series
metadata: %w", err)
+ }
+ break
+ }
+ if err != nil && !errors.Is(err, io.EOF) {
+ return fmt.Errorf("failed to read series metadata: %w",
err)
+ }
+ readMetadataBytes = append(readMetadataBytes, buf[:n]...)
Review Comment:
The manual buffer reading loop can be replaced with `io.ReadAll`, which is
already used elsewhere in this codebase (e.g., in `readPrimaryBlockMetadata`,
`readStreamPrimaryBlockMetadata`, and `readMeasurePrimaryBlockMetadata`
functions). Using `io.ReadAll` is simpler, more idiomatic, and less error-prone
than manually accumulating bytes with a fixed-size buffer.
```suggestion
readMetadataBytes, err := io.ReadAll(seqReader)
if err != nil {
return fmt.Errorf("failed to read series metadata: %w", err)
```
##########
banyand/cmd/dump/trace_test.go:
##########
@@ -187,3 +189,74 @@ func decodeStringArray(data []byte) []string {
}
return values
}
+
+// TestDumpTracePartWithSeriesMetadata tests that the dump tool can parse
smeta file.
+// This test creates a part with series metadata and verifies the dump tool
can correctly parse it.
+func TestDumpTracePartWithSeriesMetadata(t *testing.T) {
+ tmpPath, defFn := test.Space(require.New(t))
+ defer defFn()
+
+ fileSystem := fs.NewLocalFileSystem()
+
+ // Create a part with series metadata
+ partPath, cleanup := createTestTracePartWithSeriesMetadata(tmpPath,
fileSystem)
+ defer cleanup()
+
+ // Extract part ID from path
+ partName := filepath.Base(partPath)
+ partID, err := strconv.ParseUint(partName, 16, 64)
+ require.NoError(t, err, "part directory should have valid hex name")
+
+ // Parse the part using dump tool functions
+ p, err := openFilePart(partID, filepath.Dir(partPath), fileSystem)
+ require.NoError(t, err, "should be able to open part with series
metadata")
+ defer closePart(p)
+
+ // Verify series metadata reader is available
+ assert.NotNil(t, p.seriesMetadata, "series metadata reader should be
available")
+
+ // Create a dump context to test parsing
+ opts := traceDumpOptions{
+ shardPath: filepath.Dir(partPath),
+ segmentPath: tmpPath, // Not used for this test
+ verbose: false,
+ csvOutput: false,
+ }
+ ctx, err := newTraceDumpContext(opts)
+ require.NoError(t, err)
+ if ctx != nil {
+ defer ctx.close()
+ }
+
+ // Test parsing series metadata
+ err = ctx.parseAndDisplaySeriesMetadata(partID, p)
+ require.NoError(t, err, "should be able to parse series metadata")
+}
+
+// createTestTracePartWithSeriesMetadata creates a test trace part with series
metadata.
+func createTestTracePartWithSeriesMetadata(tmpPath string, fileSystem
fs.FileSystem) (string, func()) {
+ // Use trace package to create a part
+ partPath, cleanup := trace.CreateTestPartForDump(tmpPath, fileSystem)
+
+ // Create sample series metadata file
+ seriesMetadataPath := filepath.Join(partPath, "smeta.bin")
+
+ // Create sample documents
+ docs := index.Documents{
+ {
+ DocID: 1,
+ EntityValues: []byte("service.name=test-service"),
+ },
+ {
+ DocID: 2,
+ EntityValues: []byte("service.name=another-service"),
+ },
+ }
+
+ seriesMetadataBytes, err := docs.Marshal()
+ if err == nil {
+ fs.MustFlush(fileSystem, seriesMetadataBytes,
seriesMetadataPath, storage.FilePerm)
+ }
Review Comment:
The error from `docs.Marshal()` is silently ignored. If marshaling fails,
the test will continue with an empty or invalid smeta.bin file, which may cause
the test to pass when it should fail. Consider either returning the error to
the caller or using a test assertion to ensure marshaling succeeds.
##########
banyand/cmd/dump/stream_test.go:
##########
@@ -148,3 +150,75 @@ func TestDumpStreamPartFormat(t *testing.T) {
func createTestStreamPartForDump(tmpPath string, fileSystem fs.FileSystem)
(string, func()) {
return stream.CreateTestPartForDump(tmpPath, fileSystem)
}
+
+// TestDumpStreamPartWithSeriesMetadata tests that the dump tool can parse
smeta file.
+// This test creates a part with series metadata and verifies the dump tool
can correctly parse it.
+func TestDumpStreamPartWithSeriesMetadata(t *testing.T) {
+ tmpPath, defFn := test.Space(require.New(t))
+ defer defFn()
+
+ fileSystem := fs.NewLocalFileSystem()
+
+ // Create a part with series metadata
+ partPath, cleanup := createTestStreamPartWithSeriesMetadata(tmpPath,
fileSystem)
+ defer cleanup()
+
+ // Extract part ID from path
+ partName := filepath.Base(partPath)
+ partID, err := strconv.ParseUint(partName, 16, 64)
+ require.NoError(t, err, "part directory should have valid hex name")
+
+ // Parse the part using dump tool functions
+ p, err := openStreamPart(partID, filepath.Dir(partPath), fileSystem)
+ require.NoError(t, err, "should be able to open part with series
metadata")
+ defer closeStreamPart(p)
+
+ // Verify series metadata reader is available
+ assert.NotNil(t, p.seriesMetadata, "series metadata reader should be
available")
+
+ // Create a dump context to test parsing
+ opts := streamDumpOptions{
+ shardPath: filepath.Dir(partPath),
+ segmentPath: tmpPath, // Not used for this test
+ verbose: false,
+ csvOutput: false,
+ }
+ ctx, err := newStreamDumpContext(opts)
+ require.NoError(t, err)
+ if ctx != nil {
+ defer ctx.close()
+ }
+
+ // Test parsing series metadata
+ err = ctx.parseAndDisplaySeriesMetadata(partID, p)
+ require.NoError(t, err, "should be able to parse series metadata")
+}
+
+// createTestStreamPartWithSeriesMetadata creates a test stream part with
series metadata.
+func createTestStreamPartWithSeriesMetadata(tmpPath string, fileSystem
fs.FileSystem) (string, func()) {
+ // Use stream package to create a part
+ partPath, cleanup := stream.CreateTestPartForDump(tmpPath, fileSystem)
+
+ // Create sample series metadata file
+ // This is a simplified version - in real scenarios, smeta is created
during flush
+ seriesMetadataPath := filepath.Join(partPath, "smeta.bin")
+
+ // Create sample documents
+ docs := index.Documents{
+ {
+ DocID: 1,
+ EntityValues: []byte("test=entity1"),
+ },
+ {
+ DocID: 2,
+ EntityValues: []byte("test=entity2"),
+ },
+ }
+
+ seriesMetadataBytes, err := docs.Marshal()
+ if err == nil {
+ fs.MustFlush(fileSystem, seriesMetadataBytes,
seriesMetadataPath, storage.FilePerm)
+ }
Review Comment:
The error from `docs.Marshal()` is silently ignored. If marshaling fails,
the test will continue with an empty or invalid smeta.bin file, which may cause
the test to pass when it should fail. Consider either returning the error to
the caller or using a test assertion to ensure marshaling succeeds.
##########
banyand/cmd/dump/measure.go:
##########
@@ -1294,3 +1315,54 @@ func measureTagValueDecoder(valueType pbv1.ValueType,
value []byte, valueArr [][
return pbv1.NullTagValue
}
}
+
+func (ctx *measureDumpContext) parseAndDisplaySeriesMetadata(partID uint64, p
*measurePart) error {
+ // Read all data from series metadata file
+ seqReader := p.seriesMetadata.SequentialRead()
+ defer seqReader.Close()
+
+ var readMetadataBytes []byte
+ buf := make([]byte, 1024)
+ for {
+ n, err := seqReader.Read(buf)
+ if n == 0 {
+ if err != nil && !errors.Is(err, io.EOF) {
+ return fmt.Errorf("failed to read series
metadata: %w", err)
+ }
+ break
+ }
+ if err != nil && !errors.Is(err, io.EOF) {
+ return fmt.Errorf("failed to read series metadata: %w",
err)
+ }
+ readMetadataBytes = append(readMetadataBytes, buf[:n]...)
Review Comment:
The manual buffer reading loop can be replaced with `io.ReadAll`, which is
already used elsewhere in this codebase (e.g., in
`readMeasurePrimaryBlockMetadata` at line 661). Using `io.ReadAll` is simpler,
more idiomatic, and less error-prone than manually accumulating bytes with a
fixed-size buffer.
```suggestion
readMetadataBytes, err := io.ReadAll(seqReader)
if err != nil {
return fmt.Errorf("failed to read series metadata: %w", err)
```
--
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]
