adapter: support specifying column comments for builtin objects#32003
Merged
petrosagg merged 8 commits intoMaterializeInc:mainfrom Mar 28, 2025
Merged
adapter: support specifying column comments for builtin objects#32003petrosagg merged 8 commits intoMaterializeInc:mainfrom
petrosagg merged 8 commits intoMaterializeInc:mainfrom
Conversation
petrosagg
force-pushed
the
support-comments-on-builtins
branch
2 times, most recently
from
874e4ce to
2ec2a69
Compare
ParkMyCar
approved these changes
Mar 26, 2025
Contributor
ParkMyCar
left a comment
ParkMyCar
left a comment
There was a problem hiding this comment.
Thanks for whipping this up!
src/catalog/src/builtin.rs
Outdated
Comment on lines
2183
to
2237
| column_comments: Some(vec![ | ||
| "The ID of a compute object. Corresponds to `mz_catalog.mz_indexes.id`, \ | ||
| `mz_catalog.mz_materialized_views.id`, or `mz_internal.mz_subscriptions`", | ||
| "The ID of a compute dependency. Corresponds to `mz_catalog.mz_indexes.id`, \ | ||
| `mz_catalog.mz_materialized_views.id`, `mz_catalog.mz_sources.id`, or `mz_catalog.mz_tables.id`", | ||
| ]), |
Contributor
There was a problem hiding this comment.
I would prefer an API that takes a (column_index, comment) or (column_name, comment) pair, this way you don't have to leave a comment on all columns. But as-is this is a net win, so not blocking!
src/adapter/src/catalog/open.rs
Outdated
| Builtin::View(view) => { | ||
| match view.column_comments { | ||
| Some(ref column_comments) => { | ||
| for (col, &comment) in column_comments.iter().enumerate() { |
Contributor
There was a problem hiding this comment.
Darn, missing the num_columns == num_comments assert! here isn't great, but we don't have a RelationDesc so not much we can do :/
petrosagg
force-pushed
the
support-comments-on-builtins
branch
2 times, most recently
from
7f5be42 to
e19cb07
Compare
Signed-off-by: Petros Angelatos <petrosagg@gmail.com>
petrosagg
force-pushed
the
support-comments-on-builtins
branch
2 times, most recently
from
005d528 to
25590bd
Compare
petrosagg
force-pushed
the
support-comments-on-builtins
branch
2 times, most recently
from
19347ea to
cc08493
Compare
Signed-off-by: Petros Angelatos <petrosagg@gmail.com>
Signed-off-by: Petros Angelatos <petrosagg@gmail.com>
This commit has been automatically generated. Signed-off-by: Petros Angelatos <petrosagg@gmail.com>
Signed-off-by: Petros Angelatos <petrosagg@gmail.com>
This commit has been automatically generated. Signed-off-by: Petros Angelatos <petrosagg@gmail.com>
Signed-off-by: Petros Angelatos <petrosagg@gmail.com>
petrosagg
force-pushed
the
support-comments-on-builtins
branch
from
cc08493 to
6f3f0fe
Compare
Signed-off-by: Petros Angelatos <petrosagg@gmail.com>
petrosagg
force-pushed
the
support-comments-on-builtins
branch
from
6f3f0fe to
f214975
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Motivation
Motivated by a recent slack discussion on the topic.
This PR tidies up some aspects of builtin objects definitions. Specifically, builtin views must now declare their expected schema and a test is added that verifies that the schema produced by planning the view definition corresponds to the declared schema.
Additionally, I have automatically ported all of the column documentation from our docs into SQL column comments. Our documentation includes markdown links in the docs of various fields. I have used a markdown parser to turn them into
psqlfriendly text.The commits that have been automatically generated are marked by
[codegen]and don't need to be reviewed.Tips for reviewer
Checklist
$T ⇔ Proto$Tmapping (possibly in a backwards-incompatible way), then it is tagged with aT-protolabel.