Force quoted column aliases for Oracle-like databases #5686

villebro · 2018-08-21T16:37:55Z

Oracle-like databases return uppercased column names for queries with unquoted aliases in the select statement. To deal with this issue column names in query results were previously either forcefully lowercased (dbapi v2) or uppercased (dbapi v1). As this proved unsustainable a new solution is proposed in this PR. The logic is as follows:

a new attribute force_column_alias_quotes is introduced in db_engine_spec. The default value for this is False, and is overridden to True for Snowflake, Oracle, Redshift and DB2, which are generally known to exhibit Oracle-like case conversion behaviour.
For affected databases, column names during query construction (NOT table metadata gathering/refreshing) are given a label that is wrapped in the SQLAlchemy wrapper quoted_name, which forces quotes around the alias.

Before:

After:

In practice this forces the query result to have the same case as the column name in the metadata, circumventing problems caused by changing case. While forced quoting shouldn't cause adverse results in other databases, behaviour for other databases remains unchanged.

mistercrunch · 2018-08-21T17:07:31Z

I feel like all of this is getting pretty messy and I'm starting to think we need a higher level refactor. Instead of mutating dataframe columns names, I feel like we should target SQLAlchemy labels (aliases) (upstream).

This would mean that everywhere where we create a SQLA label we'd send it through a db_engine_spec normalize_sqla_label method. This also means that we need to also systematically do this every time we access a column in a dataframe.

Now for the Druid connector there's no db_engine_spec as it's a SQLA construct. Maybe BaseViz can register a self.normalize_label() that points to db_engine_spec.normalize_sqla_label for the sqla connector, and points to a no-op for the druid connector.

What do you think?

villebro · 2018-08-21T18:29:47Z

TBH the initial fix did feel slightly wonky, and now that you mention it I'm starting to view this whole ordeal in a new light. You know what they say about the proverbial frog in boiling water..

It is my understanding that the currently identified problematic SQLA databases/connectors (Snowflake, Redshift, Oracle) share the following characteristics:

cursor.description returns all lowercase column names.
Executing a select via the cursor with lowercase column names (or mixed/upper case for that matter) results in upper case column labels in the query result.

While it can't be excluded that some engine down the line will exhibit some other behaviour, I suspect having a normalizing function when creating a tables that uppercases column names when returned from a cursor.description should solve most of the observed problems. In addition, metrics would probably need to be passed through the same normalizer, but beyond that one might not need to do any further wrangling (famous last words).

I think I'll give it a shot and see where I land.

mistercrunch · 2018-08-21T18:36:38Z

You can add BigQuery to the list. It has issues with non alphanumeric chars as in SELECT sum(x) AS "sum(x)" and where length > 128.

Given that there are issues in this area we need to deal with it in a failure proof way.

codecov-io · 2018-08-22T17:20:13Z

Codecov Report

❗ No coverage uploaded for pull request base (master@cae0704). Click here to learn what that means.
The diff coverage is 71.66%.

@@            Coverage Diff            @@
##             master    #5686   +/-   ##
=========================================
  Coverage          ?   63.43%           
=========================================
  Files             ?      361           
  Lines             ?    22988           
  Branches          ?     2559           
=========================================
  Hits              ?    14582           
  Misses            ?     8391           
  Partials          ?       15

Impacted Files	Coverage Δ
superset/viz.py	`81.32% <ø> (ø)`
superset/dataframe.py	`94.87% <100%> (ø)`
superset/connectors/sqla/models.py	`78.86% <68%> (ø)`
superset/db_engine_specs.py	`55.34% <88.88%> (ø)`

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 cae0704...0d18163. Read the comment docs.

villebro · 2018-08-26T11:26:50Z

@mistercrunch This PR was rewritten from scratch based on your comments and is now ready for review. I have updated the description above to describe the logic. In short this PR forces quotes around column aliases using native SQLAlchemy constructs for affected databases, which aligns the column names in the query and result. I was able to contain all oracle-like logic within db_engine_specs.py and connectors/sqla/models.py, i.e. there is no longer any need for dataframe column renaming downstream e.g. viz.py.

