Skip to content

Add support for column comments #306

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

Merged
merged 5 commits into from
Jan 23, 2024

Conversation

cbornet
Copy link

@cbornet cbornet commented Dec 22, 2023

No description provided.

Signed-off-by: Christophe Bornet <[email protected]>
@cbornet
Copy link
Author

cbornet commented Jan 8, 2024

Hi @susodapop, will you have time to review this soon ?

@susodapop
Copy link
Contributor

Yep! Just getting back up to speed following vacation time away.

@cbornet
Copy link
Author

cbornet commented Jan 9, 2024

Awesome, thanks !

Copy link
Contributor

@susodapop susodapop left a comment

Choose a reason for hiding this comment

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

Thanks for this contribution! Looks great. I'm going to push a few test additions. Plus we need an entry in the changelog.

autoincrement=True on a column. See comments in test_suite.py. We may implement implicit
IDENTITY using this feature in the future, similar to the Microsoft SQL Server dialect.
"""
# Emit a log message if a user attempts to set autoincrement=True on a column.
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's keep this as a docstring, since regular comments aren't surfaced in a code editor

Jesse Whitehouse added 2 commits January 23, 2024 14:56
Signed-off-by: Jesse Whitehouse <[email protected]>
@susodapop susodapop changed the base branch from main to sqlalchemy-staging January 23, 2024 20:27
Signed-off-by: Jesse Whitehouse <[email protected]>
@susodapop
Copy link
Contributor

I've changed the base branch for this PR to a sqlalchemy-staging branch so that I can test these changes alongside #308 and update the changelog in one fell swoop.

@susodapop susodapop merged commit ae37007 into databricks:sqlalchemy-staging Jan 23, 2024
@@ -354,6 +354,7 @@ def parse_column_info_from_tgetcolumnsresponse(thrift_resp_row) -> ReflectedColu
"type": final_col_type,
"nullable": bool(thrift_resp_row.NULLABLE),
"default": thrift_resp_row.COLUMN_DEF,
"comment": thrift_resp_row.REMARKS,
Copy link
Contributor

Choose a reason for hiding this comment

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

In order to pass the ComponentReflectionTest we need to update this line to

"comment": thrift_resp_row.REMARKS or None

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.

2 participants