From 45132437406e9b1f33772f8a2fe00bce13df931c Mon Sep 17 00:00:00 2001 From: Mike Farah Date: Sun, 20 Mar 2022 12:53:35 +1100 Subject: [PATCH] Fixed "and", "or" evaluating RHS when not required --- pkg/yqlib/operator_booleans.go | 59 +++++++++++++---------------- pkg/yqlib/operator_booleans_test.go | 19 +++++++--- pkg/yqlib/operators.go | 55 +++++++++++++++++++-------- 3 files changed, 80 insertions(+), 53 deletions(-) diff --git a/pkg/yqlib/operator_booleans.go b/pkg/yqlib/operator_booleans.go index 4307ce37..d919b9a6 100644 --- a/pkg/yqlib/operator_booleans.go +++ b/pkg/yqlib/operator_booleans.go @@ -27,8 +27,6 @@ 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) @@ -48,42 +46,30 @@ func getOwner(lhs *CandidateNode, rhs *CandidateNode) *CandidateNode { return owner } -func andOp(d *dataTreeNavigator, context Context, lhs *CandidateNode, rhs *CandidateNode) (*CandidateNode, error) { +func returnRhsTruthy(d *dataTreeNavigator, context Context, lhs *CandidateNode, rhs *CandidateNode) (*CandidateNode, error) { owner := getOwner(lhs, rhs) - var err error - lhsBool := true - if lhsBool, err = getBoolean(lhs); err != nil { - return nil, err - } - if lhsBool == false { - return createBooleanCandidate(owner, false), nil - } - - rhsBool := true - if rhsBool, err = getBoolean(rhs); err != nil { + rhsBool, err := getBoolean(rhs) + if err != nil { return nil, err } return createBooleanCandidate(owner, rhsBool), nil } -func orOp(d *dataTreeNavigator, context Context, lhs *CandidateNode, rhs *CandidateNode) (*CandidateNode, error) { - owner := getOwner(lhs, rhs) - var err error - lhsBool := true - if lhsBool, err = getBoolean(lhs); err != nil { - return nil, err - } - if lhsBool == true { - return createBooleanCandidate(owner, true), nil - } +func returnLHSWhen(targetBool bool) func(lhs *CandidateNode) (*CandidateNode, error) { + return func(lhs *CandidateNode) (*CandidateNode, error) { + var err error + var lhsBool bool - rhsBool := true - if rhsBool, err = getBoolean(rhs); err != nil { - return nil, err + if lhsBool, err = getBoolean(lhs); err != nil || lhsBool != targetBool { + return nil, err + } + owner := &CandidateNode{} + if lhs != nil { + owner = lhs + } + return createBooleanCandidate(owner, targetBool), nil } - - return createBooleanCandidate(owner, rhsBool), nil } func findBoolean(wantBool bool, d *dataTreeNavigator, context Context, expressionNode *ExpressionNode, sequenceNode *yaml.Node) (bool, error) { @@ -154,12 +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, orOp, 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) { - + 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 ad15545a..f5735bb2 100644 --- a/pkg/yqlib/operator_booleans_test.go +++ b/pkg/yqlib/operator_booleans_test.go @@ -47,17 +47,17 @@ var booleanOperatorScenarios = []expressionScenario{ { skipDoc: true, description: "And should not run 2nd arg if first is false", - expression: `select(false and test(3))`, + expression: `false and test(3)`, expected: []string{ - "D0, P[], (doc)::b: hi\n", + "D0, P[], (!!bool)::false\n", }, }, { skipDoc: true, description: "Or should not run 2nd arg if first is true", - expression: `select(true or test(3))`, + expression: `true or test(3)`, expected: []string{ - "D0, P[], (doc)::b: hi\n", + "D0, P[], (!!bool)::true\n", }, }, { @@ -167,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 6ca50254..1a70db6a 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 }