Open goccy opened 7 months ago
I've confirmed that all tests pass. Please review this 🙏 .
/gcbrun
@goccy I'll give it a look. If there are changes in the top-level APIs, I'll probably ask for parallel implementations for things like unaryOpContext
, but I did a quick first pass and have a good gist of what you're aiming for
@goccy looks like there were some failures in the sub-modules:
Step #1 - "bazel-test": repl/evaluator.go:235:14: cannot use func(v ref.Val) ref.Val {…} (value of type func(v ref.Val) ref.Val) as type functions.UnaryContextOp in struct literal
Step #1 - "bazel-test": repl/evaluator.go:240:14: cannot use func(lhs ref.Val, rhs ref.Val) ref.Val {…} (value of type func(lhs ref.Val, rhs ref.Val) ref.Val) as type functions.BinaryContextOp in struct literal
Step #1 - "bazel-test": repl/evaluator.go:245:14: cannot use l.impl (variable of type functions.FunctionOp) as type functions.FunctionContextOp in struct literal
@TristonianJones Thank you for your quickly response. I've fixed that build error and I confirmed passing all tests by bazel test //...
@TristonianJones
If breaking changes to the interface of interpreter.Interpretable
or interpreter.Qualifier
etc are not allowed, would it be appropriate to add the following APIs for context to each ?
type Interpretable {
Eval(activation Activation) ref.Val
+ EvalContext(ctx context.Context, activation Activation) ref.Val
}
type Qualifier {
Qualify(vars Activation, obj any) (any, error)
QualifyIfPresent(vars Activation, obj any, presenceOnly bool) (any, bool, error)
+ QualifyContext(ctx context.Context, vars Activation, obj any) (any, error)
+ QualifyIfPresentContext(ctx context.Context, vars Activation, obj any, presenceOnly bool) (any, bool, error)
}
Also, should I add the fields for context interface to the functions.Overload
?
@goccy Sorry for the delayed reply. I've been weighing this approach with a previous one we refer to as iterative-eval. The idea behind iterative eval is that context dependent functions are initially stubbed with synchronous versions which capture arguments and return unknown
. Then a resolve step would happen to execute the functions containing Context
arguments, cache the results, and re-evaluate. This process repeats until there are no more unknowns to resolve.
The benefit of such an approach is that it's async in terms of a synchronous process and that it allows for batching of async calls. The drawback is that it's somewhat complex and doesn't completely fit with user expectations regarding which functions are executed when.
Part of the reason we haven't adopted either approach is because of the plumbing changes required to support them. Effectively, you need parallel methods to the ones that exist today. Since I'm not sure who might be using the Attribute
interface, introducing new methods on that interface could be a breaking change for large projects due to golang's minimum version selection.
I'll need to think about this more. Thanks for your patience!
@TristonianJones Thank you for your explanation ! To avoid breaking changes, what about the following approach of defining a new interface and using it only internally? If this looks good, I will try to implement it.
type InterpretableContext interface {
Interpretable
EvalContext(ctx context.Context, activation Activation) ref.Val
}
type QualifierContext interface {
Qualifier
QualifyContext(ctx context.Context, vars Activation, obj any) (any, error)
QualifyIfPresentContext(ctx context.Context, vars Activation, obj any, presenceOnly bool) (any, bool, error)
}
@TristonianJones
I apologize for the additional mention, but I've implemented this without any breaking changes. I would appreciate it if you could review it when you have time.
context.Context
instance passed inContextEval()
can be propagated to binding function to cancel the process.First of all, thanks for the great OSS. We are using cel-go heavily in https://github.com/mercari/grpc-federation . In our OSS, we may want to pass
context.Context
against bound functions. e.g.) pass to the gRPC metadata or perform a cancel.Now that the
ContextEval
function is provided, so I fixed to thecontext.Context
instance is passed to the bound function.Although I have tried to keep the Interface as unchanged as possible, I have made some changes to the Public Interface. If there is a problem, please let me know so that I can correct it.
ref https://github.com/google/cel-go/issues/557
Pull Requests Guidelines
See CONTRIBUTING.md for more details about when to create a GitHub [Pull Request][1] and when other kinds of contributions or consultation might be more desirable.
When creating a new pull request, please fork the repo and work within a development branch.
Commit Messages
Background on why the change is being made with additional detail on consequences of the changes elsewhere in the code or to the general functionality of the library. Multiple paragraphs may be used, but please keep lines to 72 characters or less.