TransformUp is now sensitive to tree modifications #867

max-hoffman · 2022-03-12T00:25:01Z

TransformUp and related node/expression DFS helper functions expect the
visit functions to return an additional boolean parameter indicating
whether the visit changed the node:

type TransformNodeFunc func(Node) (Node, bool, error)
type TransformExprFunc func(Expression) (Expression, bool, error)
type Transformer func(TransformContext) (sql.Node, bool, error)

TransformUp's implementation uses the modification information to avoid
re-creating a node with identical children:

BenchmarkTransformOld
BenchmarkTransformOld-12          	  396544	      2782 ns/op	    3000 B/op	      51 allocs/op
BenchmarkTransformOldNoEdit
BenchmarkTransformOldNoEdit-12    	  407797	      2731 ns/op	    2936 B/op	      50 allocs/op
BenchmarkTransformNew
BenchmarkTransformNew-12          	 4584258	       254.1 ns/op	      96 B/op	       5 allocs/op
BenchmarkTransformNewNoEdit
BenchmarkTransformNewNoEdit-12       	 4782098	       237.8 ns/op	      96 B/op	       5 allocs/op

We use plan.InspectUp when possible, and then plan.TransformUp
where possible, resorting to the more expensive plan.TransformUpCtx
and plan.TransformUpCtxSchema only when necessary.

zachmu

Love the performance results, happy you did this work.

How mad would you be if I made you rewrite this to return a sentinel error type instead of changing the return signature of Transform? I think that's a better pattern because a) fewer return params, b) makes it harder to mistakenly under-analyze because a buggy rule returned the wrong boolean. Over-analyzing means we take longer than we should, under-analyzing means we return incorrect results. Basically I think the signal "I made no changes" should be more explicit and harder to send by accident.

See related comments about helper methods being exported inappropriately.

zachmu · 2022-03-14T20:43:15Z

sql/plan/transform.go

