sql: clean up of scanNode and some other things #49724

yuzefovich · 2020-05-29T23:41:18Z

sql: unify PlanningCtx constructors into one

Release note: None

sql: remove separate scanVisilibity struct

This commit removes sql.scanVisibility in favor of protobuf-generated
execinfrapb.ScanVisibility. It also introduces prettier aliases for
the two values into execinfra package that are now used throughout the
code.

Release note: None

sql: clean up of scan node and a few other things

This commit does the following cleanups of scanNode:

refactors scanNode.initCols method to be standalone (it will
probably be reused later by distsql spec exec factory).
removes numBackfillColumns, specifiedIndexReverse, and
isSecondaryIndex fields since they are no longer used.
refactors the code to remove valNeededForCols field which was
always consecutive numbers in range [0, len(n.cols)-1].
refactors getIndexIdx method to not depend on scanNode.

Additionally, this commit removes planDependencies business
from planTop since optimizer now handles CREATE VIEW and handles
the plan dependencies on its own (and CREATE VIEW was the single
user of that struct in the plan top).

Also, it removes (which seems like) unnecessary call to planColumns
when creating distinct spec and an unused parameter from
createTableReaders method.

Addresses: #47474.

Release note: None

cockroach-teamcity · 2020-05-29T23:41:27Z

This change is

asubiotto

Reviewed 15 of 15 files at r1, 15 of 15 files at r2, 7 of 7 files at r3.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @yuzefovich)

pkg/sql/apply_join.go, line 264 at r1 (raw file):

	planCtx := params.p.extendedEvalCtx.ExecCfg.DistSQLPlanner.newLocalPlanningCtx(params.ctx, evalCtx)
	// Always plan local.
	planCtx.isLocal = true

I don't think newLocalPlanningCtx sets this field. Maybe it should?

pkg/sql/conn_executor_exec.go, line 866 at r1 (raw file):

	var planCtx *PlanningCtx
	if distribute {
		planCtx = ex.server.cfg.DistSQLPlanner.NewPlanningCtx(ctx, evalCtx, planner.txn, distribute)

NewPlanningCtx calls newLocalPlanningCtx. We should be able to remove this if distribute check here and everywhere and have NewPlanningCtx behave identically to the old behavior

pkg/sql/distsql_physical_planner.go, line 3253 at r1 (raw file):

	ctx context.Context, evalCtx *extendedEvalContext, txn *kv.Txn, distribute bool,
) *PlanningCtx {
	planCtx := dsp.newLocalPlanningCtx(ctx, evalCtx)

How about if !distribute { return?

pkg/sql/distsql_running_test.go, line 158 at r1 (raw file):

		evalCtx := p.ExtendedEvalContext()
		// We need distribute = true so that we executing the plan involves marshaling

nit: so that we executing the plan

pkg/sql/opt_exec_factory.go, line 2098 at r2 (raw file):

// identified by their ordinal position in the table schema.
func makeScanColumnsConfig(table cat.Table, cols exec.TableColumnOrdinalSet) scanColumnsConfig {
	// Set visibility=publicAndNotPublicColumns, since all columns in the "cols"

nit: update comment further?

pkg/sql/scan.go, line 362 at r2 (raw file):

	// Register the dependency to the planner, if requested.
	if planDeps != nil {

cc @RaduBerinde

RaduBerinde · 2020-06-01T14:07:52Z

pkg/sql/scan.go, line 362 at r2 (raw file):