Skip to content
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

sql: implement ConstructZigzagJoin in distsql_spec_exec_factory #72653

Merged
merged 1 commit into from
Nov 18, 2021

Conversation

cucaroach
Copy link
Contributor

@cucaroach cucaroach commented Nov 11, 2021

Informs #47473

Release note: None

@cucaroach cucaroach requested a review from a team as a code owner November 11, 2021 17:58
@cockroach-teamcity
Copy link
Member

This change is Reviewable

@cucaroach cucaroach requested review from yuzefovich and a team and removed request for a team November 11, 2021 17:58
Copy link
Member

@yuzefovich yuzefovich left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: link to #47473 in the PR description.

Reviewed 1 of 2 files at r1, 1 of 1 files at r2, all commit messages.
Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @cucaroach)


pkg/sql/distsql_spec_exec_factory.go, line 708 at r2 (raw file):

) (exec.Node, error) {

	// RFC: guessing it doesn't make sense to distribute zigzag...

Looks good to me, I'd just leave a comment saying that we currently choose to not distribute zigzag joins.


pkg/sql/distsql_spec_exec_factory.go, line 724 at r2 (raw file):

}

func (e *distSQLSpecExecFactory) constructZigzagJoin(

Currently there is a lot of duplication between constructZigzagJoin and DistSQLPlanner.createPlanForZigzagJoin. I'd encourage you to introduce zigzagPlanningInfo or something that would encompass all information needed to create a zigzag joiner spec and pass that as an argument to DistSQLPlanner.planZigzagJoin. This will allow for two factories to have different instantiation of zigzagPlanningInfo, yet most of the code will be in a single place (planZigzagJoin).


pkg/sql/distsql_spec_exec_factory.go, line 833 at r2 (raw file):

	}}

	// RFC: why isn't reqOrdering used?  Should it be used here?

Yeah, it should be, for the cleanliness purposes, but it would not change the behavior - since we do not distribute the zigzag joins, we have only a single zigzag join processor, so it automatically maintains the req ordering if necessary. (This req ordering is only used when merging multiple streams into one in the input synchronizer.) If you go into AddNoInputStage and then into SetMergeOrdering you'll see that we use an empty req ordering if there is a single processor in the stage (i.e. that stage is not distributed). Still, it'd be cleaner to use the proper reqOrdering here.


pkg/sql/opt/exec/execbuilder/testdata/zigzag_join, line 1 at r2 (raw file):

# LogicTest: local local-spec-planning

I'd also add a query that uses a zigzag join into experimental_distsql_planning file (where we have always mode).

Copy link
Contributor Author

@cucaroach cucaroach left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @cucaroach and @yuzefovich)


pkg/sql/distsql_spec_exec_factory.go, line 724 at r2 (raw file):

Previously, yuzefovich (Yahor Yuzefovich) wrote…

Currently there is a lot of duplication between constructZigzagJoin and DistSQLPlanner.createPlanForZigzagJoin. I'd encourage you to introduce zigzagPlanningInfo or something that would encompass all information needed to create a zigzag joiner spec and pass that as an argument to DistSQLPlanner.planZigzagJoin. This will allow for two factories to have different instantiation of zigzagPlanningInfo, yet most of the code will be in a single place (planZigzagJoin).

I thought we discussed this and decided copying would be better since we're gonna throw away the old code and we don't want
to introduce bugs into the old code with the refactoring. That said I agree it would be cleaner and if we make improvements to to the zigzag it would be nice not to have to do it twice. If we want to do it this way why not just have dist_spec_exec_factory just turn the factory args into a zigzagJoinNode and call createPlanForZigzagJoin? Seems like that would be the most straight forward way to do it with least amount of risk of breaking old code.


pkg/sql/distsql_spec_exec_factory.go, line 833 at r2 (raw file):

Previously, yuzefovich (Yahor Yuzefovich) wrote…