-	e, ok := n.(sql.Expressioner)
-	if !ok {
-		return n, nil
+func TransformUpHelper(node sql.Node, f sql.TransformNodeFunc) (sql.Node, bool, error) {


Why exported?

zachmu · 2022-03-14T20:48:44Z

sql/analyzer/stored_procedures.go

-				if _, ok := paramNames[strings.ToLower(e.Name())]; ok {
-					return expression.NewProcedureParam(e.Name()), nil
+func resolveProcedureParamsTransform(ctx *sql.Context, paramNames map[string]struct{}, n sql.Node) (sql.Node, bool, error) {
+	return plan.TransformUpHelper(n, func(n sql.Node) (sql.Node, bool, error) {


Big red flag here

The question you should be asking is: why doesn't TransformExpressionsUpWithNode work here on its own?

zachmu · 2022-03-14T20:53:27Z

sql/analyzer/resolve_ctes.go

 	cur = n
-	for i := 0; i < maxCteDepth && !nodesEqual(prev, cur); i++ {
+	for i := 0; i < maxCteDepth && mod; i++ {


Not sure about this, it makes it possible for an analyzer rule to misbehave and claim it didn't change anything when it actually did.

On the other hand, not calling DeepEquals on every node is probably a huge performance savings.

I think I would be more comfortable getting rid of DeepEquals here if a rule had to opt-in to saying it changed nothing, rather than opt-in to saying something changed.

`TransformUp` and related node/expression DFS helper functions expect the visit function to return an additional parameter indicating whether the visit changed the node: `sql.SameTree` or `sql.NewTree`. We use `plan.InspectUp` when possible, and then `plan.TransformUp` where possible, resorting to the more expensive `plan.TransformUpCtx` and `plan.TransformUpCtxSchema` only when necessary.

zachmu · 2022-03-28T19:10:20Z

This getting close to ready for review?

Needs conflicts resolved

max-hoffman · 2022-03-28T19:12:20Z

@zachmu it's next on my list after prepared statements. I spent 4/5 days last week on index issues, this keeps getting deprioritized

…mysql-server into max/reduce-transform-mallocs

zachmu

This looks really good in general, a lot of comments but overall sound

sql/analyzer/aggregations.go

sql/analyzer/analyzer.go

zachmu · 2022-03-31T19:10:55Z

sql/analyzer/apply_hash_in.go

-			return nil, err
+			return nil, transform.SameTree, err
+		}
+		if same {


A lot of places you have this block of code could be simplified:

return node, same, err

No need to special-case same, or the err from WithExpressions or WithChildren, just return all of it

if we're always creating a new node, doesn't that defeat the purpose of minimizing allocs?

Yeah on second thought this is a dumb comment in the general case

There are a couple spots you might apply it, but not here. Don't call a With method if you don't have to.

zachmu · 2022-03-31T19:34:07Z

sql/analyzer/apply_indexes_from_outer_scope.go

 }

 // pushdownIndexToTable attempts to push the index given down to the table given, if it implements
 // sql.IndexAddressableTable
-func pushdownIndexToTable(a *Analyzer, tableNode NameableNode, index sql.Index, keyExpr []sql.Expression) (sql.Node, error) {
-	return plan.TransformUp(tableNode, func(n sql.Node) (sql.Node, error) {
+func pushdownIndexToTable(a *Analyzer, tableNode NameableNode, index sql.Index, keyExpr []sql.Expression) (sql.Node, transform.TreeIdentity, error) {


Why is this here instead of in pushdown.go?

the function is only referenced in this file. it's not filter pushdown, just applying an index

sql/analyzer/assign_info_schema.go

sql/transform/node.go

zachmu · 2022-04-01T16:10:23Z

Also I forgot to mention this last night, but this would make a really good blog post.

max-hoffman force-pushed the max/reduce-transform-mallocs branch from 81c52bd to 12d1204 Compare March 14, 2022 15:55

max-hoffman changed the title ~~Minimize TransformUp mallocs~~ TransformUp is now sensitive to tree modifications Mar 14, 2022

max-hoffman marked this pull request as ready for review March 14, 2022 15:59

max-hoffman requested a review from zachmu March 14, 2022 15:59

max-hoffman assigned zachmu Mar 14, 2022

zachmu reviewed Mar 14, 2022

View reviewed changes

max-hoffman force-pushed the max/reduce-transform-mallocs branch from a258490 to ceb8e26 Compare March 17, 2022 22:40

progress for nodesEqual refactor

8eaaad0

max-hoffman added 4 commits March 31, 2022 07:35

prog

3119cde

merge main

9e84074

renames

4d662e5

fixup names more

bd8b460

max-hoffman requested a review from zachmu March 31, 2022 16:59

max-hoffman and others added 4 commits March 31, 2022 10:15

Move transform related interfaces into transform package

efa705e

[ga-format-pr] Run ./format_repo.sh to fix formatting

856fc62

missing docstring

408bb69

Merge branch 'max/reduce-transform-mallocs' of github.com:dolthub/go-…

01ca96a

…mysql-server into max/reduce-transform-mallocs

zachmu approved these changes Mar 31, 2022

View reviewed changes

max-hoffman added 2 commits April 1, 2022 07:49

prog

3b9d6ba

merge main

8063875

Zach's comments

e10499f

max-hoffman merged commit 24320fe into main Apr 1, 2022

max-hoffman deleted the max/reduce-transform-mallocs branch April 1, 2022 20:53

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

TransformUp is now sensitive to tree modifications #867

TransformUp is now sensitive to tree modifications #867

max-hoffman commented Mar 12, 2022 •

edited

Loading

zachmu left a comment

zachmu Mar 14, 2022

zachmu Mar 14, 2022

zachmu Mar 14, 2022

zachmu commented Mar 28, 2022

max-hoffman commented Mar 28, 2022

zachmu left a comment

zachmu Mar 31, 2022

max-hoffman Apr 1, 2022 •

edited

Loading

zachmu Apr 1, 2022

zachmu Mar 31, 2022

max-hoffman Apr 1, 2022

zachmu commented Apr 1, 2022

TransformUp is now sensitive to tree modifications #867

TransformUp is now sensitive to tree modifications #867

Conversation

max-hoffman commented Mar 12, 2022 • edited Loading

zachmu left a comment

Choose a reason for hiding this comment

zachmu Mar 14, 2022

Choose a reason for hiding this comment

zachmu Mar 14, 2022

Choose a reason for hiding this comment

zachmu Mar 14, 2022

Choose a reason for hiding this comment

zachmu commented Mar 28, 2022

max-hoffman commented Mar 28, 2022

zachmu left a comment

Choose a reason for hiding this comment

zachmu Mar 31, 2022

Choose a reason for hiding this comment

max-hoffman Apr 1, 2022 • edited Loading

Choose a reason for hiding this comment

zachmu Apr 1, 2022

Choose a reason for hiding this comment

zachmu Mar 31, 2022

Choose a reason for hiding this comment

max-hoffman Apr 1, 2022

Choose a reason for hiding this comment

zachmu commented Apr 1, 2022

max-hoffman commented Mar 12, 2022 •

edited

Loading

max-hoffman Apr 1, 2022 •

edited

Loading