FYI @HectorLira @minh5 @mmuru: This PR attempts (yet again) to improve stability for Redshift, Oracle, Snowflake (beware, you must use snowflake-sqlalchemy==1.1.0). Any testing help to verify whether or not it works would be valuable, especially Redhshift, as this was mainly validated against Snowflake during development.

* Replace dataframe label override logic with table column override * Add mutation to any_date_col * Linting * Add mutation to oracle and redshift * Fine tune how and which labels are mutated * Implement alias quoting logic for oracle-like databases * Fix and align column and metric sqla_col methods * Clean up typos and redundant logic * Move new attribute to old location * Linting * Replace old sqla_col property references with function calls * Remove redundant calls to mutate_column_label * Move duplicated logic to common function * Add db_engine_specs to all sqla_col calls * Add missing mydb * Add note about snowflake-sqlalchemy regression * Make db_engine_spec mandatory in sqla_col * Small refactoring and cleanup * Remove db_engine_spec from get_from_clause call * Make db_engine_spec mandatory in adhoc_metric_to_sa * Remove redundant mutate_expression_label call * Add missing db_engine_specs to adhoc_metric_to_sa * Rename arg label_name to label in get_column_label() * Rename label function and add docstring * Remove redundant db_engine_spec args * Rename col_label to label * Remove get_column_name wrapper and make direct calls to db_engine_spec * Remove unneeded db_engine_specs * Rename sa_ vars to sqla_ (cherry picked from commit 77fe9ef)

* Replace dataframe label override logic with table column override * Add mutation to any_date_col * Linting * Add mutation to oracle and redshift * Fine tune how and which labels are mutated * Implement alias quoting logic for oracle-like databases * Fix and align column and metric sqla_col methods * Clean up typos and redundant logic * Move new attribute to old location * Linting * Replace old sqla_col property references with function calls * Remove redundant calls to mutate_column_label * Move duplicated logic to common function * Add db_engine_specs to all sqla_col calls * Add missing mydb * Add note about snowflake-sqlalchemy regression * Make db_engine_spec mandatory in sqla_col * Small refactoring and cleanup * Remove db_engine_spec from get_from_clause call * Make db_engine_spec mandatory in adhoc_metric_to_sa * Remove redundant mutate_expression_label call * Add missing db_engine_specs to adhoc_metric_to_sa * Rename arg label_name to label in get_column_label() * Rename label function and add docstring * Remove redundant db_engine_spec args * Rename col_label to label * Remove get_column_name wrapper and make direct calls to db_engine_spec * Remove unneeded db_engine_specs * Rename sa_ vars to sqla_

* Replace dataframe label override logic with table column override * Add mutation to any_date_col * Linting * Add mutation to oracle and redshift * Fine tune how and which labels are mutated * Implement alias quoting logic for oracle-like databases * Fix and align column and metric sqla_col methods * Clean up typos and redundant logic * Move new attribute to old location * Linting * Replace old sqla_col property references with function calls * Remove redundant calls to mutate_column_label * Move duplicated logic to common function * Add db_engine_specs to all sqla_col calls * Add missing mydb * Add note about snowflake-sqlalchemy regression * Make db_engine_spec mandatory in sqla_col * Small refactoring and cleanup * Remove db_engine_spec from get_from_clause call * Make db_engine_spec mandatory in adhoc_metric_to_sa * Remove redundant mutate_expression_label call * Add missing db_engine_specs to adhoc_metric_to_sa * Rename arg label_name to label in get_column_label() * Rename label function and add docstring * Remove redundant db_engine_spec args * Rename col_label to label * Remove get_column_name wrapper and make direct calls to db_engine_spec * Remove unneeded db_engine_specs * Rename sa_ vars to sqla_ (cherry picked from commit 77fe9ef)