Yeah, it should be, for the cleanliness purposes, but it would not change the behavior - since we do not distribute the zigzag joins, we have only a single zigzag join processor, so it automatically maintains the req ordering if necessary. (This req ordering is only used when merging multiple streams into one in the input synchronizer.) If you go into AddNoInputStage and then into SetMergeOrdering you'll see that we use an empty req ordering if there is a single processor in the stage (i.e. that stage is not distributed). Still, it'd be cleaner to use the proper reqOrdering here.

👍


pkg/sql/opt/exec/execbuilder/testdata/zigzag_join, line 1 at r2 (raw file):

Previously, yuzefovich (Yahor Yuzefovich) wrote…

I'd also add a query that uses a zigzag join into experimental_distsql_planning file (where we have always mode).

Ha! I did that but removed it because I thought it was overkill. I confirmed in a debugger the logic test used the new factory but I agree it would be nice to have some tests under always mode. I guess duplicating tests into the experiment_distsql_planning for now is fine since we'll blow it away eventually.

Copy link
Contributor Author

@cucaroach cucaroach left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @cucaroach and @yuzefovich)


pkg/sql/distsql_spec_exec_factory.go, line 724 at r2 (raw file):

Previously, cucaroach (Tommy Reilly) wrote…

I thought we discussed this and decided copying would be better since we're gonna throw away the old code and we don't want
to introduce bugs into the old code with the refactoring. That said I agree it would be cleaner and if we make improvements to to the zigzag it would be nice not to have to do it twice. If we want to do it this way why not just have dist_spec_exec_factory just turn the factory args into a zigzagJoinNode and call createPlanForZigzagJoin? Seems like that would be the most straight forward way to do it with least amount of risk of breaking old code.

Going further zigzag join is probably so simple why don't we just do this:

func (e *distSQLSpecExecFactory) ConstructZigzagJoin(...) (exec.Node, error) {
  zigzagNode := constructZigzagNode(...factory args...)
  return e.dsp.createPlanForZigzagJoin(ctx, zigzagNode,...)
}

Essentially I'm arguing since zigzag isn't very "disty" it should just use the old code because we aren't gaining anything by doing it differently.

Copy link
Contributor Author

@cucaroach cucaroach left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @cucaroach and @yuzefovich)


pkg/sql/distsql_spec_exec_factory.go, line 724 at r2 (raw file):

Previously, cucaroach (Tommy Reilly) wrote…

Going further zigzag join is probably so simple why don't we just do this:

func (e *distSQLSpecExecFactory) ConstructZigzagJoin(...) (exec.Node, error) {
  zigzagNode := constructZigzagNode(...factory args...)
  return e.dsp.createPlanForZigzagJoin(ctx, zigzagNode,...)
}

Essentially I'm arguing since zigzag isn't very "disty" it should just use the old code because we aren't gaining anything by doing it differently.

Hmm not so sure about that now, the old way used scanNodes and a bunch of other intermediate allocations/structures and the new way eliminates that overhead. Let me look at it some more.

Copy link
Member

@yuzefovich yuzefovich left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @cucaroach)


pkg/sql/distsql_spec_exec_factory.go, line 724 at r2 (raw file):

I thought we discussed this and decided copying would be better since we're gonna throw away the old code and we don't want to introduce bugs into the old code with the refactoring.

Sorry, I might have been unclear. I think we should aim to not change the behavior (i.e. preserve the current physical planning heuristics as much as possible, which in this case means not distributing the zigzag joins); however, I'm not a fan of the idea of copying a bunch of essentially duplicated code. So what I'm suggesting is to do something similar to d55f5eb.

Creating zigzagPlanNode would defeat the purpose of the spec work - we want to eliminate creation of planNodes during the execbuild. Instead, we could create a "planning info" struct that allows us to populate ZigzagJoinerSpec in DistSQLPlanner.planZigzagJoin, and then both old and new factories would fill up that "planning info" struct in a different manner, but once that info is set, we have only a single code path in DistSQLPlanner.


