Skip to content

Commit

Permalink
colexecproj: extract a new package for projection ops with const
Browse files Browse the repository at this point in the history
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
  • Loading branch information
yuzefovich committed Apr 7, 2022
1 parent 4f9c27b commit 2862dee
Show file tree
Hide file tree
Showing 32 changed files with 326 additions and 181 deletions.
8 changes: 4 additions & 4 deletions Makefile
Original file line number Diff line number Diff line change
Expand Up @@ -865,12 +865,12 @@ EXECGEN_TARGETS = \
pkg/sql/colexec/colexecjoin/mergejoiner_rightanti.eg.go \
pkg/sql/colexec/colexecjoin/mergejoiner_rightouter.eg.go \
pkg/sql/colexec/colexecjoin/mergejoiner_rightsemi.eg.go \
pkg/sql/colexec/colexecproj/default_cmp_proj_const_op.eg.go \
pkg/sql/colexec/colexecproj/default_cmp_proj_op.eg.go \
pkg/sql/colexec/colexecproj/proj_const_left_ops.eg.go \
pkg/sql/colexec/colexecproj/proj_const_right_ops.eg.go \
pkg/sql/colexec/colexecproj/proj_like_ops.eg.go \
pkg/sql/colexec/colexecproj/proj_non_const_ops.eg.go \
pkg/sql/colexec/colexecprojconst/default_cmp_proj_const_op.eg.go \
pkg/sql/colexec/colexecprojconst/proj_const_left_ops.eg.go \
pkg/sql/colexec/colexecprojconst/proj_const_right_ops.eg.go \
pkg/sql/colexec/colexecprojconst/proj_like_ops.eg.go \
pkg/sql/colexec/colexecsel/default_cmp_sel_ops.eg.go \
pkg/sql/colexec/colexecsel/selection_ops.eg.go \
pkg/sql/colexec/colexecsel/sel_like_ops.eg.go \
Expand Down
2 changes: 2 additions & 0 deletions pkg/BUILD.bazel
Original file line number Diff line number Diff line change
Expand Up @@ -248,6 +248,8 @@ ALL_TESTS = [
"//pkg/sql/colexec/colexecjoin:colexecjoin_test",
"//pkg/sql/colexec/colexecproj:colexecproj_disallowed_imports_test",
"//pkg/sql/colexec/colexecproj:colexecproj_test",
"//pkg/sql/colexec/colexecprojconst:colexecprojconst_disallowed_imports_test",
"//pkg/sql/colexec/colexecprojconst:colexecprojconst_test",
"//pkg/sql/colexec/colexecsel:colexecsel_disallowed_imports_test",
"//pkg/sql/colexec/colexecsel:colexecsel_test",
"//pkg/sql/colexec/colexecspan:colexecspan_disallowed_imports_test",
Expand Down
8 changes: 4 additions & 4 deletions pkg/gen/execgen.bzl
Original file line number Diff line number Diff line change
Expand Up @@ -50,12 +50,12 @@ EXECGEN_SRCS = [
"//pkg/sql/colexec/colexecjoin:mergejoiner_rightanti.eg.go",
"//pkg/sql/colexec/colexecjoin:mergejoiner_rightouter.eg.go",
"//pkg/sql/colexec/colexecjoin:mergejoiner_rightsemi.eg.go",
"//pkg/sql/colexec/colexecproj:default_cmp_proj_const_op.eg.go",
"//pkg/sql/colexec/colexecproj:default_cmp_proj_op.eg.go",
"//pkg/sql/colexec/colexecproj:proj_const_left_ops.eg.go",
"//pkg/sql/colexec/colexecproj:proj_const_right_ops.eg.go",
"//pkg/sql/colexec/colexecproj:proj_like_ops.eg.go",
"//pkg/sql/colexec/colexecproj:proj_non_const_ops.eg.go",
"//pkg/sql/colexec/colexecprojconst:default_cmp_proj_const_op.eg.go",
"//pkg/sql/colexec/colexecprojconst:proj_const_left_ops.eg.go",
"//pkg/sql/colexec/colexecprojconst:proj_const_right_ops.eg.go",
"//pkg/sql/colexec/colexecprojconst:proj_like_ops.eg.go",
"//pkg/sql/colexec/colexecsel:default_cmp_sel_ops.eg.go",
"//pkg/sql/colexec/colexecsel:sel_like_ops.eg.go",
"//pkg/sql/colexec/colexecsel:selection_ops.eg.go",
Expand Down
16 changes: 16 additions & 0 deletions pkg/sql/colexec/COLEXEC.bzl
Original file line number Diff line number Diff line change
Expand Up @@ -13,3 +13,19 @@ $(location :goimports) -w $@
tools = [":execgen", ":goimports"],
visibility = [":__pkg__", "//pkg/gen:__pkg__"],
)

# Generating the code for `default_cmp_proj_const_op.eg.go` requires special
# handling because the template lives in a different package.
def gen_default_cmp_proj_const_rule(name, target, visibility = ["//visibility:private"]):
native.genrule(
name = name,
srcs = ["//pkg/sql/colexec/colexecproj:default_cmp_proj_ops_tmpl.go"],
outs = [target],
cmd = """\
export COCKROACH_INTERNAL_DISABLE_METAMORPHIC_TESTING=true
$(location :execgen) -template $(SRCS) -fmt=false pkg/sql/colexec/colexecprojconst/$@ > $@
$(location :goimports) -w $@
""",
tools = [":execgen", ":goimports"],
visibility = [":__pkg__", "//pkg/gen:__pkg__"],
)
1 change: 1 addition & 0 deletions pkg/sql/colexec/colbuilder/BUILD.bazel
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ go_library(
"//pkg/sql/colexec/colexecbase",
"//pkg/sql/colexec/colexecjoin",
"//pkg/sql/colexec/colexecproj",
"//pkg/sql/colexec/colexecprojconst",
"//pkg/sql/colexec/colexecsel",
"//pkg/sql/colexec/colexecutils",
"//pkg/sql/colexec/colexecwindow",
Expand Down
7 changes: 4 additions & 3 deletions pkg/sql/colexec/colbuilder/execplan.go
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ import (
"github.com/cockroachdb/cockroach/pkg/sql/colexec/colexecbase"
"github.com/cockroachdb/cockroach/pkg/sql/colexec/colexecjoin"
"github.com/cockroachdb/cockroach/pkg/sql/colexec/colexecproj"
"github.com/cockroachdb/cockroach/pkg/sql/colexec/colexecprojconst"
"github.com/cockroachdb/cockroach/pkg/sql/colexec/colexecsel"
"github.com/cockroachdb/cockroach/pkg/sql/colexec/colexecutils"
"github.com/cockroachdb/cockroach/pkg/sql/colexec/colexecwindow"
Expand Down Expand Up @@ -2335,7 +2336,7 @@ func planProjectionExpr(
resultIdx = len(typs)
// The projection result will be outputted to a new column which is
// appended to the input batch.
op, err = colexecproj.GetProjectionLConstOperator(
op, err = colexecprojconst.GetProjectionLConstOperator(
allocator, typs, left.ResolvedType(), outputType, projOp, input,
rightIdx, lConstArg, resultIdx, evalCtx, binFn, cmpExpr,
)
Expand Down Expand Up @@ -2381,7 +2382,7 @@ func planProjectionExpr(
switch cmpProjOp.Symbol {
case treecmp.Like, treecmp.NotLike:
negate := cmpProjOp.Symbol == treecmp.NotLike
op, err = colexecproj.GetLikeProjectionOperator(
op, err = colexecprojconst.GetLikeProjectionOperator(
allocator, evalCtx, input, leftIdx, resultIdx,
string(tree.MustBeDString(rConstArg)), negate,
)
Expand Down Expand Up @@ -2412,7 +2413,7 @@ func planProjectionExpr(
if op == nil || err != nil {
// op hasn't been created yet, so let's try the constructor for
// all other projection operators.
op, err = colexecproj.GetProjectionRConstOperator(
op, err = colexecprojconst.GetProjectionRConstOperator(
allocator, typs, right.ResolvedType(), outputType, projOp,
input, leftIdx, rConstArg, resultIdx, evalCtx, binFn, cmpExpr,
)
Expand Down
1 change: 1 addition & 0 deletions pkg/sql/colexec/colexecagg/BUILD.bazel
Original file line number Diff line number Diff line change
Expand Up @@ -79,6 +79,7 @@ disallowed_imports_test(
"//pkg/sql/colexec/colexechash",
"//pkg/sql/colexec/colexecjoin",
"//pkg/sql/colexec/colexecproj",
"//pkg/sql/colexec/colexecprojconst",
"//pkg/sql/colexec/colexecsel",
"//pkg/sql/colexec/colexecwindow",
],
Expand Down
1 change: 1 addition & 0 deletions pkg/sql/colexec/colexecbase/BUILD.bazel
Original file line number Diff line number Diff line change
Expand Up @@ -109,6 +109,7 @@ disallowed_imports_test(
"//pkg/sql/colexec/colexechash",
"//pkg/sql/colexec/colexecjoin",
"//pkg/sql/colexec/colexecproj",
"//pkg/sql/colexec/colexecprojconst",
"//pkg/sql/colexec/colexecsel",
"//pkg/sql/colexec/colexecwindow",
],
Expand Down
1 change: 1 addition & 0 deletions pkg/sql/colexec/colexechash/BUILD.bazel
Original file line number Diff line number Diff line change
Expand Up @@ -80,6 +80,7 @@ disallowed_imports_test(
"//pkg/sql/colexec/colexecbase",
"//pkg/sql/colexec/colexecjoin",
"//pkg/sql/colexec/colexecproj",
"//pkg/sql/colexec/colexecprojconst",
"//pkg/sql/colexec/colexecsel",
"//pkg/sql/colexec/colexecwindow",
],
Expand Down
1 change: 1 addition & 0 deletions pkg/sql/colexec/colexecjoin/BUILD.bazel
Original file line number Diff line number Diff line change
Expand Up @@ -105,6 +105,7 @@ disallowed_imports_test(
"//pkg/sql/colexec",
"//pkg/sql/colexec/colexecagg",
"//pkg/sql/colexec/colexecproj",
"//pkg/sql/colexec/colexecprojconst",
"//pkg/sql/colexec/colexecsel",
"//pkg/sql/colexec/colexecwindow",
],
Expand Down
28 changes: 14 additions & 14 deletions pkg/sql/colexec/colexecproj/BUILD.bazel
Original file line number Diff line number Diff line change
Expand Up @@ -5,10 +5,9 @@ load("//pkg/testutils/buildutil:buildutil.bzl", "disallowed_imports_test")
go_library(
name = "colexecproj",
srcs = [
"like_ops.go",
":gen-exec", # keep
],
importpath = "github.com/cockroachdb/cockroach/pkg/sql/colexec/colexecproj",
importpath = "github.com/cockroachdb/cockroach/pkg/sql/colexec/colexecproj", # keep
visibility = ["//visibility:public"],
deps = [
"//pkg/col/coldata", # keep
Expand All @@ -17,22 +16,22 @@ go_library(
"//pkg/server/telemetry", # keep
"//pkg/sql/colconv", # keep
"//pkg/sql/colexec/colexecbase", # keep
"//pkg/sql/colexec/colexeccmp",
"//pkg/sql/colexec/colexecutils",
"//pkg/sql/colexec/colexeccmp", # keep
"//pkg/sql/colexec/colexecutils", # keep
"//pkg/sql/colexec/execgen", # keep
"//pkg/sql/colexecerror", # keep
"//pkg/sql/colexecop",
"//pkg/sql/colmem",
"//pkg/sql/colexecop", # keep
"//pkg/sql/colmem", # keep
"//pkg/sql/execinfra", # keep
"//pkg/sql/sem/tree",
"//pkg/sql/sem/tree", # keep
"//pkg/sql/sem/tree/treebin", # keep
"//pkg/sql/sem/tree/treecmp", # keep
"//pkg/sql/sqltelemetry", # keep
"//pkg/sql/types",
"//pkg/sql/types", # keep
"//pkg/util/duration", # keep
"//pkg/util/json", # keep
"@com_github_cockroachdb_apd_v3//:apd", # keep
"@com_github_cockroachdb_errors//:errors",
"@com_github_cockroachdb_errors//:errors", # keep
],
)

Expand All @@ -44,7 +43,7 @@ go_test(
"main_test.go",
"projection_ops_test.go",
],
embed = [":colexecproj"],
embed = [":colexecproj"], # keep
tags = ["no-remote"],
deps = [
"//pkg/col/coldata",
Expand Down Expand Up @@ -74,13 +73,13 @@ go_test(
],
)

# Export the template because it is used by another target in colexecprojconst
# package.
exports_files(["default_cmp_proj_ops_tmpl.go"])

# Map between target name and relevant template.
targets = [
("default_cmp_proj_const_op.eg.go", "default_cmp_proj_ops_tmpl.go"),
("default_cmp_proj_op.eg.go", "default_cmp_proj_ops_tmpl.go"),
("proj_const_left_ops.eg.go", "proj_const_ops_tmpl.go"),
("proj_const_right_ops.eg.go", "proj_const_ops_tmpl.go"),
("proj_like_ops.eg.go", "proj_const_ops_tmpl.go"),
("proj_non_const_ops.eg.go", "proj_non_const_ops_tmpl.go"),
]

Expand All @@ -100,6 +99,7 @@ disallowed_imports_test(
"//pkg/sql/colexec/colexecagg",
"//pkg/sql/colexec/colexechash",
"//pkg/sql/colexec/colexecjoin",
"//pkg/sql/colexec/colexecprojconst",
"//pkg/sql/colexec/colexecsel",
"//pkg/sql/colexec/colexecwindow",
],
Expand Down
12 changes: 0 additions & 12 deletions pkg/sql/colexec/colexecproj/proj_non_const_ops.eg.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

12 changes: 0 additions & 12 deletions pkg/sql/colexec/colexecproj/proj_non_const_ops_tmpl.go
Original file line number Diff line number Diff line change
Expand Up @@ -76,18 +76,6 @@ func _ASSIGN(_, _, _, _, _, _ interface{}) {

// */}}

// projConstOpBase contains all of the fields for projections with a constant,
// except for the constant itself.
// NOTE: this struct should be declared in proj_const_ops_tmpl.go, but if we do
// so, it'll be redeclared because we execute that template twice. To go
// around the problem we specify it here.
type projConstOpBase struct {
colexecop.OneInputHelper
allocator *colmem.Allocator
colIdx int
outputIdx int
}

// projOpBase contains all of the fields for non-constant projections.
type projOpBase struct {
colexecop.OneInputHelper
Expand Down
64 changes: 0 additions & 64 deletions pkg/sql/colexec/colexecproj/projection_ops_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -102,70 +102,6 @@ func TestProjDivFloat64Float64Op(t *testing.T) {
})
}

func TestGetProjectionConstOperator(t *testing.T) {
defer leaktest.AfterTest(t)()
defer log.Scope(t).Close(t)
binOp := treebin.MakeBinaryOperator(treebin.Mult)
var input colexecop.Operator
colIdx := 3
inputTypes := make([]*types.T, colIdx+1)
inputTypes[colIdx] = types.Float
constVal := 31.37
constArg := tree.NewDFloat(tree.DFloat(constVal))
outputIdx := 5
op, err := GetProjectionRConstOperator(
testAllocator, inputTypes, types.Float, types.Float, binOp, input, colIdx,
constArg, outputIdx, nil /* EvalCtx */, nil /* BinFn */, nil, /* cmpExpr */
)
if err != nil {
t.Error(err)
}
expected := &projMultFloat64Float64ConstOp{
projConstOpBase: projConstOpBase{
OneInputHelper: colexecop.MakeOneInputHelper(op.(*projMultFloat64Float64ConstOp).Input),
allocator: testAllocator,
colIdx: colIdx,
outputIdx: outputIdx,
},
constArg: constVal,
}
if !reflect.DeepEqual(op, expected) {
t.Errorf("got %+v,\nexpected %+v", op, expected)
}
}

func TestGetProjectionConstMixedTypeOperator(t *testing.T) {
defer leaktest.AfterTest(t)()
defer log.Scope(t).Close(t)
cmpOp := treecmp.MakeComparisonOperator(treecmp.GE)
var input colexecop.Operator
colIdx := 3
inputTypes := make([]*types.T, colIdx+1)
inputTypes[colIdx] = types.Int
constVal := int16(31)
constArg := tree.NewDInt(tree.DInt(constVal))
outputIdx := 5
op, err := GetProjectionRConstOperator(
testAllocator, inputTypes, types.Int2, types.Int, cmpOp, input, colIdx,
constArg, outputIdx, nil /* EvalCtx */, nil /* BinFn */, nil, /* cmpExpr */
)
if err != nil {
t.Error(err)
}
expected := &projGEInt64Int16ConstOp{
projConstOpBase: projConstOpBase{
OneInputHelper: colexecop.MakeOneInputHelper(op.(*projGEInt64Int16ConstOp).Input),
allocator: testAllocator,
colIdx: colIdx,
outputIdx: outputIdx,
},
constArg: constVal,
}
if !reflect.DeepEqual(op, expected) {
t.Errorf("got %+v,\nexpected %+v", op, expected)
}
}

// TestRandomComparisons runs comparisons against all scalar types with random
// non-null data verifying that the result of Datum.Compare matches the result
// of the exec projection.
Expand Down
Loading

0 comments on commit 2862dee

Please sign in to comment.