opt: Add optimizer support for new Insert operator #32423

andy-kimball · 2018-11-16T18:42:39Z

Add Insert operator to the optimizer. The implementation is side-by-side
with the existing heuristic planner version. The main differences are that it
supports correlated subqueries, integrates with the CBO, and compiles
default and computed properties differently. Rather than keeping these
properties separate, the new Insert operator wraps the input expression
with Project operators that synthesize the default and computed
expressions.

cockroach-teamcity · 2018-11-16T18:42:52Z

This change is

knz · 2018-11-16T18:46:38Z

niiice 🎉

justinj

Looked at a bunch of this, need to let a bunch of it sit and will come back for another look later.

Reviewed 22 of 22 files at r1, 3 of 3 files at r2, 7 of 7 files at r3, 3 of 3 files at r4, 31 of 31 files at r5, 3 of 3 files at r6.
Reviewable status: complete! 0 of 0 LGTMs obtained

pkg/sql/logictest/testdata/logic_test/builtin_function, line 1990 at r1 (raw file):

UTF8  NULL

# TODO(SQL execution team): Restore this to original form by removing FROM

nit: I believe the intention of the names is generally like, "who is the person you can ask about this", which I guess in this case would either be you or Jordan.

pkg/sql/opt/memo/testdata/logprops/join, line 91 at r1 (raw file):

 │    │    ├── fd: ()-->(5,6)
 │    │    ├── prune: (6)
 │    │    ├── max1-row

nit/unrelated: I wonder if Max1Row should error if its input is provably >1 row? Perhaps that case is limited enough that it's not worth it.

pkg/sql/opt/norm/custom_funcs.go, line 556 at r1 (raw file):

// Project operator can be merged with the columns of a Values operator to form
// a single combined Values operator. This is only possible when there is a
// single Values row and when the projections are not dependent on columns from

Why must there be a single Values row? I think having an explanation of why these restrictions exist would be helpful.

pkg/sql/opt/norm/rules/project.opt, line 38 at r1 (raw file):

[MergeProjectWithValues, Normalize]
(Project
    $input:(Values)

Could this be something like (Values [ * ]) and then CanMergeProjectWithValues doesn't have to check there's a single row?

pkg/sql/opt/optbuilder/insert.go, line 495 at r5 (raw file):

	// explicitly specified:
	//
	//   INSERT INTO <table> (...) VALUES (...))

nit: extra )

pkg/sql/opt/optbuilder/insert.go, line 552 at r5 (raw file):

	ib.addSynthesizedCols(func(tabCol opt.Column) bool { return !tabCol.IsComputed() })

	// Add any missing computed columns.

I think a comment might be helpful to clarify that this ordering is enforced because computed columns can depend on default columns (but not the other way around).

pkg/sql/opt/optbuilder/insert.go, line 593 at r5 (raw file):

		expr := ib.parseDefaultOrComputedExpr(tabColID)
		texpr := ib.outScope.resolveType(expr, tabCol.DatumType())
		scopeCol := ib.b.addColumn(projectionsScope, "" /* label */, texpr)

is there any reason not to set the label to tabCol.ColName()?

pkg/sql/opt/optbuilder/insert.go, line 672 at r5 (raw file):

		more, less := "expressions", "target columns"
		if actual < expected {
			more, less = less, more

I like this, haha

andy-kimball

Reviewable status: complete! 0 of 0 LGTMs obtained

pkg/sql/opt/memo/testdata/logprops/join, line 91 at r1 (raw file):