OpenRefine / OpenRefine

OpenRefine is a free, open source power tool for working with messy data and improving it
https://openrefine.org/
BSD 3-Clause "New" or "Revised" License
10.91k stars 1.96k forks source link

GREL AND boolean function is not short-circuit evaluation? #1145

Open ignacio-chiazzo opened 8 years ago

ignacio-chiazzo commented 8 years ago

GREL Boolean Functions should be short-circuit.

AND(A,B) if A is not true, should not verify B. I get an error because, if A is not true B will throw me an error.

thadguidry commented 6 years ago

@wetneb Considering your comment in #1496 about this issue,

Do we consider this issue as not fixable/not warranted ? I need a use case for this issue presented so that it shows where something with GREL Eval causes tremendous grief for folks...otherwise 👎 Can you put that use case together for me to understand the impact of this issue ?

wetneb commented 6 years ago

This is definitely fixable by redesigning the way GREL is evaluated. I would say that it is not high priority because there are workarounds:

An example use case would be using guard conditions on an index before accessing an array, for instance.

In short: designing an expression language requires proper thinking and planning, not something we should be tweaking iteratively as issues arise (especially because users have the right to expect stability in the semantics). So yes, GREL is a toy language that is quite limited, but I don't think we should spend too much energy on fixing that: the long term solution is to use existing languages (and it makes onboarding of new users a lot easier, too).

wetneb commented 4 years ago

I actually think it would not be too hard to fix this, by changing and to be a control rather than a function. We should think carefully about the implications of this - there might be cases where that breaks workflows - but it should not be as hard as I thought to implement.

thadguidry commented 4 years ago

@wetneb It does break workflows that I rely on and other users. I think we can instead introduce Short-circuit functions and keep our Eager functions as is. I highlighted the reasons in the referenced issue #2121

## CURRENT Eager functions
or()
and()

## NEW Short-Circuit functions
orelse()
andalso()
wetneb commented 4 years ago

I'd be curious to see a concrete example of one of your workflows where changing this breaks anything?

thadguidry commented 4 years ago

Disregard that previous comment, Antonin. :) I tried a few things and it seems since we changed null handling as you mentioned in previous comments, there's no potential for breakage (an ERROR'ed evaluation) that I can see any longer. So I give up. Basically, it looks like GREL execution order is pretty solid now, but if anything ever changed that, then that is when separate Short-circuit functions might be useful to introduce.