From 005b097cee4a486706456c22f8c7b44d618c1730 Mon Sep 17 00:00:00 2001 From: Mike Farah Date: Sun, 20 Mar 2022 12:55:58 +1100 Subject: [PATCH] Boolean fix (#1148) * Fixing booleans * Fixed "and", "or" evaluating RHS when not required --- examples/data1.yaml | 3 +- pkg/yqlib/operator_booleans.go | 88 ++++++++++++++++------------- pkg/yqlib/operator_booleans_test.go | 27 ++++++++- pkg/yqlib/operators.go | 55 ++++++++++++------ 4 files changed, 115 insertions(+), 58 deletions(-) diff --git a/examples/data1.yaml b/examples/data1.yaml index 1bf3a5cb63..5b4dbd134f 100644 --- a/examples/data1.yaml +++ b/examples/data1.yaml @@ -1 +1,2 @@ -cat: purrs +a: mike +b: [t, f] \ No newline at end of file diff --git a/pkg/yqlib/operator_booleans.go b/pkg/yqlib/operator_booleans.go index cbc558c965..d919b9a6b3 100644 --- a/pkg/yqlib/operator_booleans.go +++ b/pkg/yqlib/operator_booleans.go @@ -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 } } @@ -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) { diff --git a/pkg/yqlib/operator_booleans_test.go b/pkg/yqlib/operator_booleans_test.go index e9b72f993b..f5735bb29b 100644 --- a/pkg/yqlib/operator_booleans_test.go +++ b/pkg/yqlib/operator_booleans_test.go @@ -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`, @@ -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: `{}`, diff --git a/pkg/yqlib/operators.go b/pkg/yqlib/operators.go index 6ca5025478..1a70db6ab8 100644 --- a/pkg/yqlib/operators.go +++ b/pkg/yqlib/operators.go @@ -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 } @@ -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 } @@ -81,7 +96,15 @@ 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 { @@ -89,14 +112,8 @@ func doCrossFunc(d *dataTreeNavigator, context Context, expressionNode *Expressi } 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 } @@ -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 } @@ -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 @@ -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 }