-
Notifications
You must be signed in to change notification settings - Fork 3.8k
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
bazel: generate colexecagg .eg.go files within the sandbox #58479
Conversation
Thank you for contributing to CockroachDB. Please ensure you have followed the guidelines for creating a PR. My owl senses detect your PR is good for review. Please keep an eye out for any test failures in CI. I was unable to automatically find a reviewer. You can try CCing one of the following members:
🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is otan. |
0f45612
to
5cd70a8
Compare
Thank you for updating your pull request. My owl senses detect your PR is good for review. Please keep an eye out for any test failures in CI. 🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is otan. |
46567bd
to
9e3c47d
Compare
@@ -0,0 +1,70 @@ | |||
# Map between target name and relevant template. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd prefer to just put all of this in pkg/sql/colexec/colexecagg/BUILD.bazel
. eg_go_filegroup
and gen_eg_go_rules
are not reusable, so defining them as utility functions doesn't provide any value. Just makes things more complicated for no reason.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am applying the same same treatment as 52c5f51. By pulling that inside pkg/sql/colexec/colexecagg/BUILD.bazel
you mean follow the same logic that is already there in this PR or creating one gen_rule
per file?
@@ -0,0 +1,70 @@ | |||
# Map between target name and relevant template. | |||
targets = [ | |||
('hash_any_not_null_agg.eg.go', 'any_not_null_agg_tmpl.go'), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you just make this into a helper function? Something like:
def munge(s):
if s.startswith('hash_'):
return s[len('hash_'):].replace('.eg', '_tmpl')
elif s.startswith('ordered_'):
return s[len('ordered_'):].replace('.eg', '_tmpl')
munge('hash_any_not_null_agg.eg.go') => 'any_not_null_agg_tmpl.go'
Maybe something about Bazel is keeping you from doing this -- if so just let me know :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can give a try, actually I was just following the same logic applied before in older commits but I can try my luck with that change you are suggesting and see what's happening =)
Also, do you think is necessary to create a whole new function to only get the tmpl
target name, for example this one: ('ordered_sum_int_agg.eg.go', 'sum_agg_tmpl.go')
on that one I would need to create an specific elif for checking that case?
I think that it would make it a bit complex for something pretty straight forward (only gets names for the tmpl
file) but I might be wrong and maybe it would make it easier to understand.
Fixes cockroachdb#58421. For the go dependency changes, we've applied the same treatment as cockroachdb@52c5f51b81. Release note: None Co-authored-by: irfan sharif <[email protected]>
9e3c47d
to
4e4cbd2
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the PR, LGTM. I've just amended the commit message and some comment slightly.
bors r+ |
Build succeeded: |
Fixes #58421. For the go dependency changes, we've applied the same
treatment as 52c5f51.
Release note: None