-
Notifications
You must be signed in to change notification settings - Fork 3.8k
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
Conversation
6fae896
to
ed2eea9
Compare
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.
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: 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).
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.
Reviewable status: 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
andDistSQLPlanner.createPlanForZigzagJoin
. I'd encourage you to introducezigzagPlanningInfo
or something that would encompass all information needed to create a zigzag joiner spec and pass that as an argument toDistSQLPlanner.planZigzagJoin
. This will allow for two factories to have different instantiation ofzigzagPlanningInfo
, 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 intoSetMergeOrdering
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 properreqOrdering
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 havealways
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.
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.
Reviewable status: 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.
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.
Reviewable status: 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.
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.
Reviewable status: 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.
ba65955
to
2b72a16
Compare
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.
Reviewable status: 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).
2b72a16
to
721c381
Compare
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.
Reviewed 2 of 2 files at r4, 1 of 1 files at r5, all commit messages.
Reviewable status: 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 populateZigzagJoinerSpec
inDistSQLPlanner.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 inDistSQLPlanner
.
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:
- create planNodes so that the
DistSQLPlanner
has enough info to perform the physical planning ("execbuilding"). This is done in the old factory. - perform actual physical planning. This is done in
DistSQLPlanner.createPlanForZigzagJoin
. (There is a bit of nuance thatcreatePlanForZigzagJoin
also contains some logic that performs the adaptation fromplanNode
s 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 planNode
s 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.
721c381
to
9319e2f
Compare
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.
Reviewable status: 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?
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.
Reviewable status: 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?
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.
Reviewed 4 of 4 files at r6, all commit messages.
Reviewable status: 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.
Informs cockroachdb#47473 Release note: None
9319e2f
to
a4af072
Compare
bors r+ |
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.
Reviewable status: 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..
Build succeeded: |
Informs #47473
Release note: None