-
Notifications
You must be signed in to change notification settings - Fork 9
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
AST: Allow children of commutative nodes to be reordered cost-wise. #800
Changes from all commits
6a9d98d
93cbec4
133da12
043934e
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,26 @@ | ||
package ast | ||
|
||
import ( | ||
"testing" | ||
|
||
"github.com/stretchr/testify/assert" | ||
) | ||
|
||
func TestNodeWeights(t *testing.T) { | ||
tts := []struct { | ||
n Node | ||
c int | ||
}{ | ||
{Node{Function: FUNC_AND, Children: []Node{{Function: FUNC_DB_ACCESS}, {Function: FUNC_PAYLOAD}}}, 60}, | ||
{Node{Function: FUNC_AND, Children: []Node{{Function: FUNC_DB_ACCESS}, { | ||
Function: FUNC_ADD, Children: []Node{{ | ||
Function: FUNC_AGGREGATOR, | ||
Children: []Node{{Function: FUNC_CUSTOM_LIST_ACCESS}, {Function: FUNC_PAYLOAD}}, | ||
}}, | ||
}}}, 140}, | ||
} | ||
|
||
for _, tt := range tts { | ||
assert.Equal(t, tt.c, tt.n.Cost()) | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -10,8 +10,9 @@ import ( | |
) | ||
|
||
type AstEvaluationEnvironment struct { | ||
availableFunctions map[ast.Function]evaluate.Evaluator | ||
disableCircuitBreaking bool | ||
availableFunctions map[ast.Function]evaluate.Evaluator | ||
disableCostOptimizations bool | ||
disableCircuitBreaking bool | ||
} | ||
|
||
func (environment *AstEvaluationEnvironment) AddEvaluator(function ast.Function, evaluator evaluate.Evaluator) { | ||
|
@@ -28,12 +29,25 @@ func (environment *AstEvaluationEnvironment) GetEvaluator(function ast.Function) | |
return nil, errors.New(fmt.Sprintf("function '%s' is not available", function.DebugString())) | ||
} | ||
|
||
func (environment AstEvaluationEnvironment) WithoutOptimizations() AstEvaluationEnvironment { | ||
environment.disableCostOptimizations = true | ||
environment.disableCircuitBreaking = true | ||
|
||
return environment | ||
} | ||
|
||
func (environment AstEvaluationEnvironment) WithoutCircuitBreaking() AstEvaluationEnvironment { | ||
environment.disableCircuitBreaking = true | ||
|
||
return environment | ||
} | ||
|
||
func (environment AstEvaluationEnvironment) WithoutCostOptimizations() AstEvaluationEnvironment { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Just checking my understanding here: this is necessary so we don't have to reorder error results that we return to the frontend, correct? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Now that I think about it, was your comment on the right line? If it was about the whole re-ordering / de-re-ordering instead, yes, it is done to ensure the tree results are sent in the order the frontend expects it to be, since it relies on that specific structure to match results with their node. That would not be necessary if every node had a unique ID, but since that is not the case, we need this. |
||
environment.disableCostOptimizations = true | ||
|
||
return environment | ||
} | ||
|
||
func NewAstEvaluationEnvironment() AstEvaluationEnvironment { | ||
environment := AstEvaluationEnvironment{ | ||
availableFunctions: make(map[ast.Function]evaluate.Evaluator), | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,73 @@ | ||
package ast_eval | ||
|
||
import ( | ||
"cmp" | ||
"slices" | ||
|
||
"github.com/checkmarble/marble-backend/models/ast" | ||
) | ||
|
||
// Weighted nodes manages a flat list of nodes and offers an interface to process | ||
// them sorted by node cost. A lower-cost node will be executed earlier when the | ||
// parent is commutative. | ||
// | ||
// The parent is passed to the constructor, so that if it is not commutative, this | ||
// is basically a no-op. | ||
type WeightedNodes struct { | ||
enabled bool | ||
original []ast.Node | ||
} | ||
|
||
func NewWeightedNodes(env AstEvaluationEnvironment, parent ast.Node, nodes []ast.Node) WeightedNodes { | ||
enabled := false | ||
|
||
if !env.disableCostOptimizations { | ||
if fattrs, err := parent.Function.Attributes(); err == nil { | ||
enabled = fattrs.Commutative | ||
} | ||
} | ||
|
||
if enabled { | ||
for idx := range nodes { | ||
nodes[idx].Index = idx | ||
} | ||
} | ||
|
||
return WeightedNodes{ | ||
enabled: enabled, | ||
original: nodes, | ||
} | ||
} | ||
|
||
func (wn WeightedNodes) Sorted() []ast.Node { | ||
if !wn.enabled { | ||
return wn.original | ||
} | ||
|
||
return slices.SortedFunc(slices.Values(wn.original), func(lhs, rhs ast.Node) int { | ||
return cmp.Compare(lhs.Cost(), rhs.Cost()) | ||
}) | ||
} | ||
|
||
func (wn WeightedNodes) Reorder(results []ast.NodeEvaluation) []ast.NodeEvaluation { | ||
if !wn.enabled { | ||
return results | ||
} | ||
|
||
output := make([]ast.NodeEvaluation, len(wn.original)) | ||
|
||
for idx := range wn.original { | ||
output[idx] = ast.NodeEvaluation{ | ||
Index: idx, | ||
Skipped: true, | ||
ReturnValue: nil, | ||
} | ||
} | ||
|
||
for _, result := range results { | ||
output[result.Index] = result | ||
output[result.Index].Skipped = false | ||
} | ||
|
||
return output | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also gotta do the same on name children (for keeping generality at least, not sure if the case can be seen currently)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Aren't named children already order-agnostic? They're both stored in a
map
, which does not have a reproducible order, and I assume are used out-of-order?Will look into it tomorrow.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes they are, however for cost computation the named child nodes should also have their cost taken into account