Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix nested CallExpr cost estimation #571

Merged
merged 10 commits into from Aug 17, 2022
Merged

Fix nested CallExpr cost estimation #571

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
74c9168
Fix nested CallExpr cost estimation.
DangerOnTheRanger Aug 10, 2022
dd58ff5
Add explanatory comment.
DangerOnTheRanger Aug 10, 2022
ed8e42b
Add additional tests.
DangerOnTheRanger Aug 10, 2022
7cb304a
Correct test name.
DangerOnTheRanger Aug 11, 2022
1fd75fd
Add check on CallExprs returning scalars.
DangerOnTheRanger Aug 11, 2022
e22cbc2
Add isScalar.
DangerOnTheRanger Aug 11, 2022
53c8f2b
Cover non-primitive types in isScalar.
DangerOnTheRanger Aug 12, 2022
e5b8bcd
Add godoc to isScalar.
DangerOnTheRanger Aug 15, 2022
b8b0f65
Remove kindObject case in isScalar.
DangerOnTheRanger Aug 15, 2022
ae3b305
Add TODO about merging isScalar/ComputedSize.
DangerOnTheRanger Aug 16, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
15 changes: 15 additions & 0 deletions checker/cost.go
View file
Open in desktop
Expand Up @@ -476,6 +476,21 @@ 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 {
DangerOnTheRanger marked this conversation as resolved.
Show resolved Hide resolved
DangerOnTheRanger marked this conversation as resolved.
Show resolved Hide resolved
// 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 {
DangerOnTheRanger marked this conversation as resolved.
Show resolved Hide resolved
return SizeEstimate{Min: 1, Max: 1}
}
}
}
return SizeEstimate{Min: 0, Max: math.MaxUint64}
}

Expand Down
43 changes: 43 additions & 0 deletions checker/cost_test.go
View file
Open in desktop
Expand Up @@ -354,6 +354,49 @@ 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()`,
DangerOnTheRanger marked this conversation as resolved.
Show resolved Hide resolved
decls: []*exprpb.Decl{
decls.NewVar("list1", decls.NewListType(decls.Int)),
decls.NewVar("list2", decls.NewListType(decls.Int)),
},
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 equality",
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},
},
{
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 {
Expand Down