Copilot commented on code in PR #1213:
URL:
https://github.com/apache/skywalking-banyandb/pull/1213#discussion_r3550350301
##########
pkg/bydbql/transformer.go:
##########
@@ -99,18 +99,101 @@ 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) {
+ if bq == nil || bq.stmt == nil {
+ return nil, errors.New("nil bound query; construct it with
PreparedStatement.Bind")
+ }
+ return (&transformRun{Transformer: t, bound: bq.values}).transform(ctx,
bq.stmt.template)
+}
Review Comment:
TransformBound assumes the BoundQuery was constructed by
PreparedStatement.Bind and will panic if bq.values doesn't match the prepared
statement's placeholder count (e.g., internal misuse or future refactors).
Adding a length check makes this API fail fast with a clear error instead of
risking an out-of-bounds panic in resolveValue/resolveCount.
##########
banyand/liaison/grpc/bydbql_cache.go:
##########
@@ -0,0 +1,140 @@
+// Licensed to Apache Software Foundation (ASF) under one or more contributor
+// license agreements. See the NOTICE file distributed with
+// this work for additional information regarding copyright
+// ownership. Apache Software Foundation (ASF) licenses this file to you under
+// the Apache License, Version 2.0 (the "License"); you may
+// not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+package grpc
+
+import (
+ "sync/atomic"
+
+ lru "github.com/hashicorp/golang-lru"
+
+ "github.com/apache/skywalking-banyandb/pkg/bydbql"
+)
+
+// cacheValue is a cached prepared statement plus its accounted byte cost
(query
+// text plus the estimated statement footprint), so evictions can adjust
curBytes.
+type cacheValue struct {
+ ps *bydbql.PreparedStatement
+ cost int
+}
+
+// preparedCache is a concurrency-safe LRU over parsed BydbQL prepared
statements,
+// keyed by the exact query text. A cached statement is immutable and safe to
Bind
+// concurrently, and the parse result depends only on the query text, so
entries
+// never need invalidation. The count bound is enforced by the underlying LRU;
the
+// byte bound (accounted key text plus estimated statement size) is enforced
on top
+// via curBytes and RemoveOldest. All shared state is either behind the LRU's
own
+// lock or atomic, so the cache needs no mutex of its own.
+type preparedCache struct {
+ metrics *metrics
+ lru *lru.Cache
+ maxBytes int
+ curBytes atomic.Int64
+ hits atomic.Uint64
+ misses atomic.Uint64
+}
+
+// newPreparedCache builds a cache holding up to size entries and maxBytes of
+// accounted size. A size <= 0 disables caching (lru is nil): getOrPrepare then
+// parses every request, each counted as a miss. A maxBytes <= 0 drops the byte
+// bound, leaving only the count.
Review Comment:
The comment on newPreparedCache says disabled caching counts each request as
a miss, but getOrPrepare explicitly bypasses metrics when c.lru == nil (and the
test asserts hits/misses remain 0). Update the comment to match the actual
behavior.
##########
pkg/bydbql/prepared.go:
##########
@@ -0,0 +1,289 @@
+// Licensed to Apache Software Foundation (ASF) under one or more contributor
+// license agreements. See the NOTICE file distributed with
+// this work for additional information regarding copyright
+// ownership. Apache Software Foundation (ASF) licenses this file to you under
+// the Apache License, Version 2.0 (the "License"); you may
+// not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+package bydbql
+
+import (
+ "fmt"
+ "math"
+ "reflect"
+
+ modelv1
"github.com/apache/skywalking-banyandb/api/proto/banyandb/model/v1"
+)
+
+// placeholderKind classifies a `?` placeholder by the position it occupies,
+// which determines the parameter types Bind accepts and how Transform reads
it.
+type placeholderKind uint8
+
+const (
+ // phScalar is a WHERE/HAVING comparison value: str, int, or null.
+ phScalar placeholderKind = iota
+ // phList is an element of an IN/MATCH/HAVING value list: scalar or an
array
+ // that expands in place.
+ phList
+ // phTime is a TIME value: str or timestamp.
+ phTime
+ // phCount is a LIMIT/OFFSET/TOP N count: an int within maxCount.
+ phCount
+)
+
+// placeholderSpec describes one placeholder, recorded once by Prepare and read
+// per request by Bind (to validate the parameter) and Transform (to read it).
+type placeholderSpec struct {
+ maxCount int64 // upper bound for phCount positions
+ kind placeholderKind
+}
+
+// PreparedStatement is an immutable, reusable parsed query. It is safe to
cache
+// and to Bind concurrently: Prepare numbers the placeholders once, and Bind
+// never mutates the template — the bound values live in a per-request overlay.
+type PreparedStatement struct {
+ template *Grammar
+ specs []placeholderSpec
+}
+
+// Prepare parses a query and numbers its `?` placeholders so it can be bound
+// repeatedly with different parameters without re-parsing.
+func Prepare(query string) (*PreparedStatement, error) {
+ g, err := ParseQuery(query)
+ if err != nil {
+ return nil, err
+ }
+ p := &preparer{}
+ p.walkGrammar(g)
+ return &PreparedStatement{template: g, specs: p.specs}, nil
+}
+
+// NumPlaceholders reports how many `?` placeholders the statement carries. A
zero
+// count means a literal query with no reusable parameters.
+func (ps *PreparedStatement) NumPlaceholders() int {
+ return len(ps.specs)
+}
+
+// EstimatedSize returns an approximate in-memory byte footprint of the
prepared
+// statement — its parsed grammar template and placeholder specs. It is a
+// heuristic for cache byte-accounting (a reflection walk of the object graph),
+// not an exact measurement, and is meant to be called off the hot path.
+func (ps *PreparedStatement) EstimatedSize() int {
+ return deepSize(reflect.ValueOf(ps))
+}
+
+// deepSize sums the bytes reachable from v: the inline size of each pointed-to
+// value plus the out-of-line bytes of strings and slice backing arrays. Inline
+// struct/array fields are already covered by their container's size, so only
+// their referenced memory is added. It is shaped for the grammar, an acyclic
tree
+// of concrete structs, pointers, slices, strings, and scalars.
+func deepSize(v reflect.Value) int {
+ switch v.Kind() {
+ case reflect.Pointer:
+ if v.IsNil() {
+ return 0
+ }
+ elem := v.Elem()
+ return int(elem.Type().Size()) + deepSize(elem)
+ case reflect.Struct:
+ total := 0
+ for i := 0; i < v.NumField(); i++ {
+ total += deepSize(v.Field(i))
+ }
+ return total
+ case reflect.Slice:
+ if v.IsNil() {
+ return 0
+ }
+ total := v.Cap() * int(v.Type().Elem().Size())
+ for i := 0; i < v.Len(); i++ {
+ total += deepSize(v.Index(i))
+ }
+ return total
+ case reflect.String:
+ return v.Len()
+ default:
+ return 0
+ }
+}
+
+// resolvedParam holds one placeholder's bound value in the per-request
overlay.
+// Which field is populated follows the placeholder's kind: a scalar/list
+// position fills values (one node, or several for an expanded array), a TIME
+// position fills timeStr, and a count position fills count.
+type resolvedParam struct {
+ timeStr string
+ values []*GrammarValue
+ count int
+}
+
+// BoundQuery pairs an immutable prepared statement with the values bound for
one
+// request. Its values are the per-request overlay indexed by placeholder
+// position, allocated by Bind and never shared or mutated after. Pass it to
+// Transformer.TransformBound.
+type BoundQuery struct {
+ stmt *PreparedStatement
+ values []resolvedParam
+}
+
+// Bind resolves params against the statement's placeholders and returns a
+// per-request BoundQuery, leaving the template untouched, so a statement may
be
+// bound repeatedly and concurrently. Parameter positions in errors are
1-based,
+// matching BindParams.
+func (ps *PreparedStatement) Bind(params []*modelv1.TagValue) (*BoundQuery,
error) {
+ if len(params) != len(ps.specs) {
+ return nil, fmt.Errorf("parameter count mismatch: query
contains %d placeholder(s) but %d parameter(s) provided", len(ps.specs),
len(params))
+ }
+ values := make([]resolvedParam, len(ps.specs))
+ for i, spec := range ps.specs {
+ p := params[i]
+ if p.GetValue() == nil {
+ return nil, fmt.Errorf("parameter #%d has no value",
i+1)
+ }
Review Comment:
PreparedStatement.Bind can panic if the params slice contains a nil element
(since it calls p.GetValue() without checking p != nil). This can happen with
repeated message fields in Go (they are []*T) and would turn a client error
into a server crash. Add a nil check before dereferencing the parameter.
--
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]