pkg/sql/opt/exec/execbuilder/testdata/zigzag_join, line 1 at r2 (raw file):

Previously, cucaroach (Tommy Reilly) wrote…

Ha! I did that but removed it because I thought it was overkill. I confirmed in a debugger the logic test used the new factory but I agree it would be nice to have some tests under always mode. I guess duplicating tests into the experiment_distsql_planning for now is fine since we'll blow it away eventually.

Yeah, that's my thinking too.

@cucaroach cucaroach force-pushed the zz-direct-plan branch 2 times, most recently from ba65955 to 2b72a16 Compare November 12, 2021 20:34
Copy link
Contributor Author

@cucaroach cucaroach left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @cucaroach and @yuzefovich)


pkg/sql/distsql_physical_planner.go, line 2685 at r4 (raw file):

	}}

	plan.AddNoInputStage(corePlacement, post, pi.outputTypes, execinfrapb.Ordering{})

Should I use convertOrdering to turn the sql.ReqOrdering into a execinfrapb.Ordering here?


pkg/sql/distsql_spec_exec_factory.go, line 789 at r4 (raw file):

		return nil, err
	}
	p.ResultColumns = colinfo.ResultColumnsFromColumns(tabDesc.GetID(), catCols)

I'm not sure what's going on with ResultColumns, apparently only new spec factory needs it? I discovered this when trying to run a query which fed a zigzag into a lookup join, so this isn't super well tested (because lookup join isn't implemented).

Copy link
Member

@yuzefovich yuzefovich left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 2 of 2 files at r4, 1 of 1 files at r5, all commit messages.
Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @cucaroach)


pkg/sql/distsql_physical_planner.go, line 2685 at r4 (raw file):

Previously, cucaroach (Tommy Reilly) wrote…

Should I use convertOrdering to turn the sql.ReqOrdering into a execinfrapb.Ordering here?

Yeah, I'd do it in a separate commit though.


pkg/sql/distsql_spec_exec_factory.go, line 724 at r2 (raw file):

Previously, yuzefovich (Yahor Yuzefovich) wrote…

I thought we discussed this and decided copying would be better since we're gonna throw away the old code and we don't want to introduce bugs into the old code with the refactoring.

Sorry, I might have been unclear. I think we should aim to not change the behavior (i.e. preserve the current physical planning heuristics as much as possible, which in this case means not distributing the zigzag joins); however, I'm not a fan of the idea of copying a bunch of essentially duplicated code. So what I'm suggesting is to do something similar to d55f5eb.

Creating zigzagPlanNode would defeat the purpose of the spec work - we want to eliminate creation of planNodes during the execbuild. Instead, we could create a "planning info" struct that allows us to populate ZigzagJoinerSpec in DistSQLPlanner.planZigzagJoin, and then both old and new factories would fill up that "planning info" struct in a different manner, but once that info is set, we have only a single code path in DistSQLPlanner.

I still think there is more code that could be de-duplicated about the physical planning.

In particular, I'm thinking of the physical planning via the old factory path as being performed in two steps:

  1. create planNodes so that the DistSQLPlanner has enough info to perform the physical planning ("execbuilding"). This is done in the old factory.
  2. perform actual physical planning. This is done in DistSQLPlanner.createPlanForZigzagJoin. (There is a bit of nuance that createPlanForZigzagJoin also contains some logic that performs the adaptation from planNodes to specs.)

I think we should not modify the physical planning logic in distsql_physical_planner as much as possible while we're performing this transition to the new factory (the goal is to preserve the existing behavior as much as possible). However, both the new and the old paths have different ways of setting up the necessary information for the physical planner. It requires a bit of untangling in createPlanForZigzagJoin to split that out.

With the current change the physical planning logic is being introduced into the new factory, and now we have it duplicated. This is not ideal, and apologies for being annoying, but we could de-duplicate things further. If it's unclear what I'm driving at, I prototyped my thinking in yuzefovich@97e410f.


