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.

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

math.MaxUInt cost reported for equality/inequality operations on both Timestamp and Duration #573

Closed
1 of 3 tasks
DangerOnTheRanger opened this issue Aug 11, 2022 · 2 comments
Assignees

Comments

@DangerOnTheRanger
Copy link
Contributor

DangerOnTheRanger commented Aug 11, 2022

Describe the bug
Directly comparing a pair of Timestamps or Durations using either == or != will result in an expression with an estimated cost of math.MaxUInt. Other operators (>, <=, etc.) are not affected, and will return a low constant cost as expected. As far as I can tell, this is because operators besides ==/!= have type-specific overloads and thus fall through to the default case of coster.functionCost.

But this is not so for ==/!=, which are handled by the same case that handles all equality/inequality checks. coster.sizeEstimate, called a bit further down, doesn't know how to handle Timestamps/Durations and returns math.MaxUInt. This is a similar issue to what #571 fixes, though it's a bit different here - I think it should be fixable just by adding an extra overload on ==/!= for both Timestamps and Durations.

This should be resolved before a new release is cut to pick up #571 - I can take care of this issue as well (I'd self-assign but it seems I don't have the right permissions unfortunately).

To Reproduce
Check which components this affects:

  • parser
  • checker
  • interpreter

Sample expression and input that reproduces the issue:

timestamp1 == timestamp2
duration1 != duration2

Test setup (add the following tests to TestCost in cost_test.go):

{
	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},
},

Expected behavior
The tests should pass. However, on my system I get the following output:

--- FAIL: TestCost (0.14s)
    --- FAIL: TestCost/timestamp_equality_check (0.00s)
        cost_test.go:414: Got cost interval [3, 1844674407370955266], wanted [3, 3]
    --- FAIL: TestCost/duration_inequality_check (0.00s)
        cost_test.go:414: Got cost interval [3, 1844674407370955266], wanted [3, 3]
FAIL
exit status 1
FAIL    github.com/google/cel-go/checker        0.317s

Additional context
cel-go revision: 25bb4c6da1759f0104ce8ee935431d32b71977b6

@TristonianJones
Copy link
Collaborator

@DangerOnTheRanger Is this fixed with the merge of #571? I assume so, but wanted to check.

@DangerOnTheRanger
Copy link
Contributor Author

Yep, I'll go ahead and mark this as closed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants