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

Presence test cost tracking options #721

Merged
merged 3 commits into from May 30, 2023

Conversation

TristonianJones
Copy link
Collaborator

@TristonianJones TristonianJones commented May 30, 2023

In #711, presence tests began getting tracked with the same cost as a typical select expression, but this may break some users. A new set of functional options have been added to both the checker and interpreter packages to allow users to opt-out of this change.

Also, note this change breaks the current API contract for interpreter.CostTracker and checker.Cost. Prefer using interpreter.NewCostTracker() which accepts a set of functional arguments for configuring behavior and which may return an error. Likewise, the checker.Cost will return CostEstimate, error whereas before it did not. Note, the top-level cel.EstimateCost() function signature remains unchanged.

Copy link
Collaborator

@jpbetz jpbetz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

On by default makes sense to me for this. Appreciate the inclusion of a flag so we can disable and get the legacy behavior.

@TristonianJones TristonianJones merged commit f8fc7b9 into google:master May 30, 2023
2 checks passed
@TristonianJones TristonianJones deleted the cost-tracking-flag branch May 30, 2023 19:37
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

Successfully merging this pull request may close these issues.

None yet

2 participants