Copilot commented on code in PR #981:
URL:
https://github.com/apache/skywalking-banyandb/pull/981#discussion_r2866852451
##########
pkg/query/logical/tag_filter.go:
##########
@@ -532,12 +532,15 @@ func (r *rangeTag) String() string {
}
func tagExpr(accessor TagValueIndexAccessor, registry TagSpecRegistry, tagName
string, analyzer *analysis.Analyzer) (ComparableExpr, error) {
- if tagSpec := registry.FindTagSpecByName(tagName); tagSpec != nil {
- if tagVal := accessor.GetTagValue(tagSpec.TagFamilyIdx,
tagSpec.TagIdx); tagVal != nil {
+ tagSpec := registry.FindTagSpecByName(tagName)
+ if tagSpec != nil {
+ tagVal := accessor.GetTagValue(tagSpec.TagFamilyIdx,
tagSpec.TagIdx)
+ if tagVal != nil {
return parseExpr(tagVal, analyzer)
}
+ return nil, errors.WithMessagef(ErrTagNotDefined, "tag value is
nil for tag %q, tagSpec: %+v", tagName, tagSpec)
Review Comment:
The error message includes `tagSpec: %+v`, which can leak internal schema
details (indexes/family/idx) and may produce very large/unhelpful client-facing
errors. Consider removing the full struct dump and returning a concise message
(e.g., just the tag name), and/or logging the detailed tagSpec at debug level
instead.
```suggestion
return nil, errors.WithMessagef(ErrTagNotDefined, "tag value is
nil for tag %q", tagName)
```
##########
CHANGES.md:
##########
@@ -36,6 +36,7 @@ Release Notes.
- Fix data written to the wrong shard and related stream queries.
- Fix the lifecycle panic when the trace has no sidx.
- Fix panic in sidx merge and flush operations when part counts don't match
expectations.
+- Fix trace queries with range conditions on the same tag (e.g., duration)
combined with ORDER BY by deduplicating tag names when merging logical
expression branches.
Review Comment:
The CHANGES entry has a redundant phrasing: "combined with ORDER BY by ...".
Please reword to remove the duplicated "by" for clarity.
```suggestion
- Fix trace queries with range conditions on the same tag (e.g., duration)
combined with ORDER BY, deduplicating tag names when merging logical expression
branches.
```
--
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]
