villebro opened a new pull request, #24471:
URL: https://github.com/apache/superset/pull/24471

   ### SUMMARY
   
   Currently all databases store physical datasets with normalized column 
names, and virtual datasets with denormalized column names. This is problematic 
for Oracle-like databases where the normalized name is typically all lowercase, 
and the denormalized one is ALL UPPERCASE. This causes issues when using native 
filters on dashboards, as the column references will be different, depending on 
whether or not the chart is built on a physical or virtual dataset.
   
   To fix this, this PR proposes calling `dialect.denormalize_name()` on 
dialects that have set the `requires_name_normalize` property to `True`. This 
ensures that databases like Oracle and Snowflake will have the same case for 
both physical and virtual datasets.
   
   We add a unit test to verify that Oracle changes the case correctly, and 
that MSSQL doesn't do any conversion. We don't add a Snowflake test, as our 
current testing requirements doesn't include the Snowflake driver (I don't want 
to bloat our testing dependencies).
   
   ### AFTER
   After the change, Snowflake will denormalize column names, typically 
resulting in ALL UPPERCASE:
   
![image](https://github.com/apache/superset/assets/33317356/97690263-7752-4391-b11e-91e600a87b3e)
   
   ### BEFORE
   Before physical datasets would usually store the column names as all 
lowercase on Snowflake:
   
![image](https://github.com/apache/superset/assets/33317356/a25ca91a-2415-4f88-b6d9-a93fd607fb40)
   
   ### TESTING INSTRUCTIONS
   1. Create a physical dataset on Snowflake
   2. Create a `select * from ...` virtual dataset referencing the same table
   3. Notice how one has all lowercase, and the other ALL UPPERCASE.
   
   ### ADDITIONAL INFORMATION
   <!--- Check any relevant boxes with "x" -->
   <!--- HINT: Include "Fixes #nnn" if you are fixing an existing issue -->
   - [x] Has associated issue: Fixes #18085
   - [ ] Required feature flags:
   - [ ] Changes UI
   - [ ] Includes DB Migration (follow approval process in 
[SIP-59](https://github.com/apache/superset/issues/13351))
     - [ ] 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
   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: [email protected]

For queries about this service, please contact Infrastructure at:
[email protected]


---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]

Reply via email to