Skip to content

Commit

Permalink
Unconditionally increment cost on evalTestOnly.
Browse files Browse the repository at this point in the history
  • Loading branch information
DangerOnTheRanger committed Mar 3, 2023
1 parent 22aee46 commit e2d28ac
Show file tree
Hide file tree
Showing 6 changed files with 14 additions and 25 deletions.
2 changes: 2 additions & 0 deletions checker/cost.go
Original file line number Diff line number Diff line change
Expand Up @@ -340,6 +340,8 @@ func (c *coster) costSelect(e *exprpb.Expr) CostEstimate {
sel := e.GetSelectExpr()
var sum CostEstimate
if sel.GetTestOnly() {
sum.Min = 1
sum.Max = 1
return sum
}
sum = sum.Add(c.cost(sel.GetOperand()))
Expand Down
2 changes: 1 addition & 1 deletion checker/cost_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -75,7 +75,7 @@ func TestCost(t *testing.T) {
name: "select: field test only",
expr: `has(input.single_int32)`,
decls: []*exprpb.Decl{decls.NewVar("input", decls.NewObjectType("google.expr.proto3.test.TestAllTypes"))},
wanted: zeroCost,
wanted: CostEstimate{Min: 1, Max: 1},
},
{
name: "estimated function call",
Expand Down
9 changes: 4 additions & 5 deletions interpreter/interpretable.go
Original file line number Diff line number Diff line change
Expand Up @@ -110,11 +110,10 @@ type InterpretableConstructor interface {
// Core Interpretable implementations used during the program planning phase.

type evalTestOnly struct {
id int64
attr InterpretableAttribute
qual Qualifier
field types.String
isProtoField bool
id int64
attr InterpretableAttribute
qual Qualifier
field types.String
}

// ID implements the Interpretable interface method.
Expand Down
16 changes: 4 additions & 12 deletions interpreter/planner.go
Original file line number Diff line number Diff line change
Expand Up @@ -220,19 +220,11 @@ func (p *planner) planSelect(expr *exprpb.Expr) (Interpretable, error) {

// Return the test only eval expression.
if sel.GetTestOnly() {
isProtoField := false
if opType.GetMessageType() != "" {
ft, found := p.provider.FindFieldType(opType.GetMessageType(), sel.GetField())
if found && ft.IsSet != nil && ft.GetFrom != nil {
isProtoField = true
}
}
return &evalTestOnly{
id: expr.GetId(),
field: types.String(sel.GetField()),
attr: attr,
qual: qual,
isProtoField: isProtoField,
id: expr.GetId(),
field: types.String(sel.GetField()),
attr: attr,
qual: qual,
}, nil
}

Expand Down
8 changes: 2 additions & 6 deletions interpreter/runtimecost.go
Original file line number Diff line number Diff line change
Expand Up @@ -38,12 +38,6 @@ type ActualCostEstimator interface {
func CostObserver(tracker *CostTracker) EvalObserver {
observer := func(id int64, programStep any, val ref.Val) {
switch t := programStep.(type) {
case *evalTestOnly:
// Protobuf has() checks do not incur runtime cost due to not using Eval()
// in older versions of CEL; this keeps the runtime cost behavior consistent
if !t.isProtoField {
tracker.cost++
}
case ConstantQualifier:
// TODO: Push identifiers on to the stack before observing constant qualifiers that apply to them
// and enable the below pop. Once enabled this can case can be collapsed into the Qualifier case.
Expand Down Expand Up @@ -75,6 +69,8 @@ func CostObserver(tracker *CostTracker) EvalObserver {
tracker.stack.drop(t.rhs.ID(), t.lhs.ID())
case *evalFold:
tracker.stack.drop(t.iterRange.ID())
case *evalTestOnly:
tracker.cost += common.SelectAndIdentCost
case Qualifier:
tracker.cost++
case InterpretableCall:
Expand Down
2 changes: 1 addition & 1 deletion interpreter/runtimecost_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -306,7 +306,7 @@ func TestRuntimeCost(t *testing.T) {
name: "select: field test only",
expr: `has(input.single_int32)`,
decls: []*exprpb.Decl{decls.NewVar("input", decls.NewObjectType("google.expr.proto3.test.TestAllTypes"))},
want: 0,
want: 1,
in: map[string]any{
"input": &proto3pb.TestAllTypes{
RepeatedBool: []bool{false},
Expand Down

0 comments on commit e2d28ac

Please sign in to comment.