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

[FR]Support Short-circuit for eval bool exp #155

Open
Leojangh opened this issue Nov 13, 2023 · 4 comments
Open

[FR]Support Short-circuit for eval bool exp #155

Leojangh opened this issue Nov 13, 2023 · 4 comments
Labels
enhancement New feature or request

Comments

@Leojangh
Copy link

    #[test]
    fn it_works() {
        let mut context = HashMapContext::new();
        // context.set_value("a".into(), false.into()).unwrap();
        let x = eval_boolean_with_context("true || a", &context).unwrap();
        println!("{}", x);
    }

It got an error: VariableIdentifierNotFound("a")。But if short-circuit is supported, it should be computeable.

@Leojangh Leojangh changed the title [Enhance]Support Short-circuit for eval bool exp [FR]Support Short-circuit for eval bool exp Nov 13, 2023
@ISibboI
Copy link
Owner

ISibboI commented Nov 13, 2023

This may be a useful feature. Feel free to write a pull request. I must unfortunately say though that I do not have enough time at the moment for code reviews for merging it fast.

@ISibboI ISibboI added the enhancement New feature or request label Nov 13, 2023
@Leojangh
Copy link
Author

Leojangh commented Dec 1, 2023

Hello, I have a rough understanding of the project and here is the plan I thought of.
It looks like the evaluation strategy is calculating every argument and then combining them with operator:

evalexpr/src/tree/mod.rs

Lines 323 to 329 in b39bbd7

pub fn eval_with_context<C: Context>(&self, context: &C) -> EvalexprResult<Value> {
let mut arguments = Vec::new();
for child in self.children() {
arguments.push(child.eval_with_context(context)?);
}
self.operator().eval(&arguments, context)
}

Maybe we can make the first step lazily:

    ///Lazy variant of `Node#eval_with_context`
    pub fn eval_with_context_lazily<C: Context>(&self, context: &C) -> EvalexprResult<Value> {
        self.operator.eval_lazily(&self.children(), context)
    }

So the actual variable reading from context happens when using it, then the feature might be supported.

        ...
        And => {
                expect_operator_argument_amount(children.len(), 2)?;
                let left = children[0].eval_with_context_lazily(context)?.as_boolean()?;

                Ok(Value::Boolean(left && children[1].eval_with_context_lazily(context)?.as_boolean()?))
            },
            Or => {
                expect_operator_argument_amount(children.len(), 2)?;
                let left = children[0].eval_with_context_lazily(context)?.as_boolean()?;

                Ok(Value::Boolean(left || children[1].eval_with_context_lazily(context)?.as_boolean()?))
            },
            Not => {
                expect_operator_argument_amount(children.len(), 1)?;
                let a = children[0].eval_with_context_lazily(context)?.as_boolean()?;

                Ok(Value::Boolean(!a))
            },
        ...

What is the best way? Add a lazy variant for eval_bool only or change the old?

@ISibboI
Copy link
Owner

ISibboI commented Dec 7, 2023

I would not add new methods for this. Rather have the context type decide if boolean expressions should be evaluated lazily, or not. So add a function to the context trait fn evaluate_boolean_expression_with_short_circuit(&self) -> bool (or some other name). If the function returns true, the operators should use short circuit evaluation, if not, then not.

I think it makes sense to have short circuit evaluation be the default, so the existing context types should just unconditionally return true for this method.

@bwsw
Copy link

bwsw commented Jan 16, 2024

@ISibboI you can consider using ControlFlow in std::ops for that.

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

No branches or pull requests

3 participants