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.

Sign up for GitHub

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

Jump to bottom

show pg and athena column comments and table descriptions as antd tooltip if they are defined #6582

Merged
merged 7 commits into from
Apr 12, 2024
Merged

show pg and athena column comments and table descriptions as antd tooltip if they are defined #6582

merged 7 commits into from
Apr 12, 2024

Conversation

AndrewChubatiuk
Copy link
Contributor

@AndrewChubatiuk AndrewChubatiuk commented Nov 7, 2023 •
edited
Loading

What type of PR is this?

  • Feature

Description

Use for PostgreSQL and Athena column comments to be shown instead of column name, when it's defined

How is this tested?

Running it on our Redash fork in production for a long time
Screenshot 2024-04-11 at 23 45 21

@guidopetri
Copy link
Contributor

@AndrewChubatiuk thanks for the PR. Do you have any screenshots handy to show before/after this change?

@guidopetri guidopetri self-assigned this Nov 8, 2023
@AndrewChubatiuk AndrewChubatiuk force-pushed the added-columns-comments branch 2 times, most recently from 7708078 to 3bb3e06 Compare April 10, 2024 10:21
@codecov Codecov
Copy link

codecov bot commented Apr 10, 2024 •
edited
Loading

Codecov Report

Attention: Patch coverage is 54.54545% with 10 lines in your changes are missing coverage. Please review.

Project coverage is 63.79%. Comparing base (af0773c) to head (aaf81fd).
Report is 14 commits behind head on master.

❗ Current head aaf81fd differs from pull request most recent head 30abe0e. Consider uploading reports for the commit 30abe0e to get more accurate results

Additional details and impacted files 
@@            Coverage Diff             @@
##           master    #6582      +/-   ##
==========================================
- Coverage   63.82%   63.79%   -0.03%     
==========================================
  Files         161      161              
  Lines       13060    13077      +17     
  Branches     1803     1807       +4     
==========================================
+ Hits         8335     8343       +8     
- Misses       4425     4430       +5     
- Partials      300      304       +4
Files Coverage Δ
redash/models/__init__.py 91.95% <ø> (ø)
redash/query_runner/pg.py 42.13% <0.00%> (-0.44%) ⬇️
redash/query_runner/athena.py 51.49% <60.00%> (+0.23%) ⬆️

@AndrewChubatiuk AndrewChubatiuk force-pushed the added-columns-comments branch 2 times, most recently from aaf81fd to 717b02c Compare April 11, 2024 05:37
@AndrewChubatiuk AndrewChubatiuk changed the title show column comments by default for athena and postgres if they are defined show pg and athena column comments and table descriptions as antd tooltip if they are defined Apr 11, 2024
@AndrewChubatiuk
Copy link
Contributor Author

@guidopetri @justinclift added image

@AndrewChubatiuk AndrewChubatiuk force-pushed the added-columns-comments branch from 717b02c to 415da86 Compare April 12, 2024 08:11
@justinclift
Copy link
Member

@gaecoli @junnplus Would either of you have time and interest to review this one? 😄

gaecoli
gaecoli approved these changes Apr 12, 2024
View reviewed changes
Copy link
Member

@gaecoli gaecoli left a comment

Choose a reason for hiding this comment

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

LGTM!

@justinclift
Copy link
Member

@gaecoli Thanks heaps. 😄

@AndrewChubatiuk AndrewChubatiuk force-pushed the added-columns-comments branch from d51e01a to 30abe0e Compare April 12, 2024 10:35
@AndrewChubatiuk
Copy link
Contributor Author

@justinclift had to rebase it due to concurrent PR that was merged faster

justinclift
justinclift approved these changes Apr 12, 2024
View reviewed changes
Copy link
Member

@justinclift justinclift left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Seems unchanged from when @gaecoli approved this 2 hours ago. 😄

@justinclift justinclift merged commit c12d450 into getredash:master Apr 12, 2024
11 checks passed
@justinclift
Copy link
Member

Hopefully the CI on master doesn't try running the Percy check, now that I've removed the PERCY_TOKEN secret.

@justinclift
Copy link
Member

Yep, looks like that worked. CI on master is now reporting pass/fail correctly. 😄

@justinclift
Copy link
Member

@AndrewChubatiuk Thinking over this PR, it looks like it'll be useful for people. Well done. 😄

@eradman
Copy link
Collaborator

eradman commented Apr 12, 2024

@justinclift, @gaecoli: features and improvements should not be merged until we have a working build and tests
(See #6876)

@justinclift
Copy link
Member

The build and tests appear to be working now. 😄

@AndrewChubatiuk AndrewChubatiuk deleted the added-columns-comments branch April 13, 2024 15:50
eradman added a commit to eradman/redash that referenced this pull request May 15, 2024
@eradman
Revert "show pg and athena column comments and table descriptions as …
a564fb1 
…antd tooltip if they are defined (getredash#6582)"

This reverts commit c12d450.

This commit did not sort tables properly by schema, then name
@justinclift justinclift mentioned this pull request May 15, 2024
Merged
4 tasks
gaecoli pushed a commit that referenced this pull request May 16, 2024
@eradman
Revert "show pg and athena column comments and table descriptions as …
10a46fd 
…antd tooltip if they are defined (#6582)" (#6971)

This reverts commit c12d450.

This commit did not sort tables properly by schema, then name
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gaecoli gaecoli gaecoli approved these changes

@justinclift justinclift justinclift approved these changes

Assignees

@guidopetri guidopetri

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@AndrewChubatiuk @guidopetri @justinclift @eradman @gaecoli @restyled-commits