Skip to content

exec: template the distinct operator#31957

Merged
craig[bot] merged 1 commit intocockroachdb:masterfrom
jordanlewis:distinct_tmpl
Oct 31, 2018
Merged

exec: template the distinct operator#31957
craig[bot] merged 1 commit intocockroachdb:masterfrom
jordanlewis:distinct_tmpl

Conversation

@jordanlewis
Copy link
Copy Markdown
Member

@jordanlewis jordanlewis commented Oct 28, 2018 •
edited
Loading

Template the distinct operator using a new approach, where the template
file is normal executable Go, but prevented from being compiled normally
by a build tag.

The generator file reads the template file in, replaces some special
variables with template variables, and runs the template.

There are two main warts with this approach:

  1. ColVec needed to be extended with a dummy _TemplateType method whose
    signature is _TemplateType() []interface{}, so the template can
    compile. That method is implemented to always panic().
  2. The template generator needs to textually replace things in a way
    that can be confusing. _TYPE, _GOTYPE, _TYPES_T, _EQUALITY_FN, and
    _TemplateType all need to be replaced, with {{.}}, {{.GoTypeName}},
    types.{{.}}, {{.EqualityFunction}} and {{.}} respectively.

Also, rename distinctFinalizerOp to boolVecToSelOp and move to its own
file. The implementation doesn't change.

Release note: None

@cockroach-teamcity
Copy link
Copy Markdown
Member

cockroach-teamcity commented Oct 28, 2018

This change is Reviewable

@jordanlewis jordanlewis force-pushed the distinct_tmpl branch 6 times, most recently from 3924498 to a7da435 Compare October 29, 2018 20:37
@jordanlewis
Copy link
Copy Markdown
Member Author

jordanlewis commented Oct 30, 2018

PTAL

asubiotto
asubiotto approved these changes Oct 31, 2018
View reviewed changes
Copy link
Copy Markdown
Contributor

@asubiotto asubiotto left a comment

Choose a reason for hiding this comment

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

LGTM. Very educational PR.

Reviewed 6 of 8 files at r1.
Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained

Makefile, line 1315 at r1 (raw file):

	@$(settings-doc-gen) gen settings-list --format=html > $@

pkg/sql/exec/distinct.og.go: pkg/sql/exec/distinct_tmpl.go

Is it necessary to have the _tmpl and _gen files in pkg/sql/exec? I like how the projection ops template is in execgen but I guess that might be more difficult to do with this approach. This is just being nitpicky but wondering if we can find a better structure.

@jordanlewis
Copy link
Copy Markdown
Member Author

jordanlewis commented Oct 31, 2018

distinct_gen.go is already in execgen, and it's pretty hard to not have the template in the main package if we want to try to make it read like normal Go, for a couple of reasons. I actually kind of like having the template in the main package - as it's the thing that I would probably prefer to read when understanding what the code does.

That being said I do hear your point. If you can figure out a better way for this I'm all ears, and I'm definitely going to keep thinking on it over time.

bors r+

@craig
Copy link
Copy Markdown
Contributor

craig bot commented Oct 31, 2018

Build failed (retrying...)

@craig
Copy link
Copy Markdown
Contributor

craig bot commented Oct 31, 2018

Canceled

@jordanlewis
Copy link
Copy Markdown
Member Author

jordanlewis commented Oct 31, 2018

Needed to rebase to pick up the decimal/date stuff, good to go now though.

bors r+

craig bot pushed a commit that referenced this pull request Oct 31, 2018
@jordanlewis
Merge #31957
7ae78a1 
31957: exec: template the distinct operator r=jordanlewis a=jordanlewis

Template the distinct operator using a new approach, where the template
file is normal executable Go, but prevented from being compiled normally
by a build tag.

The generator file reads the template file in, replaces some special
variables with template variables, and runs the template.

There are two main warts with this approach:

1. ColVec needed to be extended with a dummy _TemplateType method whose
   signature is _TemplateType() []interface{}, so the template can
   compile. That method is implemented to always panic().
