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

allow customizing field names in BQ schema #10

Merged
merged 2 commits into from
Jan 14, 2019

Conversation

jhump
Copy link
Contributor

@jhump jhump commented Nov 7, 2018

No description provided.

@jhump
Copy link
Contributor Author

jhump commented Nov 7, 2018

I am migrating protos to use a different naming convention, but need the BQ schema to remain the same. This provides two ways to customize the field names:

  1. At a message level, have all fields use their JSON name. This is most convenient for me since I also have code that relies on the JSON format. So, even though I need to rename fields, I am preserving their JSON name, so the messages' JSON format is unchanged. (I was previously already relying on the JSON name matching the BQ field name, so this will be perfect for me.)
  2. I've also added another way to customize field names, which I think is most intuitive: a separate field option.

I saw that extension 1026 (table description, which is unused BTW) already had a TODO to register. Per extension best practices, I've added a new extension that is a message -- that way any future extensions can just be added as fields of that message.

In doing this, I went ahead and moved table description into the message and marked the other one as deprecated. But I also noticed that the table description extension isn't even used anywhere... Should it just be removed?

Anyhow, guidance on how to handle the message extension tag numbers would be appreciated.

@jhump
Copy link
Contributor Author

jhump commented Nov 7, 2018

@yugui, is this something you could take a look at? In the meantime, I'll need to maintain a fork of protoc-gen-bq-schema, because I really need to be able to refactor my protos without it mucking with my BQ schema.

main.go Show resolved Hide resolved
@jhump
Copy link
Contributor Author

jhump commented Jan 7, 2019

@yugui, @sgammon, @mdittmer: Is this project still maintained? It's been a while since there were any commits, and no one has taken a look at this. Could someone please give a review? Or, if this project is not being maintained anymore and PRs will not be accepted, please let me know that, too.

@mdittmer
Copy link
Contributor

mdittmer commented Jan 7, 2019

For my part, I contributed a single patch to fix build back when a project of mine depended on this. (Said project no longer does depend on this.) I would not consider myself an "official maintainer" and will let others speak to ongoing maintenance.

@jhump
Copy link
Contributor Author

jhump commented Jan 7, 2019

Sorry if I spammed you, @mdittmer. In the search for someone that might be maintaining the project, I just looked at names of people that had commits on master.

@yugui yugui self-requested a review January 9, 2019 17:00
Copy link
Collaborator

@yugui yugui left a comment

Choose a reason for hiding this comment

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

Thank you for sharing your good use-case.

Unfortunately, as I commented in bq_table.proto, there's a problem which prevents us from adding a new option due to my fault. Do you have any good idea?

bq_table.proto Outdated Show resolved Hide resolved
@jhump
Copy link
Contributor Author

jhump commented Jan 9, 2019

@yugui, I think I've made changes as requested. I don't know the right way to communicate the backwards-incompatible change, so I've just put it as a comment in the proto file.

I also added a test case that uses the old format for the extension, to make sure it can correctly fallback to parsing as a string, the old way. As I noted above, however, this is not a fool-proof solution. And I think it is likely that folks that upgrade the plugin may also end up upgrading their proto files at the same time (so this does little to help users with the compatibility issue).

Let me know what you think.

@yugui yugui merged commit cb190f8 into GoogleCloudPlatform:master Jan 14, 2019
yugui added a commit that referenced this pull request Jan 14, 2019
@yugui
Copy link
Collaborator

yugui commented Jan 14, 2019

Merged.
Thank you for your great contribution.

@maguro
Copy link

maguro commented Aug 5, 2019

I need to make all my BQ fields in the table required. It would be might handy to have something like

option (gen_bq_schema.bigquery_opts).default_require = true;

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.

5 participants