Add Value::Dynamic variant
#92
Comments
|
#43 proposal only works on root, having a @chirino, what's your oppinion on this? Same linear time evaluation remark as you've noted in #47 applies here. But it's not something I'd adhere to at all costs because it's not a good metric. In this context, constant time means evaluating expression always takes 1000ms because of non-negotiable upfront cost, while variable complexity would be 20ms-1000ms based on which fields are being read; if an HTTP request has to happen, it's still got to happen, be it handled by registered handler or code that uses |
|
I've been wondering whether the laziness should really be applied at interpreter time. Obviously that's the only way to avoid populating unnecessary state, but in CEL that's relatively rare. So, wouldn't it be more sensible to used the parsed expression to resolve all |
|
@alexsnaps are you proposing that there just be an additional step where we clone & convert types into CEL values prior to running the interpreter? This is a crude, non-working example but just want to make sure I understand what you're suggesting. let mut context = Context::default();
let values = HashMap::from([("github", ...)])
let program = Program::compile("github.orgs['clarkmcc'].projects['cel-rust'].license == true");
program.resolve_or_something(&mut context, values).unwrap(); // <-
let result = program.execute(&context); |
|
Yeah, something from the crate could help, but I'm (literally right now started) doing it outside of it... Something analogous to |
|
Still WIP, but here is a passing test for what this would look like... or at least the first part of it, i.e. finding all the paths. I'm working on populating that |
Depends how you look at it. Strictly speaking no, because string has no members. But after parsing, if you're not using a literal, you'll have
I think the cost of 1 extra enum variant (maybe a larger jump/cache miss?), isn't really that severe for how much it simplifies the code I'm working on.
What benefits does it offer over having lazy values? You still do the same thing, only now with 5 extra steps, 1 extra place where you need to transcribe data structs into code, 1 additional dependency, and more redundant compute/work. Each place you'd try running a CEL
I personally don't want to do that, it's just a lazy value, why would I need to handle parsing and expression traversal just to run Ideally, you'd construct It's also unsafe to assume |
Haha! Obviously, my bad, I meant
Are you talking about the
Can you elaborate here what the use case is? I.e. what's changing the most, the CEL expressions themselves? Or the
Did you look at the "reference" implementation, how do you think that'd work there? Anyways, a bit like how |
Returned from internally stored variable, that's inferred from result type of the provided function: pub struct LazyValue {
eval: Box<dyn ErasedProducer>,
value_type: ValueType,
current: Option<Value>,
}
impl LazyValue {
pub fn new<R>(ctor: impl Into<Producer<R>>);
pub fn value_type(&self) -> ValueType;
pub fn reset(&mut self);
}
// Incomplete type projection magic:
type Producer<R> = Box<dyn Fn(&Value) -> R>;
trait ErasedProducer;
impl ErasedProducer for Fn(&Value) -> impl ResultType;
trait ResultType; // projects `R` into `ValueType` stored in `ErasedProducer`
impl ResultType for Into<Value>;Calling
I assume it would use something similar to above, and then use the
I'm not familiar with protobuf though, not sure if it has a similar concept, but if it does, then it's not possible to construct |
|
If I have map with millions of keys, and I want to avoid converting all those keys to Cel values, would this lazy idea help? Or do you still end up inserting millions of lazy entries into the cel context? |
My question was unclear here, I was wondering how (if at all) one would address that with the golang implementation without changing it. The golang version is much more complete than the Rust one here... |
For the most part no. It's only useful for cases like the one you show in #43 where you'd use a jump table for lookup in resolver. If the data is fully dynamic (e.g. value of each field is This mostly addresses high evaluation cost members might carry, not as much Map length/memory limitations. I'd personally approach dynamic members via a trait (trait from #76, example impl in #96) - it's not dissimilar to #58, the difference is it uses a trait that can be inserted at any level of the tree structure and the parent of If both concepts were implemented, I see myself using both.
That's not a question then, but a statement: "I don't think this is a good suggestion because it deviates from reference implementation." I agree, it does. It also doesn't interfere with existing functionality/spec. Spec expects all data present in CEL to be precomputed (in order to serialize it with protobuf), which isn't ideal in some use cases. The example you provided includes an additional lexing pass just to collect that data in advance. If fetching expensive data is common enough, it makes more sense to provide an ergonomic way to do it easily instead of expecting users to handle constructing dynamic data structures based on used identifiers in the expression (because that will be slower 90% of the time). My requirements differ enough that I'm using a fork of this library, but I still see no reason why this is a bad idea. And "reference implementation doesn't have it" isn't a good reason IMO (see Weston). It can be controlled with a feature in case you need no-alloc or have a kink for unsafely parsing all your CEL expressions to check for requirements. |
It is a question. I really don't know if that is feasible in a way or another. I was not arguing that it shouldn't be done because the reference implementation doesn't do it. I'd argue that out of the box, it'd be desirable for this crate to behave as the golang implementation does, if only to provide portability of CEL expressions (and their behavior, including in terms of data accesses) across the two implementations. But as you rightfully mentioned, things can be extended in non "compliant" ways (through a feature flag, which has been discussed even in the golang implementation already) or made possible through the introductions of certain hooks and/or SPIs. So from your answer tho, I guess you do know this isn't feasible with the golang implementation neither, is that right?
This is where it's hard to understand where you are coming from, given how little you share about the problem and rather solely focus on your solution. Let me showcase: in my case, |
Sorry, it seemed like a loaded question.
I'm not the best person to ask (as I've said before) as I've never touched protobufs and have very limited experience with golang. It's not currently supported, but it is feasible. Looking at the reference implementation, The issue with representation is that a That's why I said there's no way to actually send
Oh, pfff, me? Well, uhm... 👉👈 I'm, of course, working on a ✨ground breaking✨ labeling action for GitHub. Randomly stumbled across this crate to parse expressions like: - if: commit.body.has_word("Wayland") && !commit.body.has_word("X11")
then:
- add_label("wayland")
- add_to_milestone("Wayland")
- if: comment.matches("$assign: @(\w+)") && user.role == "owner"
then:
- assign(matches[1])And now I'm pushing for changes on this project - don't you love FOSS?! /j In my case I have a single file that's used by multiple contexts - if So I don't use protobuf, I simply need something that will evaluate expressions and don't feel like writing everything from scratch. I could likely do batching as well if I switched to GitHub GraphQL API, but that feels like more trouble than it's worth atm. Anyway, |
|
Thanks for sharing what the context of your problem! In the meantime, I've looked up the golang implementation a little further, and I wonder if we're not missing blocks that would actually let us decouple the process of hydrating the whole context upon creation from the crate. Still investigating this and might even start some exploratory work, but I think here are the blocks we miss:
But wondering for now, if adding a A bit of a rambling, but seeing how the reference implementation does it, i.e. having an open set of |
#96 +/- minor tweaks, does precisely that. And allows for things that aren't sanctioned by the spec such as
This sounds more or less like what I want: pub struct DynValue {
eval: Box<Fn(&Context) -> Value>,
current: RefCell<Option<Value>>,
}
impl DynValue {
pub fn new<F>(ctor: F) where F: Fn(&Context) -> Value;
// simplified - without explicit RefCell borrowing
pub fn value_type(&self, ctx: &Context) -> ValueType {
if let Some(current) = self.current {
current.value_type()
} else {
let value = (self.eval)(ctx);
self.current = Some(value);
value.value_type()
}
}
#[cfg(feature="non_spec_compliant")]
pub fn reset(&mut self);
}"unknown layout before evaluation" is basically what I'm pushing for (I think). Layout is determined by type, so something without a layout can conceptually be:
Other than that, unsized types ( I'm repurposing this issue for |
|
Howdy everyone, sorry I was on vacation last week and am just catching up on this. Let me summarize and see if I'm following. There are two general proposals:
Am I understanding correctly where the discussion has landed? I need to go brush up on DynValue in the Go implementation and get a better handle on how that works, and what it means for #80. |
|
I think option 2 can absolutely be made compliant with the spec, as this is how the golang impl does gets it's input // Val interface defines the functions supported by all expression values.
// Val implementations may specialize the behavior of the value through the addition of traits.
type Val interface {
// ConvertToNative converts the Value to a native Go struct according to the
// reflected type description, or error if the conversion is not feasible.
//
// The ConvertToNative method is intended to be used to support conversion between CEL types
// and native types during object creation expressions or by clients who need to adapt the,
// returned CEL value into an equivalent Go value instance.
//
// When implementing or using ConvertToNative, the following guidelines apply:
// - Use ConvertToNative when marshalling CEL evaluation results to native types.
// - Do not use ConvertToNative within CEL extension functions.
// - Document whether your implementation supports non-CEL field types, such as Go or Protobuf.
ConvertToNative(typeDesc reflect.Type) (any, error)
// ConvertToType supports type conversions between CEL value types supported by the expression language.
ConvertToType(typeValue Type) Val
// Equal returns true if the `other` value has the same type and content as the implementing struct.
Equal(other Val) Val
// Type returns the TypeValue of the value.
Type() Type
// Value returns the raw value of the instance which may not be directly compatible with the expression
// language types.
Value() any
}
// NewActivation returns an activation based on a map-based binding where the map keys are
// expected to be qualified names used with ResolveName calls.
//
// The input `bindings` may either be of type `Activation` or `map[string]any`.
//
// Lazy bindings may be supplied within the map-based input in either of the following forms:
// - func() any
// - func() ref.Val
//
// The output of the lazy binding will overwrite the variable reference in the internal map.
//
// Values which are not represented as ref.Val types on input may be adapted to a ref.Val using
// the types.Adapter configured in the environment.
func NewActivation(bindings any) (Activation, error) {
if bindings == nil {
return nil, errors.New("bindings must be non-nil")
}
a, isActivation := bindings.(Activation)
if isActivation {
return a, nil
}
m, isMap := bindings.(map[string]any)
if !isMap {
return nil, fmt.Errorf(
"activation input must be an activation or map[string]interface: got %T",
bindings)
}
return &mapActivation{bindings: m}, nil
} |
It would be a nice improvement if lazy values were supported (like
std::cell::LazyCell).These variables would evaluate their value only when used, at which point they'd turn into some other
Value::Variant.Example where this would be useful is having variables bind to an external API and evaluating all of them if they're not used is expensive.
There's ways of achieving this functionality already (with functions), but it would be nice if users could refer to them with
member.syntax.The text was updated successfully, but these errors were encountered: