mrproliu commented on code in PR #1213:
URL:
https://github.com/apache/skywalking-banyandb/pull/1213#discussion_r3548804026
##########
pkg/bydbql/binder.go:
##########
@@ -336,40 +318,115 @@ func (b *binder) expandLists() {
}
func bindTimeValue(v *GrammarTimeValue, p *modelv1.TagValue) error {
+ strVal, err := resolveTimeParam(p)
+ if err != nil {
+ return err
+ }
+ v.String = &strVal
+ v.Param = false
+ return nil
+}
+
+func bindScalarValue(v *GrammarValue, p *modelv1.TagValue) error {
+ resolved, err := resolveScalarParam(p)
+ if err != nil {
+ return err
+ }
+ v.String, v.Integer, v.Null = resolved.String, resolved.Integer,
resolved.Null
+ v.Param = false
+ return nil
+}
+
+// resolveScalarParam produces a fresh literal value node for a str/int/null
+// parameter. It is the single source of scalar type acceptance, shared by the
+// in-place bind path and the reusable Bind path.
+func resolveScalarParam(p *modelv1.TagValue) (*GrammarValue, error) {
switch val := p.Value.(type) {
case *modelv1.TagValue_Str:
strVal := val.Str.GetValue()
- v.String = &strVal
+ return &GrammarValue{String: &strVal}, nil
+ case *modelv1.TagValue_Int:
+ intVal := val.Int.GetValue()
+ return &GrammarValue{Integer: &intVal}, nil
+ case *modelv1.TagValue_Null:
+ return &GrammarValue{Null: true}, nil
+ default:
+ return nil, fmt.Errorf("this position only accepts str, int, or
null parameters, got %T", p.Value)
+ }
+}
+
+// resolveTimeParam produces the time string for a str/timestamp parameter. int
+// is rejected: the transformer cannot parse a bare integer as a timestamp, so
+// accepting it would only defer a certain failure.
+func resolveTimeParam(p *modelv1.TagValue) (string, error) {
+ switch val := p.Value.(type) {
+ case *modelv1.TagValue_Str:
+ return val.Str.GetValue(), nil
case *modelv1.TagValue_Timestamp:
// A nil inner timestamp would silently decode as the Unix
epoch and an
// out-of-range one would format as a nonsense year; reject
both here.
if err := val.Timestamp.CheckValid(); err != nil {
- return fmt.Errorf("invalid timestamp parameter: %w",
err)
+ return "", fmt.Errorf("invalid timestamp parameter:
%w", err)
}
- strVal := val.Timestamp.AsTime().Format(time.RFC3339Nano)
- v.String = &strVal
+ return val.Timestamp.AsTime().Format(time.RFC3339Nano), nil
default:
- // int is rejected as well: the transformer cannot parse a bare
integer
- // as a timestamp, so accepting it would only defer a certain
failure.
- return fmt.Errorf("time clause only accepts str or timestamp
parameters, got %T", p.Value)
+ return "", fmt.Errorf("time clause only accepts str or
timestamp parameters, got %T", p.Value)
}
- v.Param = false
- return nil
}
-func bindScalarValue(v *GrammarValue, p *modelv1.TagValue) error {
+// resolveArrayElements produces the literal nodes a str_array/int_array
+// parameter expands to, rejecting empty arrays.
+func resolveArrayElements(p *modelv1.TagValue) ([]*GrammarValue, error) {
switch val := p.Value.(type) {
- case *modelv1.TagValue_Str:
- strVal := val.Str.GetValue()
- v.String = &strVal
- case *modelv1.TagValue_Int:
- intVal := val.Int.GetValue()
- v.Integer = &intVal
- case *modelv1.TagValue_Null:
- v.Null = true
+ case *modelv1.TagValue_StrArray:
+ if len(val.StrArray.GetValue()) == 0 {
+ return nil, fmt.Errorf("array parameter must not be
empty")
+ }
+ expanded := make([]*GrammarValue, 0,
len(val.StrArray.GetValue()))
+ for _, s := range val.StrArray.GetValue() {
+ expanded = append(expanded, &GrammarValue{String: &s})
+ }
Review Comment:
These are false positives. The module targets go 1.25.8 (see go.mod), and
since Go 1.22 the range loop variable is scoped per-iteration, so &s / &i take
a distinct address each iteration. This is also covered by the equivalence
tests — str_array param expands in IN and int_array param expands in HAVING
bind ("a","b") / (200,500) and assert a proto-equal result against the literal
query; an aliasing bug would collapse them to the last element and fail those
tests.
##########
pkg/bydbql/binder.go:
##########
@@ -336,40 +318,115 @@ func (b *binder) expandLists() {
}
func bindTimeValue(v *GrammarTimeValue, p *modelv1.TagValue) error {
+ strVal, err := resolveTimeParam(p)
+ if err != nil {
+ return err
+ }
+ v.String = &strVal
+ v.Param = false
+ return nil
+}
+
+func bindScalarValue(v *GrammarValue, p *modelv1.TagValue) error {
+ resolved, err := resolveScalarParam(p)
+ if err != nil {
+ return err
+ }
+ v.String, v.Integer, v.Null = resolved.String, resolved.Integer,
resolved.Null
+ v.Param = false
+ return nil
+}
+
+// resolveScalarParam produces a fresh literal value node for a str/int/null
+// parameter. It is the single source of scalar type acceptance, shared by the
+// in-place bind path and the reusable Bind path.
+func resolveScalarParam(p *modelv1.TagValue) (*GrammarValue, error) {
switch val := p.Value.(type) {
case *modelv1.TagValue_Str:
strVal := val.Str.GetValue()
- v.String = &strVal
+ return &GrammarValue{String: &strVal}, nil
+ case *modelv1.TagValue_Int:
+ intVal := val.Int.GetValue()
+ return &GrammarValue{Integer: &intVal}, nil
+ case *modelv1.TagValue_Null:
+ return &GrammarValue{Null: true}, nil
+ default:
+ return nil, fmt.Errorf("this position only accepts str, int, or
null parameters, got %T", p.Value)
+ }
+}
+
+// resolveTimeParam produces the time string for a str/timestamp parameter. int
+// is rejected: the transformer cannot parse a bare integer as a timestamp, so
+// accepting it would only defer a certain failure.
+func resolveTimeParam(p *modelv1.TagValue) (string, error) {
+ switch val := p.Value.(type) {
+ case *modelv1.TagValue_Str:
+ return val.Str.GetValue(), nil
case *modelv1.TagValue_Timestamp:
// A nil inner timestamp would silently decode as the Unix
epoch and an
// out-of-range one would format as a nonsense year; reject
both here.
if err := val.Timestamp.CheckValid(); err != nil {
- return fmt.Errorf("invalid timestamp parameter: %w",
err)
+ return "", fmt.Errorf("invalid timestamp parameter:
%w", err)
}
- strVal := val.Timestamp.AsTime().Format(time.RFC3339Nano)
- v.String = &strVal
+ return val.Timestamp.AsTime().Format(time.RFC3339Nano), nil
default:
- // int is rejected as well: the transformer cannot parse a bare
integer
- // as a timestamp, so accepting it would only defer a certain
failure.
- return fmt.Errorf("time clause only accepts str or timestamp
parameters, got %T", p.Value)
+ return "", fmt.Errorf("time clause only accepts str or
timestamp parameters, got %T", p.Value)
}
- v.Param = false
- return nil
}
-func bindScalarValue(v *GrammarValue, p *modelv1.TagValue) error {
+// resolveArrayElements produces the literal nodes a str_array/int_array
+// parameter expands to, rejecting empty arrays.
+func resolveArrayElements(p *modelv1.TagValue) ([]*GrammarValue, error) {
switch val := p.Value.(type) {
- case *modelv1.TagValue_Str:
- strVal := val.Str.GetValue()
- v.String = &strVal
- case *modelv1.TagValue_Int:
- intVal := val.Int.GetValue()
- v.Integer = &intVal
- case *modelv1.TagValue_Null:
- v.Null = true
+ case *modelv1.TagValue_StrArray:
+ if len(val.StrArray.GetValue()) == 0 {
+ return nil, fmt.Errorf("array parameter must not be
empty")
+ }
+ expanded := make([]*GrammarValue, 0,
len(val.StrArray.GetValue()))
+ for _, s := range val.StrArray.GetValue() {
+ expanded = append(expanded, &GrammarValue{String: &s})
+ }
+ return expanded, nil
+ case *modelv1.TagValue_IntArray:
+ if len(val.IntArray.GetValue()) == 0 {
+ return nil, fmt.Errorf("array parameter must not be
empty")
+ }
+ expanded := make([]*GrammarValue, 0,
len(val.IntArray.GetValue()))
+ for _, i := range val.IntArray.GetValue() {
+ expanded = append(expanded, &GrammarValue{Integer: &i})
+ }
Review Comment:
These are false positives. The module targets go 1.25.8 (see go.mod), and
since Go 1.22 the range loop variable is scoped per-iteration, so &s / &i take
a distinct address each iteration. This is also covered by the equivalence
tests — str_array param expands in IN and int_array param expands in HAVING
bind ("a","b") / (200,500) and assert a proto-equal result against the literal
query; an aliasing bug would collapse them to the last element and fail those
tests.
--
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]