* Replace dataframe label override logic with table column override * Add mutation to any_date_col * Linting * Add mutation to oracle and redshift * Fine tune how and which labels are mutated * Implement alias quoting logic for oracle-like databases * Fix and align column and metric sqla_col methods * Clean up typos and redundant logic * Move new attribute to old location * Linting * Replace old sqla_col property references with function calls * Remove redundant calls to mutate_column_label * Move duplicated logic to common function * Add db_engine_specs to all sqla_col calls * Add missing mydb * Add note about snowflake-sqlalchemy regression * Make db_engine_spec mandatory in sqla_col * Small refactoring and cleanup * Remove db_engine_spec from get_from_clause call * Make db_engine_spec mandatory in adhoc_metric_to_sa * Remove redundant mutate_expression_label call * Add missing db_engine_specs to adhoc_metric_to_sa * Rename arg label_name to label in get_column_label() * Rename label function and add docstring * Remove redundant db_engine_spec args * Rename col_label to label * Remove get_column_name wrapper and make direct calls to db_engine_spec * Remove unneeded db_engine_specs * Rename sa_ vars to sqla_

villebro changed the title ~~[bugfix] Improve column name case alignment logic~~ [WIP] Improve column name case alignment logic Aug 21, 2018

villebro added 17 commits August 26, 2018 00:11

Replace dataframe label override logic with table column override

d5242d0

Add mutation to any_date_col

ead1b55

Linting

4540d11

Add mutation to oracle and redshift

027f794

Fine tune how and which labels are mutated

f8f851a

Implement alias quoting logic for oracle-like databases

ce3617b

Fix and align column and metric sqla_col methods

5f67d59

Clean up typos and redundant logic

8b6e52d

Move new attribute to old location

62163fa

Linting

e8dd7da

Replace old sqla_col property references with function calls

6c6436f

Remove redundant calls to mutate_column_label

4b0e593

Move duplicated logic to common function

76959dd

Add db_engine_specs to all sqla_col calls

6c7fccc

Add missing mydb

c187104

Add note about snowflake-sqlalchemy regression

53b77a2

Make db_engine_spec mandatory in sqla_col

e792d15

villebro changed the title ~~[WIP] Improve column name case alignment logic~~ [WIP] Force quoted column aliases for Oracle-like databases Aug 26, 2018

villebro added 5 commits August 26, 2018 11:25

Small refactoring and cleanup

44eac83

Remove db_engine_spec from get_from_clause call

a161efc

Make db_engine_spec mandatory in adhoc_metric_to_sa

901b635

Remove redundant mutate_expression_label call

7e7a2a8

Add missing db_engine_specs to adhoc_metric_to_sa

d60ed34

villebro changed the title ~~[WIP] Force quoted column aliases for Oracle-like databases~~ Force quoted column aliases for Oracle-like databases Aug 26, 2018

simond mentioned this pull request Nov 7, 2018

Recent Refactor of db_engine_specs.py causes KeyError again on Redshift sources. #6162

Closed

3 tasks

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Force quoted column aliases for Oracle-like databases #5686

Force quoted column aliases for Oracle-like databases #5686

villebro commented Aug 21, 2018 •

edited

Loading

mistercrunch commented Aug 21, 2018

villebro commented Aug 21, 2018

mistercrunch commented Aug 21, 2018

codecov-io commented Aug 22, 2018 •

edited

Loading

villebro commented Aug 26, 2018

Force quoted column aliases for Oracle-like databases #5686

Force quoted column aliases for Oracle-like databases #5686

Conversation

villebro commented Aug 21, 2018 • edited Loading

mistercrunch commented Aug 21, 2018

villebro commented Aug 21, 2018

mistercrunch commented Aug 21, 2018

codecov-io commented Aug 22, 2018 • edited Loading

Codecov Report

villebro commented Aug 26, 2018

villebro commented Aug 21, 2018 •

edited

Loading

codecov-io commented Aug 22, 2018 •

edited

Loading