Skip to content

colexecspan: de-templatize span assembler and use span.Splitter#76901

Merged
craig[bot] merged 2 commits intocockroachdb:masterfrom
RaduBerinde:span-assembler-no-tmpl
Feb 23, 2022
Merged

colexecspan: de-templatize span assembler and use span.Splitter#76901
craig[bot] merged 2 commits intocockroachdb:masterfrom
RaduBerinde:span-assembler-no-tmpl

Conversation

@RaduBerinde
Copy link
Member

@RaduBerinde RaduBerinde commented Feb 22, 2022

colexecspan: de-templatize span assembler

The span assembler code is generated only to inline a piece of code
that has two variants. This change converts it to non-generated code
and simply forks the code paths above the batch loop.

Release note: None

colexecspan: use span.Splitter

The span assembler duplicates some of the logic in span.Splitter.
Now that the latter is a separate type, we can use it instead.

Release note: None

@RaduBerinde RaduBerinde requested review from a team and yuzefovich February 22, 2022 19:56
@RaduBerinde RaduBerinde requested a review from a team as a code owner February 22, 2022 19:56
@cockroach-teamcity
Copy link
Member

This change is Reviewable

@RaduBerinde RaduBerinde force-pushed the span-assembler-no-tmpl branch from 647648b to 807fe6c Compare February 22, 2022 21:09
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 1 of 1 files at r1, 6 of 6 files at r2, 3 of 3 files at r3, all commit messages.
Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained (waiting on @RaduBerinde)


-- commits, line 2 at r1:
In order to not break the build (and keep the bisect working), we need to do the same changes in span_assembler_gen.go and colexecspan/BUILD.bazel.


pkg/sql/span/span_splitter.go, line 87 at r3 (raw file):

}

// FamilyIDs returns the family IDs into which spans will be split, or nil if

nit: maybe add this as a comment on neededFamilies too.

The span assembler code is generated only to inline a piece of code
that has two variants. This change converts it to non-generated code
and simply forks the code paths above the batch loop.

Release note: None
The span assembler duplicates some of the logic in `span.Splitter`.
Now that the latter is a separate type, we can use it instead.

Release note: None
@RaduBerinde RaduBerinde force-pushed the span-assembler-no-tmpl branch from 807fe6c to 9b6a2f1 Compare February 22, 2022 22:20
Copy link
Member Author

@RaduBerinde RaduBerinde left a comment

Choose a reason for hiding this comment

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

TFTR!

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


-- commits, line 2 at r1:

Previously, yuzefovich (Yahor Yuzefovich) wrote…

In order to not break the build (and keep the bisect working), we need to do the same changes in span_assembler_gen.go and colexecspan/BUILD.bazel.

I just squashed the commits, it was only to make the review easier.

@RaduBerinde
Copy link
Member Author

bors r+

@craig
Copy link
Contributor

craig bot commented Feb 23, 2022

Build failed (retrying...):

@craig
Copy link
Contributor

craig bot commented Feb 23, 2022

Build succeeded:

@craig craig bot merged commit 5522769 into cockroachdb:master Feb 23, 2022
@RaduBerinde RaduBerinde deleted the span-assembler-no-tmpl branch March 19, 2022 01:33
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

Comments