Skip to content

Commit

Permalink
Boolean fix (#1148)
Browse files Browse the repository at this point in the history
* Fixing booleans

* Fixed "and", "or" evaluating RHS when not required
  • Loading branch information
mikefarah authored Mar 20, 2022
1 parent e08b680 commit 005b097
Show file tree
Hide file tree
Showing 4 changed files with 115 additions and 58 deletions.
3 changes: 2 additions & 1 deletion examples/data1.yaml
Original file line number Diff line number Diff line change
@@ -1 +1,2 @@
cat: purrs
a: mike
b: [t, f]
88 changes: 48 additions & 40 deletions pkg/yqlib/operator_booleans.go
Original file line number Diff line number Diff line change
Expand Up @@ -27,41 +27,48 @@ func isTruthy(c *CandidateNode) (bool, error) {
return isTruthyNode(node)
}

type boolOp func(bool, bool) bool
func getBoolean(candidate *CandidateNode) (bool, error) {
if candidate != nil {
candidate.Node = unwrapDoc(candidate.Node)
return isTruthy(candidate)
}
return false, nil
}

func performBoolOp(op boolOp) func(d *dataTreeNavigator, context Context, lhs *CandidateNode, rhs *CandidateNode) (*CandidateNode, error) {
return func(d *dataTreeNavigator, context Context, lhs *CandidateNode, rhs *CandidateNode) (*CandidateNode, error) {
owner := lhs
func getOwner(lhs *CandidateNode, rhs *CandidateNode) *CandidateNode {
owner := lhs

if lhs == nil && rhs == nil {
owner = &CandidateNode{}
} else if lhs == nil {
owner = rhs
}
if lhs == nil && rhs == nil {
owner = &CandidateNode{}
} else if lhs == nil {
owner = rhs
}
return owner
}

var errDecoding error
lhsTrue := false
if lhs != nil {
lhs.Node = unwrapDoc(lhs.Node)
lhsTrue, errDecoding = isTruthy(lhs)
func returnRhsTruthy(d *dataTreeNavigator, context Context, lhs *CandidateNode, rhs *CandidateNode) (*CandidateNode, error) {
owner := getOwner(lhs, rhs)
rhsBool, err := getBoolean(rhs)
if err != nil {
return nil, err
}

if errDecoding != nil {
return nil, errDecoding
}
return createBooleanCandidate(owner, rhsBool), nil
}

func returnLHSWhen(targetBool bool) func(lhs *CandidateNode) (*CandidateNode, error) {
return func(lhs *CandidateNode) (*CandidateNode, error) {
var err error
var lhsBool bool

if lhsBool, err = getBoolean(lhs); err != nil || lhsBool != targetBool {
return nil, err
}
log.Debugf("-- lhsTrue", lhsTrue)

rhsTrue := false
if rhs != nil {
rhs.Node = unwrapDoc(rhs.Node)
rhsTrue, errDecoding = isTruthy(rhs)
if errDecoding != nil {
return nil, errDecoding
}
owner := &CandidateNode{}
if lhs != nil {
owner = lhs
}
log.Debugf("-- rhsTrue", rhsTrue)

return createBooleanCandidate(owner, op(lhsTrue, rhsTrue)), nil
return createBooleanCandidate(owner, targetBool), nil
}
}

Expand Down Expand Up @@ -133,20 +140,21 @@ func anyOperator(d *dataTreeNavigator, context Context, expressionNode *Expressi
}

func orOperator(d *dataTreeNavigator, context Context, expressionNode *ExpressionNode) (Context, error) {
log.Debugf("-- orOp")
return crossFunction(d, context.ReadOnlyClone(), expressionNode, performBoolOp(
func(b1 bool, b2 bool) bool {
log.Debugf("-- peformingOrOp with %v and %v", b1, b2)
return b1 || b2
}), true)
prefs := crossFunctionPreferences{
CalcWhenEmpty: true,
Calculation: returnRhsTruthy,
LhsResultValue: returnLHSWhen(true),
}
return crossFunctionWithPrefs(d, context.ReadOnlyClone(), expressionNode, prefs)
}

func andOperator(d *dataTreeNavigator, context Context, expressionNode *ExpressionNode) (Context, error) {
log.Debugf("-- AndOp")
return crossFunction(d, context.ReadOnlyClone(), expressionNode, performBoolOp(
func(b1 bool, b2 bool) bool {
return b1 && b2
}), true)
prefs := crossFunctionPreferences{
CalcWhenEmpty: true,
Calculation: returnRhsTruthy,
LhsResultValue: returnLHSWhen(false),
}
return crossFunctionWithPrefs(d, context.ReadOnlyClone(), expressionNode, prefs)
}

func notOperator(d *dataTreeNavigator, context Context, expressionNode *ExpressionNode) (Context, error) {
Expand Down
27 changes: 26 additions & 1 deletion pkg/yqlib/operator_booleans_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,22 @@ var booleanOperatorScenarios = []expressionScenario{
"D0, P[], (doc)::b: hi\n",
},
},
{
skipDoc: true,
description: "And should not run 2nd arg if first is false",
expression: `false and test(3)`,
expected: []string{
"D0, P[], (!!bool)::false\n",
},
},
{
skipDoc: true,
description: "Or should not run 2nd arg if first is true",
expression: `true or test(3)`,
expected: []string{
"D0, P[], (!!bool)::true\n",
},
},
{
description: "`and` example",
expression: `true and false`,
Expand Down Expand Up @@ -151,12 +167,21 @@ var booleanOperatorScenarios = []expressionScenario{
document: `{a: true, b: false}`,
expression: `.[] or (false, true)`,
expected: []string{
"D0, P[a], (!!bool)::true\n",
"D0, P[a], (!!bool)::true\n",
"D0, P[b], (!!bool)::false\n",
"D0, P[b], (!!bool)::true\n",
},
},
{
skipDoc: true,
document: `{a: true, b: false}`,
expression: `.[] and (false, true)`,
expected: []string{
"D0, P[a], (!!bool)::false\n",
"D0, P[a], (!!bool)::true\n",
"D0, P[b], (!!bool)::false\n",
},
},
{
skipDoc: true,
document: `{}`,
Expand Down
55 changes: 39 additions & 16 deletions pkg/yqlib/operators.go
Original file line number Diff line number Diff line change
Expand Up @@ -54,10 +54,25 @@ func emptyOperator(d *dataTreeNavigator, context Context, expressionNode *Expres

type crossFunctionCalculation func(d *dataTreeNavigator, context Context, lhs *CandidateNode, rhs *CandidateNode) (*CandidateNode, error)

func resultsForRHS(d *dataTreeNavigator, context Context, lhsCandidate *CandidateNode, rhs Context, calculation crossFunctionCalculation, results *list.List, calcWhenEmpty bool) error {
func resultsForRHS(d *dataTreeNavigator, context Context, lhsCandidate *CandidateNode, prefs crossFunctionPreferences, rhsExp *ExpressionNode, results *list.List) error {

if calcWhenEmpty && rhs.MatchingNodes.Len() == 0 {
resultCandidate, err := calculation(d, context, lhsCandidate, nil)
if prefs.LhsResultValue != nil {
result, err := prefs.LhsResultValue(lhsCandidate)
if err != nil {
return err
} else if result != nil {
results.PushBack(result)
return nil
}
}

rhs, err := d.GetMatchingNodes(context, rhsExp)
if err != nil {
return err
}

if prefs.CalcWhenEmpty && rhs.MatchingNodes.Len() == 0 {
resultCandidate, err := prefs.Calculation(d, context, lhsCandidate, nil)
if err != nil {
return err
}
Expand All @@ -70,7 +85,7 @@ func resultsForRHS(d *dataTreeNavigator, context Context, lhsCandidate *Candidat
for rightEl := rhs.MatchingNodes.Front(); rightEl != nil; rightEl = rightEl.Next() {
log.Debugf("Applying calc")
rhsCandidate := rightEl.Value.(*CandidateNode)
resultCandidate, err := calculation(d, context, lhsCandidate, rhsCandidate)
resultCandidate, err := prefs.Calculation(d, context, lhsCandidate, rhsCandidate)
if err != nil {
return err
}
Expand All @@ -81,22 +96,24 @@ func resultsForRHS(d *dataTreeNavigator, context Context, lhsCandidate *Candidat
return nil
}

func doCrossFunc(d *dataTreeNavigator, context Context, expressionNode *ExpressionNode, calculation crossFunctionCalculation, calcWhenEmpty bool) (Context, error) {
type crossFunctionPreferences struct {
CalcWhenEmpty bool
// if this returns a result node,
// we wont bother calculating the RHS
LhsResultValue func(*CandidateNode) (*CandidateNode, error)
Calculation crossFunctionCalculation
}

func doCrossFunc(d *dataTreeNavigator, context Context, expressionNode *ExpressionNode, prefs crossFunctionPreferences) (Context, error) {
var results = list.New()
lhs, err := d.GetMatchingNodes(context, expressionNode.LHS)
if err != nil {
return Context{}, err
}
log.Debugf("crossFunction LHS len: %v", lhs.MatchingNodes.Len())

rhs, err := d.GetMatchingNodes(context, expressionNode.RHS)

if err != nil {
return Context{}, err
}

if calcWhenEmpty && lhs.MatchingNodes.Len() == 0 {
err := resultsForRHS(d, context, nil, rhs, calculation, results, calcWhenEmpty)
if prefs.CalcWhenEmpty && lhs.MatchingNodes.Len() == 0 {
err := resultsForRHS(d, context, nil, prefs, expressionNode.RHS, results)
if err != nil {
return Context{}, err
}
Expand All @@ -105,7 +122,7 @@ func doCrossFunc(d *dataTreeNavigator, context Context, expressionNode *Expressi
for el := lhs.MatchingNodes.Front(); el != nil; el = el.Next() {
lhsCandidate := el.Value.(*CandidateNode)

err := resultsForRHS(d, context, lhsCandidate, rhs, calculation, results, calcWhenEmpty)
err = resultsForRHS(d, context, lhsCandidate, prefs, expressionNode.RHS, results)
if err != nil {
return Context{}, err
}
Expand All @@ -115,6 +132,11 @@ func doCrossFunc(d *dataTreeNavigator, context Context, expressionNode *Expressi
}

func crossFunction(d *dataTreeNavigator, context Context, expressionNode *ExpressionNode, calculation crossFunctionCalculation, calcWhenEmpty bool) (Context, error) {
prefs := crossFunctionPreferences{CalcWhenEmpty: calcWhenEmpty, Calculation: calculation}
return crossFunctionWithPrefs(d, context, expressionNode, prefs)
}

func crossFunctionWithPrefs(d *dataTreeNavigator, context Context, expressionNode *ExpressionNode, prefs crossFunctionPreferences) (Context, error) {
var results = list.New()

var evaluateAllTogether = true
Expand All @@ -124,15 +146,16 @@ func crossFunction(d *dataTreeNavigator, context Context, expressionNode *Expres
break
}
}

if evaluateAllTogether {
log.Debug("crossFunction evaluateAllTogether!")
return doCrossFunc(d, context, expressionNode, calculation, calcWhenEmpty)
return doCrossFunc(d, context, expressionNode, prefs)
}

log.Debug("crossFunction evaluate apart!")

for matchEl := context.MatchingNodes.Front(); matchEl != nil; matchEl = matchEl.Next() {
innerResults, err := doCrossFunc(d, context.SingleChildContext(matchEl.Value.(*CandidateNode)), expressionNode, calculation, calcWhenEmpty)
innerResults, err := doCrossFunc(d, context.SingleChildContext(matchEl.Value.(*CandidateNode)), expressionNode, prefs)
if err != nil {
return Context{}, err
}
Expand Down

0 comments on commit 005b097

Please sign in to comment.