You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@skywalking.apache.org by "ButterBright (via GitHub)" <gi...@apache.org> on 2026/04/13 16:00:42 UTC
[PR] Support multiple types for the same tag in a single part [skywalking-banyandb]
ButterBright opened a new pull request, #1066:
URL: https://github.com/apache/skywalking-banyandb/pull/1066
<!--
β οΈ Please make sure to read this template first, pull requests that don't accord with this template
maybe closed without notice.
Texts surrounded by `<` and `>` are meant to be replaced by you, e.g. <framework name>, <issue number>.
Put an `x` in the `[ ]` to mark the item as CHECKED. `[x]`
-->
<!-- ==== π Remove this line WHEN AND ONLY WHEN you're fixing a bug, follow the checklist π ====
### Fix <bug description or the bug issue number or bug issue link>
- [ ] Add a unit test to verify that the fix works.
- [ ] Explain briefly why the bug exists and how to fix it.
==== π Remove this line WHEN AND ONLY WHEN you're fixing a bug, follow the checklist π ==== -->
<!-- ==== π Remove this line WHEN AND ONLY WHEN you're adding a new feature, follow the checklist π ====
### <Feature description>
- [ ] If this is non-trivial feature, paste the links/URLs to the design doc.
- [ ] Update the documentation to include this new feature.
- [ ] Tests(including UT, IT, E2E) are added to verify the new feature.
- [ ] If it's UI related, attach the screenshots below.
==== π Remove this line WHEN AND ONLY WHEN you're adding a new feature, follow the checklist π ==== -->
- [x] If this pull request closes/resolves/fixes an existing issue, replace the issue number. Fixes apache/skywalking#13662.
- [ ] Update the [`CHANGES` log](https://github.com/apache/skywalking-banyandb/blob/main/CHANGES.md).
--
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] Support multiple types for the same tag in a single trace or sidx part [skywalking-banyandb]
Posted by "hanahmily (via GitHub)" <gi...@apache.org>.
hanahmily merged PR #1066:
URL: https://github.com/apache/skywalking-banyandb/pull/1066
--
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] Support multiple types for the same tag in a single trace or sidx part [skywalking-banyandb]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #1066:
URL: https://github.com/apache/skywalking-banyandb/pull/1066#discussion_r3107945480
##########
banyand/trace/tag.go:
##########
@@ -26,6 +28,55 @@ import (
pbv1 "github.com/apache/skywalking-banyandb/pkg/pb/v1"
)
+var (
+ valueTypeToSuffix = map[pbv1.ValueType]string{
+ pbv1.ValueTypeStr: "str",
+ pbv1.ValueTypeInt64: "int",
+ pbv1.ValueTypeFloat64: "float",
+ pbv1.ValueTypeBinaryData: "bin",
+ pbv1.ValueTypeStrArr: "str_arr",
+ pbv1.ValueTypeInt64Arr: "int_arr",
+ pbv1.ValueTypeTimestamp: "ts",
+ pbv1.ValueTypeUnknown: "",
+ }
+ suffixToValueType = map[string]pbv1.ValueType{
+ "str": pbv1.ValueTypeStr,
+ "int": pbv1.ValueTypeInt64,
+ "float": pbv1.ValueTypeFloat64,
+ "bin": pbv1.ValueTypeBinaryData,
+ "str_arr": pbv1.ValueTypeStrArr,
+ "int_arr": pbv1.ValueTypeInt64Arr,
+ "ts": pbv1.ValueTypeTimestamp,
+ }
+)
+
+func encodeTypedTag(name string, vt pbv1.ValueType) string {
+ suffix, ok := valueTypeToSuffix[vt]
+ if !ok || suffix == "" {
+ return name
+ }
+ return name + "." + suffix
+}
+
+func decodeTypedTag(key string) string {
+ for suffix := range suffixToValueType {
+ dotSuffix := "." + suffix
+ if strings.HasSuffix(key, dotSuffix) {
+ return key[:len(key)-len(dotSuffix)]
+ }
+ }
+ return key
+}
Review Comment:
`decodeTypedTag` strips any recognized suffix (e.g. `.int`, `.str`) from the tag key unconditionally. Since trace tag names appear to be only validated as non-empty (see `api/validate/validate.go:197-205`), a user-defined tag that legitimately ends with one of these suffixes (like `foo.int`) would be misinterpreted as a typed tag and queried/compared under the wrong name (`foo`).
To avoid accidental collisions/backward-incompatibility, consider either reserving these suffixes at schema-validation time (reject tag names ending with `.<suffix>`), or changing the encoding to include an unambiguous marker so only internally-encoded tags are decoded.
##########
banyand/internal/sidx/query_result.go:
##########
@@ -114,17 +114,23 @@ func (qr *queryResult) loadBlockData(tmpBlock *block, p *part, bm *blockMetadata
// Load tag data for selected tags only
for tagName := range tagsToLoad {
tagBlockInfo, exists := bm.tagsBlocks[tagName]
- if !exists {
- continue // Skip missing tags
- }
-
- if !qr.loadTagData(tmpBlock, p, tagName, &tagBlockInfo, int(bm.count), decoder) {
- // Continue loading other tags even if one fails
+ if exists {
+ qr.loadTagData(tmpBlock, p, tagName, &tagBlockInfo, int(bm.count), decoder)
continue
}
+ for typedTag, typedBlock := range bm.tagsBlocks {
+ if decodeTypedTag(typedTag) == tagName {
+ if qr.loadTagData(tmpBlock, p, typedTag, &typedBlock, int(bm.count), decoder) {
+ td := tmpBlock.tags[typedTag]
+ td.name = tagName
+ tmpBlock.tags[tagName] = td
+ delete(tmpBlock.tags, typedTag)
+ }
+ break
+ }
+ }
Review Comment:
When the requested tag name isn't found, this falls back to the first `typedTag` whose `decodeTypedTag(typedTag)` matches. If a merged block contains *multiple* typed variants for the same logical tag (e.g. both `status.int` and `status.str` due to merging blocks from different parts), map iteration order makes this selection nondeterministic and can silently drop one type's values.
Consider detecting how many typed variants match `tagName`: only alias to the bare name when exactly one match exists; if multiple matches exist, either (a) require clients to explicitly project `*.int`/`*.str` tags, or (b) load/return all typed tags without aliasing to avoid ambiguity.
```suggestion
var matchedTypedTag string
var matchedTypedBlock dataBlock
matchCount := 0
for typedTag, typedBlock := range bm.tagsBlocks {
if decodeTypedTag(typedTag) == tagName {
matchCount++
if matchCount == 1 {
matchedTypedTag = typedTag
matchedTypedBlock = typedBlock
}
}
}
if matchCount == 1 {
if qr.loadTagData(tmpBlock, p, matchedTypedTag, &matchedTypedBlock, int(bm.count), decoder) {
td := tmpBlock.tags[matchedTypedTag]
td.name = tagName
tmpBlock.tags[tagName] = td
delete(tmpBlock.tags, matchedTypedTag)
}
}
```
##########
banyand/internal/sidx/tag.go:
##########
@@ -28,6 +29,64 @@ import (
"github.com/apache/skywalking-banyandb/pkg/pool"
)
+var (
+ valueTypeToSuffix = map[pbv1.ValueType]string{
+ pbv1.ValueTypeStr: "str",
+ pbv1.ValueTypeInt64: "int",
+ pbv1.ValueTypeFloat64: "float",
+ pbv1.ValueTypeBinaryData: "bin",
+ pbv1.ValueTypeStrArr: "str_arr",
+ pbv1.ValueTypeInt64Arr: "int_arr",
+ pbv1.ValueTypeTimestamp: "ts",
+ pbv1.ValueTypeUnknown: "",
+ }
+ suffixToValueType = map[string]pbv1.ValueType{
+ "str": pbv1.ValueTypeStr,
+ "int": pbv1.ValueTypeInt64,
+ "float": pbv1.ValueTypeFloat64,
+ "bin": pbv1.ValueTypeBinaryData,
+ "str_arr": pbv1.ValueTypeStrArr,
+ "int_arr": pbv1.ValueTypeInt64Arr,
+ "ts": pbv1.ValueTypeTimestamp,
+ }
+)
+
+func encodeTypedTag(name string, vt pbv1.ValueType) string {
+ suffix, ok := valueTypeToSuffix[vt]
+ if !ok || suffix == "" {
+ return name
+ }
+ return name + "." + suffix
+}
+
+func decodeTypedTag(key string) string {
+ for suffix := range suffixToValueType {
+ dotSuffix := "." + suffix
+ if strings.HasSuffix(key, dotSuffix) {
+ return key[:len(key)-len(dotSuffix)]
+ }
+ }
+ return key
+}
+
+func hasTypeSuffix(key string) bool {
+ for suffix := range suffixToValueType {
+ if strings.HasSuffix(key, "."+suffix) {
+ return true
+ }
+ }
+ return false
+}
Review Comment:
Same concern as in trace/tag.go: `decodeTypedTag`/`hasTypeSuffix` treat any tag key ending in `.<suffix>` as type-encoded. Because tag names are only checked for non-empty in validation, a user tag like `foo.int` could be misdecoded as `foo`, breaking projections/filters.
Consider reserving these suffixes for internal use (schema validation rejects them) or switching to an encoding that cannot collide with user-provided names.
--
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] Support multiple types for the same tag in a single trace or sidx part [skywalking-banyandb]
Posted by "hanahmily (via GitHub)" <gi...@apache.org>.
hanahmily commented on code in PR #1066:
URL: https://github.com/apache/skywalking-banyandb/pull/1066#discussion_r3107921252
##########
banyand/internal/sidx/query_result.go:
##########
@@ -114,17 +114,23 @@ func (qr *queryResult) loadBlockData(tmpBlock *block, p *part, bm *blockMetadata
// Load tag data for selected tags only
for tagName := range tagsToLoad {
tagBlockInfo, exists := bm.tagsBlocks[tagName]
- if !exists {
- continue // Skip missing tags
- }
-
- if !qr.loadTagData(tmpBlock, p, tagName, &tagBlockInfo, int(bm.count), decoder) {
- // Continue loading other tags even if one fails
+ if exists {
+ qr.loadTagData(tmpBlock, p, tagName, &tagBlockInfo, int(bm.count), decoder)
continue
}
+ for typedTag, typedBlock := range bm.tagsBlocks {
+ if decodeTypedTag(typedTag) == tagName {
Review Comment:
Map iteration order in Go is random. If both status.int and status.str exist, whichever is found first gets returned. Unlike the trace package (which checks schemaTagTypes), the sidx package has no schema-aware filtering β it returns whichever type variant the map iteration hits first. This could return wrong-typed data.
##########
banyand/internal/sidx/tag.go:
##########
@@ -28,6 +29,64 @@ import (
"github.com/apache/skywalking-banyandb/pkg/pool"
)
+var (
+ valueTypeToSuffix = map[pbv1.ValueType]string{
+ pbv1.ValueTypeStr: "str",
+ pbv1.ValueTypeInt64: "int",
+ pbv1.ValueTypeFloat64: "float",
+ pbv1.ValueTypeBinaryData: "bin",
+ pbv1.ValueTypeStrArr: "str_arr",
+ pbv1.ValueTypeInt64Arr: "int_arr",
+ pbv1.ValueTypeTimestamp: "ts",
+ pbv1.ValueTypeUnknown: "",
+ }
+ suffixToValueType = map[string]pbv1.ValueType{
+ "str": pbv1.ValueTypeStr,
+ "int": pbv1.ValueTypeInt64,
+ "float": pbv1.ValueTypeFloat64,
+ "bin": pbv1.ValueTypeBinaryData,
+ "str_arr": pbv1.ValueTypeStrArr,
+ "int_arr": pbv1.ValueTypeInt64Arr,
+ "ts": pbv1.ValueTypeTimestamp,
+ }
+)
+
+func encodeTypedTag(name string, vt pbv1.ValueType) string {
+ suffix, ok := valueTypeToSuffix[vt]
+ if !ok || suffix == "" {
+ return name
+ }
+ return name + "." + suffix
+}
+
+func decodeTypedTag(key string) string {
Review Comment:
This is unsafe for tag names that naturally end with a type suffix:
Tag name: "my.str" β hasTypeSuffix returns TRUE (false positive)
Tag name: "arr.int" β hasTypeSuffix returns TRUE (false positive)
Tag name: "data.bin" β hasTypeSuffix returns TRUE (false positive)
decodeTypedTag("my.str") would return "my" instead of "my.str". If a real tag is named my.str, it would be misinterpreted as a type-encoded version of tag my. This can cause silent data corruption during both merge and query.
##########
banyand/internal/sidx/query_result.go:
##########
@@ -114,17 +114,23 @@ func (qr *queryResult) loadBlockData(tmpBlock *block, p *part, bm *blockMetadata
// Load tag data for selected tags only
for tagName := range tagsToLoad {
tagBlockInfo, exists := bm.tagsBlocks[tagName]
- if !exists {
- continue // Skip missing tags
- }
-
- if !qr.loadTagData(tmpBlock, p, tagName, &tagBlockInfo, int(bm.count), decoder) {
- // Continue loading other tags even if one fails
+ if exists {
+ qr.loadTagData(tmpBlock, p, tagName, &tagBlockInfo, int(bm.count), decoder)
continue
}
+ for typedTag, typedBlock := range bm.tagsBlocks {
+ if decodeTypedTag(typedTag) == tagName {
+ if qr.loadTagData(tmpBlock, p, typedTag, &typedBlock, int(bm.count), decoder) {
+ td := tmpBlock.tags[typedTag]
+ td.name = tagName
+ tmpBlock.tags[tagName] = td
+ delete(tmpBlock.tags, typedTag)
+ }
+ break
+ }
+ }
}
-
- return len(tmpBlock.userKeys) > 0
+ return true
Review Comment:
```suggestion
return len(tmpBlock.userKeys) > 0
```
--
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] Support multiple types for the same tag in a single part [skywalking-banyandb]
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #1066:
URL: https://github.com/apache/skywalking-banyandb/pull/1066#issuecomment-4237990967
## [Codecov](https://app.codecov.io/gh/apache/skywalking-banyandb/pull/1066?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
:white_check_mark: All modified and coverable lines are covered by tests.
:white_check_mark: Project coverage is 31.15%. Comparing base ([`3530dd9`](https://app.codecov.io/gh/apache/skywalking-banyandb/commit/3530dd9b5cefaa4fef60eec91ecbfbdfcaf243c5?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`d35ada3`](https://app.codecov.io/gh/apache/skywalking-banyandb/commit/d35ada338e78d8b087d6b1a487c00ea10cf98eb6?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
:warning: Report is 199 commits behind head on main.
> :exclamation: There is a different number of reports uploaded between BASE (3530dd9) and HEAD (d35ada3). Click for more details.
>
> <details><summary>HEAD has 1 upload less than BASE</summary>
>
>| Flag | BASE (3530dd9) | HEAD (d35ada3) |
>|------|------|------|
>||1|0|
></details>
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## main #1066 +/- ##
===========================================
- Coverage 45.97% 31.15% -14.83%
===========================================
Files 328 124 -204
Lines 55505 12920 -42585
===========================================
- Hits 25520 4025 -21495
+ Misses 27909 8508 -19401
+ Partials 2076 387 -1689
```
| [Flag](https://app.codecov.io/gh/apache/skywalking-banyandb/pull/1066/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Ξ | |
|---|---|---|
| [pkg](https://app.codecov.io/gh/apache/skywalking-banyandb/pull/1066/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `31.15% <ΓΈ> (?)` | |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#carryforward-flags-in-the-pull-request-comment) to find out more.
</details>
[:umbrella: View full report in Codecov by Sentry](https://app.codecov.io/gh/apache/skywalking-banyandb/pull/1066?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
<details><summary> :rocket: New features to boost your workflow: </summary>
- :snowflake: [Test Analytics](https://docs.codecov.com/docs/test-analytics): Detect flaky tests, report on failures, and find test suite problems.
- :package: [JS Bundle Analysis](https://docs.codecov.com/docs/javascript-bundle-analysis): Save yourself from yourself by tracking and limiting bundle sizes in JS merges.
</details>
--
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