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

execinfra: explicitly pass the output RowReceiver to Processor.Run #98651

Merged
merged 2 commits into from
Mar 15, 2023

Conversation

yuzefovich
Copy link
Member

@yuzefovich yuzefovich commented Mar 15, 2023

colflow: simplify the vectorized flow creator a bit

This commit refactors the vectorized flow creator to directly embed two
structs that implement interfaces used by the creator. This allows for
those structs to not be allocated on the main code path. Also,
flowCreatorHelper no longer implements the Releasable interface.

Release note: None

execinfra: explicitly pass the output RowReceiver to Processor.Run

This commit modifies how the output of a processor is plumbed through.
Previously, we would pass it on the processor construction and store in
the ProcessorBaseNoHelper to later be used when calling
execinfra.Run method. However, the upcoming work on multiple active
portals will require updating the output when resuming the flow, so this
commit makes it so that we now pass the output explicitly into
execinfra.Processor.Run method. This will make the work on multiple
active portals cleaner, but also this change seems like a good one
independent of that because we're able to store all outputs in the
single place (FlowBase.outputs).

Epic: CRDB-17622

Release note: None

@cockroach-teamcity
Copy link
Member

This change is Reviewable

@yuzefovich yuzefovich force-pushed the run-output branch 4 times, most recently from 482bcd2 to 8251d20 Compare March 15, 2023 05:24
@yuzefovich yuzefovich changed the title WIP execinfra: explicitly pass the output to Processor.Run execinfra: explicitly pass the output RowReceiver to Processor.Run Mar 15, 2023
@yuzefovich yuzefovich marked this pull request as ready for review March 15, 2023 05:25
@yuzefovich yuzefovich requested review from a team as code owners March 15, 2023 05:25
@yuzefovich yuzefovich requested a review from a team March 15, 2023 05:25
@yuzefovich yuzefovich requested a review from a team as a code owner March 15, 2023 05:25
@yuzefovich yuzefovich requested review from benbardin, miretskiy, msirek, ZhouXing19 and DrewKimball and removed request for a team, benbardin and miretskiy March 15, 2023 05:25
@yuzefovich
Copy link
Member Author

The first commit is in #98654, so it should be ignored here.

@yuzefovich yuzefovich removed the request for review from msirek March 15, 2023 06:06
Copy link
Collaborator

@ZhouXing19 ZhouXing19 left a comment

Choose a reason for hiding this comment

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

Thanks for making this! It will be very helpful for multiple active portals.
I just reviewed the 3rd commit and it lgtm. Just left nit comments.
Would like to have the queries team to finalize the review as I'm not familiar with this part of the codebase.

Reviewed 2 of 3 files at r2, 78 of 78 files at r3, all commit messages.
Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @DrewKimball and @yuzefovich)


pkg/sql/execinfra/processorsbase.go line 723 at r3 (raw file):

