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

colexecproj: break it down into two packages #79462

Merged
merged 2 commits into from
Apr 8, 2022

Conversation

yuzefovich
Copy link
Member

@yuzefovich yuzefovich commented Apr 5, 2022

colexecproj: split up default cmp proj op file into two

This commit splits up a single file containing two default comparison
projection operators into two files. This is done in preparation of
the following commit (which will move one of the operators to a
different package).

Release note: None

colexecproj: extract a new package for projection ops with const

This commit extracts a new colexecprojconst package out of
colexecproj that contains all projection operators with one
constant argument. This will allow for faster build speeds since both
packages tens of thousands lines of code.

Special care had to be taken for default comparison operator because we
need to generate two files in different packages based on a single
template. I followed the precedent of sort_partitioner.eg.go which had
to do the same.

Addresses: #79357.

Release note: None

@cockroach-teamcity
Copy link
Member

This change is Reviewable

@yuzefovich yuzefovich marked this pull request as ready for review April 6, 2022 04:33
@yuzefovich yuzefovich requested review from a team as code owners April 6, 2022 04:33
@yuzefovich yuzefovich requested review from irfansharif, rickystewart and a team and removed request for a team April 6, 2022 04:33
@yuzefovich yuzefovich requested a review from michae2 April 7, 2022 21:01
Copy link
Collaborator

@michae2 michae2 left a comment

Choose a reason for hiding this comment

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

:lgtm: Just a couple bazel questions, but this looks good.

Reviewed 7 of 7 files at r1, 32 of 32 files at r2, all commit messages.
Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained (waiting on @irfansharif, @rickystewart, and @yuzefovich)


pkg/sql/colexec/COLEXEC.bzl, line 26 at r2 (raw file):

        cmd = """\
export COCKROACH_INTERNAL_DISABLE_METAMORPHIC_TESTING=true
$(location :execgen) -template $(SRCS) -fmt=false pkg/sql/colexec/colexecprojconst$@ > $@

Should this have a / after colexecprojconst?


pkg/sql/colexec/colexecprojconst/BUILD.bazel, line 24 at r2 (raw file):

        "//pkg/sql/colexec/colexecbase",  # keep
        "//pkg/sql/colexec/colexeccmp",
        "//pkg/sql/colexec/colexecutils",

In colexecproj/BUILD.bazel all deps now have # keep. Should they all have it here as well?


pkg/sql/colexec/execgen/cmd/execgen/default_cmp_proj_ops_gen.go, line 35 at r1 (raw file):

// licenses/APL.txt.

package %s

nit: Since this is a template, I think you could use {{.TargetPkg}} instead of %s if you wanted to avoid the call to fmt.Sprintf. (Then you'd need to add TargetPkg to defaultCmpProjOpOverload.)

This commit splits up a single file containing two default comparison
projection operators into two files. This is done in preparation of
the following commit (which will move one of the operators to a
different package).

Release note: None
This commit extracts a new `colexecprojconst` package out of
`colexecproj` that contains all projection operators with one
constant argument. This will allow for faster build speeds since both
packages tens of thousands lines of code.

Special care had to be taken for default comparison operator because we
need to generate two files in different packages based on a single
template. I followed the precedent of `sort_partitioner.eg.go` which had
to do the same.

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.

TFTR!

bors r+

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @irfansharif, @michae2, and @rickystewart)


pkg/sql/colexec/COLEXEC.bzl, line 26 at r2 (raw file):

Previously, michae2 (Michael Erickson) wrote…

Should this have a / after colexecprojconst?

Nice catch! I don't know if it matters (because the bazel build still works), but it's good to be consistent with the above, fixed.


pkg/sql/colexec/colexecprojconst/BUILD.bazel, line 24 at r2 (raw file):

Previously, michae2 (Michael Erickson) wrote…

In colexecproj/BUILD.bazel all deps now have # keep. Should they all have it here as well?

# keep is a special directive to make sure that bazel doesn't prune dependencies that are needed only by the generated code. In other words, if we can get by without using # keep (meaning that there is regular, non-generated code that needs a particular dependency), then it's preferable, but in colexecproj we no longer have such code, so we have to use # keep directive.


pkg/sql/colexec/execgen/cmd/execgen/default_cmp_proj_ops_gen.go, line 35 at r1 (raw file):

Previously, michae2 (Michael Erickson) wrote…

nit: Since this is a template, I think you could use {{.TargetPkg}} instead of %s if you wanted to avoid the call to fmt.Sprintf. (Then you'd need to add TargetPkg to defaultCmpProjOpOverload.)

Nice, I like this, done.

@craig
Copy link
Contributor

craig bot commented Apr 8, 2022

Build succeeded:

@craig craig bot merged commit 15bbd78 into cockroachdb:master Apr 8, 2022
@yuzefovich yuzefovich deleted the colexecprojconst branch April 8, 2022 00:43
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