2. The template generator needs to textually replace things in a way
   that can be confusing. _TYPE, _GOTYPE, _TYPES_T, _EQUALITY_FN, and
   _TemplateType all need to be replaced, with {{.}}, {{.GoTypeName}},
   types.{{.}}, {{.EqualityFunction}} and {{.}} respectively.

Also, rename distinctFinalizerOp to boolVecToSelOp and move to its own
file. The implementation doesn't change.

Release note: None

Co-authored-by: Jordan Lewis <jordanthelewis@gmail.com>
@benesch benesch requested review from benesch and removed request for benesch October 31, 2018 19:02
@jordanlewis
exec: template the distinct operator
c907806 
Template the distinct operator using a new approach, where the template
file is normal executable Go, but prevented from being compiled normally
by a build tag.

The generator file reads the template file in, replaces some special
variables with template variables, and runs the template.

There are two main warts with this approach:

1. ColVec needed to be extended with a dummy _TemplateType method whose
   signature is _TemplateType() []interface{}, so the template can
   compile. That method is implemented to always panic().
2. The template generator needs to textually replace things in a way
   that can be confusing. _TYPE, _GOTYPE, _TYPES_T, _EQUALITY_FN, and
   _TemplateType all need to be replaced, with {{.}}, {{.GoTypeName}},
   types.{{.}}, {{.EqualityFunction}} and {{.}} respectively.

Also, rename distinctFinalizerOp to boolVecToSelOp and move to its own
file. The implementation doesn't change.

Release note: None
@jordanlewis
Copy link
Copy Markdown
Member Author

jordanlewis commented Oct 31, 2018

bors r+

@craig
Copy link
Copy Markdown
Contributor

craig bot commented Oct 31, 2018

Build failed

@jordanlewis
Copy link
Copy Markdown
Member Author

jordanlewis commented Oct 31, 2018

Flaky test: TestStoreRangeMergeAbandonedFollowersAutomaticallyGarbageCollected failed. Going to file an issue for that and retrigger. This is frustrating.

bors r+

@jordanlewis jordanlewis mentioned this pull request Oct 31, 2018
Closed
@jordanlewis
Copy link
Copy Markdown
Member Author

jordanlewis commented Oct 31, 2018

The batch crashed. Bors really hates this PR.

bors r+

craig bot pushed a commit that referenced this pull request Oct 31, 2018
@jordanlewis
Merge #31957
40cbbf0 
31957: exec: template the distinct operator r=jordanlewis a=jordanlewis

Template the distinct operator using a new approach, where the template
file is normal executable Go, but prevented from being compiled normally
by a build tag.

The generator file reads the template file in, replaces some special
variables with template variables, and runs the template.

There are two main warts with this approach:

1. ColVec needed to be extended with a dummy _TemplateType method whose
   signature is _TemplateType() []interface{}, so the template can
   compile. That method is implemented to always panic().
2. The template generator needs to textually replace things in a way
   that can be confusing. _TYPE, _GOTYPE, _TYPES_T, _EQUALITY_FN, and
   _TemplateType all need to be replaced, with {{.}}, {{.GoTypeName}},
   types.{{.}}, {{.EqualityFunction}} and {{.}} respectively.

Also, rename distinctFinalizerOp to boolVecToSelOp and move to its own
file. The implementation doesn't change.

Release note: None

Co-authored-by: Jordan Lewis <jordanthelewis@gmail.com>
@craig
Copy link
Copy Markdown
Contributor

craig bot commented Oct 31, 2018

Build succeeded

@craig craig bot merged commit c907806 into cockroachdb:master Oct 31, 2018
@jordanlewis jordanlewis deleted the distinct_tmpl branch October 31, 2018 22:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@solongordon solongordon Awaiting requested review from solongordon

@changangela changangela Awaiting requested review from changangela

1 more reviewer

@asubiotto asubiotto asubiotto approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@jordanlewis @cockroach-teamcity @asubiotto