// Run is part of the Processor interface.
func (pb *ProcessorBaseNoHelper) Run(ctx context.Context, output RowReceiver) {
	pb.self.Start(ctx)

nit: maybe we should still have a nil check for output


pkg/sql/flowinfra/flow.go line 369 at r3 (raw file):

// SetProcessors overrides the current f.processors and f.outputs with the
// provided slices.
func (f *FlowBase) SetProcessors(

nit: might be more explicit if rename it to SetProcessorsAndOutputs


pkg/sql/flowinfra/flow.go line 409 at r3 (raw file):

	ctx context.Context, processors []execinfra.Processor, outputs []execinfra.RowReceiver,
) error {
	log.VEventf(

nit : do we want to assert len(processors) == len(outputs) ?


pkg/sql/flowinfra/flow.go line 478 at r3 (raw file):

// Run is part of the Flow interface.
func (f *FlowBase) Run(ctx context.Context) {
	defer f.Wait()

nit: ditto, assert the lengths are equal?


pkg/sql/rowexec/columnbackfiller.go line 63 at r3 (raw file):

	processorID int32,
	spec execinfrapb.BackfillerSpec,
	post *execinfrapb.PostProcessSpec,

nit: this post arg is not used. Remove?


pkg/sql/rowexec/indexbackfiller.go line 72 at r3 (raw file):

	ctx context.Context,
	flowCtx *execinfra.FlowCtx,
	processorID int32,

nit: processorID and post are never used.


pkg/sql/rowexec/ordinality.go line 42 at r3 (raw file):

	flowCtx *execinfra.FlowCtx,
	processorID int32,
	spec *execinfrapb.OrdinalitySpec,

nit: spec is never used


pkg/sql/rowexec/processors.go line 117 at r3 (raw file):

) (execinfra.Processor, error) {
	if core.Noop != nil {
		if err := checkNumIn(inputs, 1); err != nil {

I'm not familiar with this part of code, but is this length check for input and output important? Do we need to implement something similar for the output in FlowBase.Run()?


pkg/sql/rowflow/row_based_flow.go line 205 at r3 (raw file):

}

func (f *rowBasedFlow) makeProcessor(

nit : rename it to makeProcessorAndReceiver?

@benbardin
Copy link
Collaborator

Approved for backupccl

This commit refactors the vectorized flow creator to directly embed two
structs that implement interfaces used by the creator. This allows for
those structs to not be allocated on the main code path. Also,
`flowCreatorHelper` no longer implements the `Releasable` interface.

Release note: None
This commit modifies how the output of a processor is plumbed through.
Previously, we would pass it on the processor construction and store in
the `ProcessorBaseNoHelper` to later be used when calling
`execinfra.Run` method. However, the upcoming work on multiple active
portals will require updating the output when resuming the flow, so this
commit makes it so that we now pass the output explicitly into
`execinfra.Processor.Run` method. This will make the work on multiple
active portals cleaner, but also this change seems like a good one
independent of that because we're able to store all outputs in the
single place (`FlowBase.outputs`).

Release note: None
Copy link
Member Author

@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.

Thanks for taking a look both! Addressed the comments so far.

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


pkg/sql/flowinfra/flow.go line 478 at r3 (raw file):

Previously, ZhouXing19 (Jane Xing) wrote…

nit: ditto, assert the lengths are equal?

I added the check to FlowBase.SetProcessorsAndOuputs.


pkg/sql/rowexec/ordinality.go line 42 at r3 (raw file):

Previously, ZhouXing19 (Jane Xing) wrote…

nit: spec is never used

I'll keep this one for consistency with other processors.


pkg/sql/rowexec/processors.go line 117 at r3 (raw file):

Previously, ZhouXing19 (Jane Xing) wrote…

I'm not familiar with this part of code, but is this length check for input and output important? Do we need to implement something similar for the output in FlowBase.Run()?

I don't think we need to something like this in FlowBase.Run. The idea here is that we perform this check on a per-processor basis, so it should only be done in NewProcessor (since each processor can have different number of inputs). I simplified this function to only check the number of inputs since all processors have exactly one output, and that is now "encoded" into execinfra.Processor.Run signature, so no need to check the number of outputs here.

Copy link
Collaborator

@DrewKimball DrewKimball 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 3 of 3 files at r1, 3 of 3 files at r2, 78 of 78 files at r3, 78 of 78 files at r4, 78 of 78 files at r5, all commit messages.
Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained (waiting on @ZhouXing19)

@yuzefovich
Copy link
Member Author

TFTRs!

bors r+

@craig
Copy link
Contributor

craig bot commented Mar 15, 2023

Build failed (retrying...):

@craig
Copy link
Contributor

craig bot commented Mar 15, 2023

Build succeeded:

@craig craig bot merged commit 77a55f9 into cockroachdb:master Mar 15, 2023
@yuzefovich yuzefovich deleted the run-output branch March 15, 2023 21:36
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.

5 participants