pkg/sql/distsql_spec_exec_factory.go, line 789 at r4 (raw file):

Previously, cucaroach (Tommy Reilly) wrote…

I'm not sure what's going on with ResultColumns, apparently only new spec factory needs it? I discovered this when trying to run a query which fed a zigzag into a lookup join, so this isn't super well tested (because lookup join isn't implemented).

In the old factory, these result columns are taken off zigzagJoinNode (via planColumns method), but in the new factory we set it directly on the physical plan (because we don't have those planNodes to propagate this info).


pkg/sql/distsql_spec_exec_factory.go, line 716 at r5 (raw file):

	for s, table := range tables {
		// RFC: does casting a cat.Table to an optTable ruin the ability to test this code from opttester?  opt_exec_factory

I dunno about this.


pkg/sql/distsql_spec_exec_factory.go, line 762 at r5 (raw file):

		for c, ok := col.Next(0); ok; c, ok = col.Next(c + 1) {
			column := tdesc.PublicColumns()[c]
			// RFC: what's going on with visibility?  can't find where its set I think its just

Yeah, I agree, it does look like Go default is used here.

Copy link
Contributor Author

@cucaroach cucaroach left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @cucaroach and @yuzefovich)


pkg/sql/distsql_spec_exec_factory.go, line 738 at r6 (raw file):

) (exec.Node, error) {
	planCtx := e.getPlanCtx(cannotDistribute)

Should I check onCond and the fixed value expressions against checkExpr or does that not matter because I specified cannotDistribute?

Copy link
Contributor Author

@cucaroach cucaroach left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @cucaroach and @yuzefovich)


pkg/sql/distsql_spec_exec_factory.go, line 694 at r6 (raw file):

) (zigzagPlanningSide, error) {
	desc := table.(*optTable).desc
	colCfg := scanColumnsConfig{wantedColumns: make([]tree.ColumnID, 0, wantedCols.Len())}

Should this be using makeScanColumnsConfig?

Copy link
Member

@yuzefovich yuzefovich left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:lgtm:

Reviewed 4 of 4 files at r6, all commit messages.
Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained (waiting on @cucaroach)


pkg/sql/distsql_spec_exec_factory.go, line 694 at r6 (raw file):

Previously, cucaroach (Tommy Reilly) wrote…

Should this be using makeScanColumnsConfig?

I think no given that the old factory is creating scanColumnsConfig directly.

Maybe it's worth extracting a helper method to initialize this struct when it is needed for the zigzag joins, and then use that method in both old and new factories.


pkg/sql/distsql_spec_exec_factory.go, line 714 at r6 (raw file):

		return zigzagPlanningSide{}, err
	}
	return zigzagPlanningSide{

In the old factory we also are keeping track of the indexUsageStats. I think we don't - yet - do that in the new factory, but maybe add a TODO here for that.


pkg/sql/distsql_spec_exec_factory.go, line 738 at r6 (raw file):

Previously, cucaroach (Tommy Reilly) wrote…

Should I check onCond and the fixed value expressions against checkExpr or does that not matter because I specified cannotDistribute?

Yeah, I think it doesn't matter because of cannotDistribute. Maybe add a quick comment about that.

@cucaroach
Copy link
Contributor Author

bors r+

Copy link
Contributor Author

@cucaroach cucaroach left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @cucaroach and @yuzefovich)


pkg/sql/distsql_spec_exec_factory.go, line 694 at r6 (raw file):

Previously, yuzefovich (Yahor Yuzefovich) wrote…

I think no given that the old factory is creating scanColumnsConfig directly.

Maybe it's worth extracting a helper method to initialize this struct when it is needed for the zigzag joins, and then use that method in both old and new factories.

I'll leave it like this and extract a helper when i need it..

@craig craig bot merged commit 4c8c0d4 into cockroachdb:master Nov 18, 2021
@craig
Copy link
Contributor

craig bot commented Nov 18, 2021

Build succeeded:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants