-
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
sql: Refactoring structured.proto. Renaming ColumnType Kind enum to SemanticType. #16606
Conversation
Oh my. A dream come true. @arjunravinarayan come check this out! Masha:
Reviewed 29 of 29 files at r1. Comments from Reviewable |
Wonderful! Will give a thorough review shortly, but a quick note that "Closes #10705" would be useful in the commit message. |
@knz, what is the "other PR"? |
Raphael:
Comments from Reviewable |
Arjun, added the issue to the commit message. Comments from Reviewable |
@arjunravinarayan the renaming discussion started in #16548 (comment) |
@m-schneider you need to write literally "Fixes #10705." or "Closes #10705." on a line by itself in either the PR description or the commit message for Github to pick it up when you'll merge this. |
Reviewed 6 of 6 files at r2. Comments from Reviewable |
Reviewed 28 of 29 files at r1, 6 of 6 files at r2. Comments from Reviewable |
…emanticType. The current use of the the word kind to denote type, in the ColunmType message is confusing. The upcoming changes to intoduce new enums to seperate semantic types from visible types will only make this worse. Closes cockroachdb#10705
@knz, didn't know that thanks for the tip! Review status: all files reviewed at latest revision, all discussions resolved, some commit checks pending. Comments from Reviewable |
The current use of the the word kind to denote type, in the ColunmType message
is confusing. The upcoming changes to intoduce new enums to seperate semantic
types from visible types will only make this worse.
Closes #10705