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 broken dedup and remove redundant db_spec logic #5467

Merged
merged 2 commits into from
Jul 23, 2018
Merged

Fix broken dedup and remove redundant db_spec logic #5467

merged 2 commits into from
Jul 23, 2018

Conversation

villebro
Copy link
Member

@villebro villebro commented Jul 23, 2018

Fixes regression introduced by PR #4724 that broke column name deduplication. Also remove column name lowercasing logic from db_engine_spec that was made redundant by PR #5178.

@mistercrunch
Copy link
Member

@villebro I'm trying to understand how #5178 handles lowercasing or removes the need for it

@villebro
Copy link
Member Author

villebro commented Jul 23, 2018

@mistercrunch The lowercasing hack was introduced to deal with SQLA engines that returned different cases for column names in the cursor.description (db api 1) which was used by e.g. SQL Lab and ResultProxy.keys() (db api 2), which was used for retrieving data. #5178 replaced all v2 api calls with v1 calls, ensuring that the column names are always of the same case.

@codecov-io
Copy link

codecov-io commented Jul 23, 2018

Codecov Report

Merging #5467 into master will decrease coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #5467      +/-   ##
==========================================
- Coverage   59.12%   59.11%   -0.01%     
==========================================
  Files         372      372              
  Lines       23762    23751      -11     
  Branches     2758     2758              
==========================================
- Hits        14049    14041       -8     
+ Misses       9698     9695       -3     
  Partials       15       15
Impacted Files Coverage Δ
superset/db_engine_specs.py 53.25% <ø> (-0.34%) ⬇️
superset/dataframe.py 94.49% <100%> (ø) ⬆️

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 971e9f0...b65ec27. Read the comment docs.

@villebro
Copy link
Member Author

To add context, #4994 was the PR that added the lowercasing logic. Without this new fix reverting the hack, I believe Redshift, Oracle and Snowflake will not work via SQL Lab as of #5178 .

@mistercrunch
Copy link
Member

LGTM

@mistercrunch mistercrunch merged commit a165aec into apache:master Jul 23, 2018
timifasubaa pushed a commit to airbnb/superset-fork that referenced this pull request Jul 25, 2018
* Fix broken dedup and remove redundant db_spec logic

* Add test case
wenchma pushed a commit to wenchma/incubator-superset that referenced this pull request Nov 16, 2018
* Fix broken dedup and remove redundant db_spec logic

* Add test case
@mistercrunch mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 0.28.0 labels Feb 27, 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 🚢 0.28.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants