From 74c916842d04aa63aedb7f44f1caa240babdbf92 Mon Sep 17 00:00:00 2001 From: Kermit Alexander II Date: Wed, 10 Aug 2022 00:30:25 +0000 Subject: [PATCH 01/10] Fix nested CallExpr cost estimation. --- checker/cost.go | 3 +++ checker/cost_test.go | 9 +++++++++ 2 files changed, 12 insertions(+) diff --git a/checker/cost.go b/checker/cost.go index eabd9bdb..f2f9b54f 100644 --- a/checker/cost.go +++ b/checker/cost.go @@ -476,6 +476,9 @@ func (c *coster) sizeEstimate(t AstNode) SizeEstimate { if l := c.estimator.EstimateSize(t); l != nil { return *l } + if _, ok := t.Expr().ExprKind.(*exprpb.Expr_CallExpr); ok { + return SizeEstimate{Min: 1, Max: 1} + } return SizeEstimate{Min: 0, Max: math.MaxUint64} } diff --git a/checker/cost_test.go b/checker/cost_test.go index e2118f87..d4e15d4c 100644 --- a/checker/cost_test.go +++ b/checker/cost_test.go @@ -354,6 +354,15 @@ func TestCost(t *testing.T) { hints: map[string]int64{"str1": 10, "str2": 10}, wanted: CostEstimate{Min: 2, Max: 6}, }, + { + name: "list size comparison", + expr: `list1.size() == list2.size()`, + decls: []*exprpb.Decl{ + decls.NewVar("list1", decls.NewListType(decls.Int)), + decls.NewVar("list2", decls.NewListType(decls.Int)), + }, + wanted: CostEstimate{Min: 5, Max: 5}, + }, } for _, tc := range cases { From dd58ff5d058495839625a444992fab6b0dc95e95 Mon Sep 17 00:00:00 2001 From: Kermit Alexander II Date: Wed, 10 Aug 2022 18:38:22 +0000 Subject: [PATCH 02/10] Add explanatory comment. --- checker/cost.go | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/checker/cost.go b/checker/cost.go index f2f9b54f..054744da 100644 --- a/checker/cost.go +++ b/checker/cost.go @@ -476,6 +476,11 @@ func (c *coster) sizeEstimate(t AstNode) SizeEstimate { if l := c.estimator.EstimateSize(t); l != nil { return *l } + // return a constant size if we're dealing with a CallExpr and EstimateSize + // came up empty; this code is reached when a CallExpr is nested inside + // another CallExpr and doesn't have an estimator that can handle it, such + // as `(1 == 2) == (3 == 4)` - without this block, that expression would + // return SizeEstimate{Min: 0, Max: math.MaxUint64} if _, ok := t.Expr().ExprKind.(*exprpb.Expr_CallExpr); ok { return SizeEstimate{Min: 1, Max: 1} } From ed8e42b9c08a6f98296de7d12b59bf1111251311 Mon Sep 17 00:00:00 2001 From: Kermit Alexander II Date: Wed, 10 Aug 2022 18:39:06 +0000 Subject: [PATCH 03/10] Add additional tests. --- checker/cost_test.go | 25 +++++++++++++++++++++++++ 1 file changed, 25 insertions(+) diff --git a/checker/cost_test.go b/checker/cost_test.go index d4e15d4c..6747b39e 100644 --- a/checker/cost_test.go +++ b/checker/cost_test.go @@ -363,6 +363,31 @@ func TestCost(t *testing.T) { }, wanted: CostEstimate{Min: 5, Max: 5}, }, + { + name: "list size from ternary", + expr: `x > y ? list1.size() : list2.size()`, + decls: []*exprpb.Decl{ + decls.NewVar("x", decls.Int), + decls.NewVar("y", decls.Int), + decls.NewVar("list1", decls.NewListType(decls.Int)), + decls.NewVar("list2", decls.NewListType(decls.Int)), + }, + wanted: CostEstimate{Min: 5, Max: 5}, + }, + { + name: "str endsWith inequality", + expr: `str1.endsWith("abcdefghijklmnopqrstuvwxyz") == str2.endsWith("abcdefghijklmnopqrstuvwxyz")`, + decls: []*exprpb.Decl{ + decls.NewVar("str1", decls.String), + decls.NewVar("str2", decls.String), + }, + wanted: CostEstimate{Min: 9, Max: 9}, + }, + { + name: "nested subexpression operators", + expr: `((5 != 6) == (1 == 2)) == ((3 <= 4) == (9 != 9))`, + wanted: CostEstimate{Min: 7, Max: 7}, + }, } for _, tc := range cases { From 7cb304a3d7bbe6349d37f066cfd56de0d3686f19 Mon Sep 17 00:00:00 2001 From: Kermit Alexander II Date: Thu, 11 Aug 2022 15:18:17 +0000 Subject: [PATCH 04/10] Correct test name. --- checker/cost_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/checker/cost_test.go b/checker/cost_test.go index 6747b39e..31e361c0 100644 --- a/checker/cost_test.go +++ b/checker/cost_test.go @@ -375,7 +375,7 @@ func TestCost(t *testing.T) { wanted: CostEstimate{Min: 5, Max: 5}, }, { - name: "str endsWith inequality", + name: "str endsWith equality", expr: `str1.endsWith("abcdefghijklmnopqrstuvwxyz") == str2.endsWith("abcdefghijklmnopqrstuvwxyz")`, decls: []*exprpb.Decl{ decls.NewVar("str1", decls.String), From 1fd75fd45f4307533204343afa5fa13b8b97e831 Mon Sep 17 00:00:00 2001 From: Kermit Alexander II Date: Thu, 11 Aug 2022 15:34:31 +0000 Subject: [PATCH 05/10] Add check on CallExprs returning scalars. --- checker/cost.go | 9 ++++++++- checker/cost_test.go | 9 +++++++++ 2 files changed, 17 insertions(+), 1 deletion(-) diff --git a/checker/cost.go b/checker/cost.go index 054744da..371f6885 100644 --- a/checker/cost.go +++ b/checker/cost.go @@ -482,7 +482,14 @@ func (c *coster) sizeEstimate(t AstNode) SizeEstimate { // as `(1 == 2) == (3 == 4)` - without this block, that expression would // return SizeEstimate{Min: 0, Max: math.MaxUint64} if _, ok := t.Expr().ExprKind.(*exprpb.Expr_CallExpr); ok { - return SizeEstimate{Min: 1, Max: 1} + // ensure we only return an estimate of 1 for return types of set + // lengths, since strings/bytes/more complex objects could be of + // variable length + if kindOf(t.Type()) == kindPrimitive { + if t.Type().GetPrimitive() != exprpb.Type_STRING && t.Type().GetPrimitive() != exprpb.Type_BYTES { + return SizeEstimate{Min: 1, Max: 1} + } + } } return SizeEstimate{Min: 0, Max: math.MaxUint64} } diff --git a/checker/cost_test.go b/checker/cost_test.go index 31e361c0..ce40c297 100644 --- a/checker/cost_test.go +++ b/checker/cost_test.go @@ -388,6 +388,15 @@ func TestCost(t *testing.T) { expr: `((5 != 6) == (1 == 2)) == ((3 <= 4) == (9 != 9))`, wanted: CostEstimate{Min: 7, Max: 7}, }, + { + name: "str size estimate", + expr: `string(timestamp1) == string(timestamp2)`, + decls: []*exprpb.Decl{ + decls.NewVar("timestamp1", decls.Timestamp), + decls.NewVar("timestamp2", decls.Timestamp), + }, + wanted: CostEstimate{Min: 5, Max: 1844674407370955268}, + }, } for _, tc := range cases { From e22cbc279bd53f43bdbc65d0b1474e2b1fb8e41e Mon Sep 17 00:00:00 2001 From: Kermit Alexander II Date: Thu, 11 Aug 2022 22:26:08 +0000 Subject: [PATCH 06/10] Add isScalar. --- checker/cost.go | 15 +++++++++++---- 1 file changed, 11 insertions(+), 4 deletions(-) diff --git a/checker/cost.go b/checker/cost.go index 371f6885..920e3d80 100644 --- a/checker/cost.go +++ b/checker/cost.go @@ -485,10 +485,8 @@ func (c *coster) sizeEstimate(t AstNode) SizeEstimate { // ensure we only return an estimate of 1 for return types of set // lengths, since strings/bytes/more complex objects could be of // variable length - if kindOf(t.Type()) == kindPrimitive { - if t.Type().GetPrimitive() != exprpb.Type_STRING && t.Type().GetPrimitive() != exprpb.Type_BYTES { - return SizeEstimate{Min: 1, Max: 1} - } + if isScalar(t.Type()) { + return SizeEstimate{Min: 1, Max: 1} } } return SizeEstimate{Min: 0, Max: math.MaxUint64} @@ -614,3 +612,12 @@ func (c *coster) newAstNode(e *exprpb.Expr) *astNode { } return &astNode{path: path, t: c.getType(e), expr: e, derivedSize: derivedSize} } + +func isScalar(t *exprpb.Type) bool { + if kindOf(t) == kindPrimitive { + if t.GetPrimitive() != exprpb.Type_STRING && t.GetPrimitive() != exprpb.Type_BYTES { + return true + } + } + return false +} From 53c8f2bf6d87c2daf1c642d849e11e385551acdf Mon Sep 17 00:00:00 2001 From: Kermit Alexander II Date: Fri, 12 Aug 2022 20:29:16 +0000 Subject: [PATCH 07/10] Cover non-primitive types in isScalar. --- checker/cost.go | 37 ++++++++++++++++++++++++------------- checker/cost_test.go | 18 ++++++++++++++++++ 2 files changed, 42 insertions(+), 13 deletions(-) diff --git a/checker/cost.go b/checker/cost.go index 920e3d80..a171879a 100644 --- a/checker/cost.go +++ b/checker/cost.go @@ -476,18 +476,11 @@ func (c *coster) sizeEstimate(t AstNode) SizeEstimate { if l := c.estimator.EstimateSize(t); l != nil { return *l } - // return a constant size if we're dealing with a CallExpr and EstimateSize - // came up empty; this code is reached when a CallExpr is nested inside - // another CallExpr and doesn't have an estimator that can handle it, such - // as `(1 == 2) == (3 == 4)` - without this block, that expression would - // return SizeEstimate{Min: 0, Max: math.MaxUint64} - if _, ok := t.Expr().ExprKind.(*exprpb.Expr_CallExpr); ok { - // ensure we only return an estimate of 1 for return types of set - // lengths, since strings/bytes/more complex objects could be of - // variable length - if isScalar(t.Type()) { - return SizeEstimate{Min: 1, Max: 1} - } + // return an estimate of 1 for return types of set + // lengths, since strings/bytes/more complex objects could be of + // variable length + if isScalar(t.Type()) { + return SizeEstimate{Min: 1, Max: 1} } return SizeEstimate{Min: 0, Max: math.MaxUint64} } @@ -614,10 +607,28 @@ func (c *coster) newAstNode(e *exprpb.Expr) *astNode { } func isScalar(t *exprpb.Type) bool { - if kindOf(t) == kindPrimitive { + switch kindOf(t) { + case kindPrimitive: if t.GetPrimitive() != exprpb.Type_STRING && t.GetPrimitive() != exprpb.Type_BYTES { return true } + case kindWellKnown: + if t.GetWellKnown() == exprpb.Type_DURATION || t.GetWellKnown() == exprpb.Type_TIMESTAMP { + return true + } + case kindObject: + switch t.GetMessageType() { + case "google.protobuf.Duration", "google.protobuf.Timestamp", + "google.protobuf.BoolValue", "google.protobuf.BytesValue", + "google.protobuf.DoubleValue", "google.protobuf.FloatValue", + "google.protobuf.Int32Value", "google.protobuf.Int64Value", + "google.protobuf.UInt32Value", "google.protobuf.UInt64Value": + return true + default: + return false + } + default: + return false } return false } diff --git a/checker/cost_test.go b/checker/cost_test.go index ce40c297..4e8cb9b4 100644 --- a/checker/cost_test.go +++ b/checker/cost_test.go @@ -397,6 +397,24 @@ func TestCost(t *testing.T) { }, wanted: CostEstimate{Min: 5, Max: 1844674407370955268}, }, + { + name: "timestamp equality check", + expr: `timestamp1 == timestamp2`, + decls: []*exprpb.Decl{ + decls.NewVar("timestamp1", decls.Timestamp), + decls.NewVar("timestamp2", decls.Timestamp), + }, + wanted: CostEstimate{Min: 3, Max: 3}, + }, + { + name: "duration inequality check", + expr: `duration1 != duration2`, + decls: []*exprpb.Decl{ + decls.NewVar("duration1", decls.Duration), + decls.NewVar("duration2", decls.Duration), + }, + wanted: CostEstimate{Min: 3, Max: 3}, + }, } for _, tc := range cases { From e5b8bcde518fcecc3ec22c0a1756c712840f7330 Mon Sep 17 00:00:00 2001 From: Kermit Alexander II Date: Mon, 15 Aug 2022 19:15:38 +0000 Subject: [PATCH 08/10] Add godoc to isScalar. --- checker/cost.go | 3 +++ 1 file changed, 3 insertions(+) diff --git a/checker/cost.go b/checker/cost.go index a171879a..7c67863f 100644 --- a/checker/cost.go +++ b/checker/cost.go @@ -606,6 +606,9 @@ func (c *coster) newAstNode(e *exprpb.Expr) *astNode { return &astNode{path: path, t: c.getType(e), expr: e, derivedSize: derivedSize} } +// isScalar returns true if the given type is known to be of a constant size at +// compile time. isScalar will return false for strings (they are variable-width) +// in addition to protobuf.Any and protobuf.Value (their size is not knowable at compile time). func isScalar(t *exprpb.Type) bool { switch kindOf(t) { case kindPrimitive: From b8b0f65022c87e4fd06442c392596e380786beb8 Mon Sep 17 00:00:00 2001 From: Kermit Alexander II Date: Mon, 15 Aug 2022 19:58:22 +0000 Subject: [PATCH 09/10] Remove kindObject case in isScalar. --- checker/cost.go | 13 ------------- 1 file changed, 13 deletions(-) diff --git a/checker/cost.go b/checker/cost.go index 7c67863f..70d96b1c 100644 --- a/checker/cost.go +++ b/checker/cost.go @@ -619,19 +619,6 @@ func isScalar(t *exprpb.Type) bool { if t.GetWellKnown() == exprpb.Type_DURATION || t.GetWellKnown() == exprpb.Type_TIMESTAMP { return true } - case kindObject: - switch t.GetMessageType() { - case "google.protobuf.Duration", "google.protobuf.Timestamp", - "google.protobuf.BoolValue", "google.protobuf.BytesValue", - "google.protobuf.DoubleValue", "google.protobuf.FloatValue", - "google.protobuf.Int32Value", "google.protobuf.Int64Value", - "google.protobuf.UInt32Value", "google.protobuf.UInt64Value": - return true - default: - return false - } - default: - return false } return false } From ae3b3057881d9ecb1392c0b658f89269a8c96abf Mon Sep 17 00:00:00 2001 From: Kermit Alexander II Date: Tue, 16 Aug 2022 19:35:26 +0000 Subject: [PATCH 10/10] Add TODO about merging isScalar/ComputedSize. --- checker/cost.go | 3 +++ 1 file changed, 3 insertions(+) diff --git a/checker/cost.go b/checker/cost.go index 70d96b1c..7312d1fe 100644 --- a/checker/cost.go +++ b/checker/cost.go @@ -480,6 +480,9 @@ func (c *coster) sizeEstimate(t AstNode) SizeEstimate { // lengths, since strings/bytes/more complex objects could be of // variable length if isScalar(t.Type()) { + // TODO: since the logic for size estimation is split between + // ComputedSize and isScalar, changing one will likely require changing + // the other, so they should be merged in the future if possible return SizeEstimate{Min: 1, Max: 1} } return SizeEstimate{Min: 0, Max: math.MaxUint64}