Copilot commented on code in PR #1213:
URL:
https://github.com/apache/skywalking-banyandb/pull/1213#discussion_r3548774336
##########
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:
In resolveArrayElements, the code appends GrammarValue pointers to the range
variable `s` (`String: &s`). In Go, the range variable is reused each
iteration, so all appended pointers end up pointing at the same string (the
last element), producing incorrect bindings for str_array params.
##########
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:
In resolveArrayElements, the code appends GrammarValue pointers to the range
variable `i` (`Integer: &i`). As with strings, the range variable is reused
each iteration, so every element will point to the same int64 (the last
element), producing incorrect bindings for int_array params.
##########
pkg/bydbql/transformer.go:
##########
@@ -99,18 +99,98 @@ func NewTransformer(registry metadata.Repo) *Transformer {
}
}
-// Transform transforms a Grammar into a native query request.
+// transformRun holds the per-request state of a single transformation: the
+// stateless Transformer plus the bound parameter overlay (nil for a literal or
+// in-place-bound grammar). The conversion methods hang off transformRun so the
+// shared Transformer stays safe for concurrent requests.
+type transformRun struct {
+ *Transformer
+ bound []resolvedParam
+}
+
+// resolveValue returns the effective literal value node for a scalar position:
+// the overlay's bound node for a placeholder, or the node itself for a literal
+// (also the in-place-bound path, where bound is nil and the node was mutated).
+func (r *transformRun) resolveValue(v *GrammarValue) *GrammarValue {
+ if v != nil && v.Param && r.bound != nil {
+ return r.bound[v.ParamIndex].values[0]
+ }
+ return v
+}
+
+// resolveValues expands a list of value nodes, splicing each placeholder's
bound
+// values (one, or several for an array) in place of the placeholder node. A
list
+// with no placeholder (a literal list, or a re-resolve of an already-expanded
+// one) is returned unchanged, so the bound path allocates only when it must.
+func (r *transformRun) resolveValues(values []*GrammarValue) []*GrammarValue {
+ if r.bound == nil {
+ return values
+ }
+ hasParam := false
+ for _, v := range values {
+ if v != nil && v.Param {
+ hasParam = true
+ break
+ }
+ }
+ if !hasParam {
+ return values
+ }
+ out := make([]*GrammarValue, 0, len(values))
+ for _, v := range values {
+ if v != nil && v.Param {
+ out = append(out, r.bound[v.ParamIndex].values...)
+ continue
+ }
+ out = append(out, v)
+ }
+ return out
+}
+
+// resolveTimeString returns a TIME value's effective string: the overlay's
bound
+// string for a placeholder, or the node's own literal.
+func (r *transformRun) resolveTimeString(v *GrammarTimeValue) string {
+ if v != nil && v.Param && r.bound != nil {
+ return r.bound[v.ParamIndex].timeStr
+ }
+ return v.ToString()
+}
+
+// resolveCount returns a count position's effective value: the overlay's bound
+// int for a placeholder, or the node's own literal.
+func (r *transformRun) resolveCount(value int, param bool, paramIndex int) int
{
+ if param && r.bound != nil {
+ return r.bound[paramIndex].count
+ }
+ return value
+}
+
+// Transform transforms a Grammar into a native query request. The grammar must
+// be free of unbound placeholders: either a literal query or one bound in
place
+// by BindParams. Use TransformBound for a reusable prepared statement.
func (t *Transformer) Transform(ctx context.Context, grammar *Grammar)
(*TransformResult, error) {
+ return (&transformRun{Transformer: t}).transform(ctx, grammar)
+}
+
+// TransformBound transforms a bound prepared statement, reading parameter
values
+// from the per-request overlay instead of the immutable template.
+func (t *Transformer) TransformBound(ctx context.Context, bq *BoundQuery)
(*TransformResult, error) {
+ return (&transformRun{Transformer: t, bound: bq.values}).transform(ctx,
bq.stmt.template)
+}
Review Comment:
TransformBound dereferences bq, bq.stmt, and bq.stmt.template without
validation. Passing a nil *BoundQuery (or one produced via zero value in the
same package) will panic instead of returning an error, and a mismatched
overlay length could panic later during ParamIndex resolution.
--
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]
