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

Support table comment in MongoDB #11424

Merged
merged 1 commit into from
Mar 11, 2022
Merged

Conversation

ebyhr
Copy link
Member

@ebyhr ebyhr commented Mar 11, 2022

Description

Support table comment in MongoDB

Documentation

( ) No documentation is needed.
(x) Sufficient documentation is included in this PR.
( ) Documentation PR is available with #prnumber.
( ) Documentation issue #issuenumber is filed, and can be handled later.

Release notes

( ) No release notes entries required.
(x) Release notes entries required with the following suggested text:

# Section
* Add support for [`COMMENT ON TABLE`](/sql/comment). ({issue}`11424`)

Copy link
Member

@hashhar hashhar left a comment

Choose a reason for hiding this comment

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

LGTM % doc.

@@ -369,6 +369,7 @@ statements, the connector supports the following features:
* :doc:`/sql/alter-table`
* :doc:`/sql/create-schema`
* :doc:`/sql/drop-schema`
* :doc:`/sql/comment` only supports ``COMMENT ON TABLE``
Copy link
Member

Choose a reason for hiding this comment

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

The convention until now has been to add a heading called COMMENT and mention what is not supported.

See ALTER TABLE just below for example.

(I don't have a strong preference but would like consistency).

Copy link
Member Author

Choose a reason for hiding this comment

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

mention what is not supported.

This is fragile for future changes. We have to visit all connectors' page when adding a new option to the statement.

Copy link
Member

Choose a reason for hiding this comment

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

That is a very good point and good justification for only mentioning what is supported.

cc: @jhlodin Maybe you'd want to update existing docs. It's very easy for docs to go out of date since when adding features it's not obvious that I need to search if it has been excluded somewhere. Much easier to explicitly mention what is supported.

Copy link
Contributor

@jhlodin jhlodin Mar 11, 2022

Choose a reason for hiding this comment

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

I agree with the philosophy of saying what's supported rather than unsupported, we can look into standardizing that. From a maintenance perspective though, isn't it still the same work to find applicable connectors and append the new feature to those that now support it? Unless the connector supports all uses of a given statement, we'll still need to explicitly list which ones it does (whether that's framed as inclusive or exclusive).

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm still happy to standardize that, I'm just not sure how that makes this part of the process easier.

Copy link
Member

Choose a reason for hiding this comment

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

Appending things when you add support is logical. Looking for places where you need to "remove" stuff when you added a feature would generally not be on someone's mind unless they are already familiar what exclusions exist.

@@ -369,6 +369,7 @@ statements, the connector supports the following features:
* :doc:`/sql/alter-table`
* :doc:`/sql/create-schema`
* :doc:`/sql/drop-schema`
* :doc:`/sql/comment` only supports ``COMMENT ON TABLE``
Copy link
Member

Choose a reason for hiding this comment

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

That is a very good point and good justification for only mentioning what is supported.

cc: @jhlodin Maybe you'd want to update existing docs. It's very easy for docs to go out of date since when adding features it's not obvious that I need to search if it has been excluded somewhere. Much easier to explicitly mention what is supported.

@ebyhr ebyhr merged commit 271a713 into trinodb:master Mar 11, 2022
@ebyhr ebyhr deleted the ebi/mongodb-comment branch March 11, 2022 11:47
@ebyhr ebyhr mentioned this pull request Mar 11, 2022
@github-actions github-actions bot added this to the 374 milestone Mar 11, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

3 participants