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

With introduction of null, it's better to allow short circuiting (lazy evaluation) for those boolean operators #427

Closed
stevenylai opened this issue Jan 12, 2024 · 4 comments · Fixed by #432

Comments

@stevenylai
Copy link
Contributor

stevenylai commented Jan 12, 2024

I see the library now support null but perhaps it's better to also allow short circuiting for those boolean operator? Consider an example:

IF(v != NULL && v > 0, LOG(v), 0)

If v is set to null, we will get an exception saying null cannot be compared.

It seems that operators don't have the option to be lazy? So perhaps the workaround here is to introduce an AND function:

IF(AND(v != NULL, v > 0), LOG(v), 0)
@stevenylai stevenylai changed the title With introduction of null, it's better to allow short circuiting for those boolean operators With introduction of null, it's better to allow short circuiting (lazy evaluation) for those boolean operators Jan 12, 2024
@uklimaschewski
Copy link
Collaborator

I think this can be easily implemented in the expression evaluator here

      case INFIX_OPERATOR:
        result =
            token
                .getOperatorDefinition()
                .evaluate(
                    this,
                    token,
                    evaluateSubtree(startNode.getParameters().get(0)),
                    evaluateSubtree(startNode.getParameters().get(1)));
        break;

The operator type has to be checked. If it is a logical AND, then evaluate the first operand only.
Only, if the result is TRUE, evaluate also the second operand.

@uklimaschewski
Copy link
Collaborator

Similar logic can be implemented for the logical OR operator. If the first operand is TRUE, skip evaluating the second operand.

@stevenylai
Copy link
Contributor Author

OK. I see. I think given that user can define custom operators, we can define an additional isLazy parameter in InfixOperator interface (assuming post/prefix operators do not need the same) and implement the lazy evaluation logic only for AND/OR operators. Documentation will also have to be updated.

I can also help to make the change if this sounds reasonable.

@uklimaschewski
Copy link
Collaborator

This is exactly what I had in mind. If there is no breaking changes, it could go to a 3.2.0 version.
Feel free to volunteer, I am always happy with people contributing. 🙂

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants