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

fix: quote column name if db requires #15465

Merged
merged 2 commits into from Jul 2, 2021

Conversation

eschutho
Copy link
Member

SUMMARY

We were seeing some issues when using Snowflake with column aliases in quotes. When creating the dataset, the outer select statement was quoting the alias/label but not the column name. Snowflake in particular requires that if the original query column name is quoted then any reference to it must also be quoted.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

Before with quotes (snowflake)
Screenshot_6_29_21__2_17_PM

Before without quotes (snowflake):
Screenshot_6_29_21__2_22_PM

After with quotes (snowflake):
Screenshot_6_29_21__12_52_PM
Screenshot_6_29_21__12_53_PM

After without quotes (snowflake):
Screenshot_6_29_21__12_54_PM
_DEV__Explore_-_snowflake_with_quotes

TESTING INSTRUCTIONS

ADDITIONAL INFORMATION

  • Has associated issue:
  • Changes UI
  • Includes DB Migration (follow approval process in SIP-59)
    • Migration is atomic, supports rollback & is backwards-compatible
    • Confirm DB migration upgrade and downgrade tested
    • Runtime estimates and downtime expectations provided
  • Introduces new feature or API
  • Removes existing feature or API

@eschutho
Copy link
Member Author

These changes will also affect db2, hana, and oracle which I haven't tested. I'd be grateful if someone could test another db that has access to one or more of them.

@eschutho eschutho changed the title quote column name if db requires fix: quote column name if db requires Jun 29, 2021
@eschutho eschutho force-pushed the elizabeth/fix-column-name-quotes branch from 8093198 to 629d5dc Compare June 29, 2021 23:55
@codecov
Copy link

codecov bot commented Jun 30, 2021

Codecov Report

Merging #15465 (76e3906) into master (e5ab9a4) will decrease coverage by 0.04%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #15465      +/-   ##
==========================================
- Coverage   76.97%   76.92%   -0.05%     
==========================================
  Files         976      974       -2     
  Lines       51260    50511     -749     
  Branches     6900     6494     -406     
==========================================
- Hits        39456    38857     -599     
+ Misses      11585    11449     -136     
+ Partials      219      205      -14     
Flag Coverage Δ
hive ?
mysql 81.62% <100.00%> (+<0.01%) ⬆️
postgres 81.64% <100.00%> (+<0.01%) ⬆️
presto ?
python 81.72% <100.00%> (-0.45%) ⬇️
sqlite 81.25% <100.00%> (+<0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
superset/connectors/sqla/models.py 88.24% <100.00%> (-1.63%) ⬇️
...d/src/filters/components/Time/TimeFilterPlugin.tsx 0.00% <0.00%> (-92.31%) ⬇️
superset/db_engines/hive.py 0.00% <0.00%> (-82.15%) ⬇️
...tend/src/filters/components/Time/transformProps.ts 22.22% <0.00%> (-44.45%) ⬇️
...d/src/dashboard/components/FiltersBadge/Styles.tsx 54.05% <0.00%> (-40.82%) ⬇️
...uperset-frontend/src/components/Form/FormLabel.tsx 64.70% <0.00%> (-35.30%) ⬇️
superset/db_engine_specs/hive.py 69.20% <0.00%> (-17.21%) ⬇️
.../database/DatabaseModal/DatabaseConnectionForm.tsx 48.48% <0.00%> (-14.62%) ⬇️
...nd/src/components/SupersetResourceSelect/index.tsx 70.96% <0.00%> (-12.91%) ⬇️
...rset-frontend/src/filters/components/Time/index.ts 88.88% <0.00%> (-11.12%) ⬇️
... and 126 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e5ab9a4...76e3906. Read the comment docs.

@eschutho eschutho force-pushed the elizabeth/fix-column-name-quotes branch from 629d5dc to a9c6b92 Compare June 30, 2021 00:17
@eschutho eschutho force-pushed the elizabeth/fix-column-name-quotes branch from a9c6b92 to b6517d8 Compare June 30, 2021 00:26
Copy link
Member

@betodealmeida betodealmeida left a comment

Choose a reason for hiding this comment

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

Looks great! I pinged #debug-together asking for people who might be able to test in the other DBs, but I wouldn't hold my breath.

Copy link
Member

@villebro villebro left a comment

Choose a reason for hiding this comment

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

@eschutho did you make sure this also works with physical tables? If memory serves me well, I believe this problem only currently affects virtual tables, and when I tried to fix the virtual table problem, it broke physical tables. So just to be sure, it would be nice to test that this also works on physical tables with all lowercase, all uppercase and mixed case column names.

@yousoph
Copy link
Member

yousoph commented Jul 1, 2021

We were able to repro on a physical table before this fix. I did some additional testing of this with physical and virtual tables, and lower/upper/mixed case column aliases, it was looking good in all those cases :)

@hughhhh hughhhh merged commit 80b8df0 into apache:master Jul 2, 2021
@eschutho eschutho deleted the elizabeth/fix-column-name-quotes branch July 17, 2021 03:10
eschutho added a commit to preset-io/superset that referenced this pull request Jul 17, 2021
eschutho added a commit that referenced this pull request Jul 19, 2021
henryyeh pushed a commit to preset-io/superset that referenced this pull request Jul 19, 2021
cccs-RyanS pushed a commit to CybercentreCanada/superset that referenced this pull request Dec 17, 2021
Co-authored-by: hughhhh <hughmil3s@gmail.com>
cccs-RyanS pushed a commit to CybercentreCanada/superset that referenced this pull request Dec 17, 2021
QAlexBall pushed a commit to QAlexBall/superset that referenced this pull request Dec 29, 2021
Co-authored-by: hughhhh <hughmil3s@gmail.com>
QAlexBall pushed a commit to QAlexBall/superset that referenced this pull request Dec 29, 2021
cccs-rc pushed a commit to CybercentreCanada/superset that referenced this pull request Mar 6, 2024
Co-authored-by: hughhhh <hughmil3s@gmail.com>
cccs-rc pushed a commit to CybercentreCanada/superset that referenced this pull request Mar 6, 2024
@mistercrunch mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 1.3.0 labels Mar 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels preset-io size/M 🚢 1.3.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants