Copilot commented on code in PR #904:
URL:
https://github.com/apache/skywalking-banyandb/pull/904#discussion_r2631647535
##########
pkg/query/logical/criteria_tags.go:
##########
@@ -40,3 +44,31 @@ func CollectCriteriaTagNames(criteria *modelv1.Criteria,
tags map[string]struct{
CollectCriteriaTagNames(exp.Le.Right, tags)
}
}
+
+// ValidateCriteriaTags validates that all tags referenced in the criteria
exist in the schema.
+func ValidateCriteriaTags(criteria *modelv1.Criteria, registry
TagSpecRegistry) error {
+ if criteria == nil {
+ return nil
+ }
+ switch exp := criteria.GetExp().(type) {
+ case *modelv1.Criteria_Condition:
+ cond := criteria.GetCondition()
+ if cond == nil {
+ return nil
+ }
+ if registry.FindTagSpecByName(cond.Name) == nil {
+ return errors.WithMessagef(ErrTagNotDefined, "condition
tag %q does not exist in schema", cond.Name)
+ }
+ case *modelv1.Criteria_Le:
+ if exp.Le == nil {
+ return nil
+ }
+ if err := ValidateCriteriaTags(exp.Le.Left, registry); err !=
nil {
+ return err
+ }
+ if err := ValidateCriteriaTags(exp.Le.Right, registry); err !=
nil {
+ return err
+ }
+ }
+ return nil
+}
Review Comment:
The new ValidateCriteriaTags function is a useful addition for centralizing
validation logic. However, there appears to be no test coverage for this new
function. Consider adding unit tests to verify:
1. Validation passes for valid criteria with existing tags
2. Validation fails with appropriate error messages for non-existent tags in
conditions
3. Validation correctly handles logical expressions (Le) with nested criteria
4. Validation handles nil/empty criteria gracefully
--
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]
