Add Context::get_value_cow()

[mtopolnik]

We are trying to use evalexpr, but a small missing detail is preventing us from using it effectively.

The way the contract of Context is defined, get_value() is forced to return a reference to a value it owns, but in our case we'd like to return a computed value without holding on to it.

Specifically, we want to iterate over a whole table of data, and each row we visit should be another "context" for the expression. With the current design we're forced to create an ephemeral hash map on each iteration step and fill it with the values. This becomes the bottleneck of the overall computation.

Our proposed change is to introduce a new method to Context, get_value_cow(), which returns a Cow and thus covers both the owned and the referenced cases. This would be a non-breaking change because get_value_cow() comes with a default implementation that delegates to the existing get_value(). Here's how it looks for our implementation:

impl<'a> evalexpr::Context for TableRow<'a> {
    fn get_value_cow(&self, identifier: &str) -> Option<std::borrow::Cow<'_, evalexpr::Value>> {
        self.table.peek(identifier, |col| Cow::Owned(col.get(self.row).into()))
    }

    fn get_value(&self, _: &str) -> Option<&evalexpr::Value> {
        panic!("Use get_value_cow");
    }

    fn call_function(&self, identifier: &str, _: &evalexpr::Value) -> EvalexprResult<evalexpr::Value> {
        Result::Err(EvalexprError::FunctionIdentifierNotFound(identifier.to_owned()))
    }
}

[ISibboI]

Thanks, this looks interesting. I think though that it is better if there is just one get_value() method, so we should replace the return type of the existing method, rather than adding another one.

This will make it harder to implement a custom context though, so the get_value() method's documentation should contain an example of how to implement it e.g. for a stdlib HashMap.

Could you make the required changes?

[mtopolnik]

Thanks for the reply, I pushed the new idea, I hope I understood it correctly.

[ISibboI]

Hi, sorry, so I think there was a misunderstanding. I would prefer having a function get_value(&self, &identifier) -> Option<std::borrow::Cow<'_, evalexpr::Value>>.

So only have a function that returns a Cow.

[mtopolnik]

Sorry for the delay, GitHub notifications weren't reaching me.

I'm making the change to use Option<Cow<'_, Value>>, but when I do it, I notice this is its only usage:

if let Some(value) = context.get_value(identifier) {
    Ok(value.into_owned())
}

So the Cow turns into an owned value upon first contact with it. Isn't it simpler to just ask for an owned value right away?