-
Notifications
You must be signed in to change notification settings - Fork 13.5k
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(snowflake): opt-in denormalization of column names #24982
fix(snowflake): opt-in denormalization of column names #24982
Conversation
254921a
to
1fa6d86
Compare
87fed4b
to
04575d3
Compare
04575d3
to
c7a7656
Compare
c7a7656
to
a6346db
Compare
def upgrade(): | ||
op.add_column( | ||
"tables", | ||
sa.Column("normalize_columns", sa.Boolean(), nullable=True, default=False), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's the difference between NULL
and FALSE
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@john-bodley they're essentially the same. Would you prefer I change it to just nullable=True
without a default value, or just have the default value?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we only need two states, then I would stick with TRUE
and FALSE
, i.e., non-nullable, unless there's a performance or storage cost for using FALSE
rather than NULL
—which likely will be the predominant value.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems SQLAlchemy is slightly flaky when it comes to assigning default values with the NULL constraint in place. I dug around, and found that the is_sqllab_viz
flag on the SqlaTable
model is expected to work similarly, and there the migration also had to allow for nullable=True
. So reverting back to that.
4f5c21e
to
232f9b4
Compare
@@ -105,6 +105,7 @@ def test_external_metadata_by_name_for_physical_table(self): | |||
"database_name": tbl.database.database_name, | |||
"schema_name": tbl.schema, | |||
"table_name": tbl.table_name, | |||
"normalize_columns": tbl.normalize_columns, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we add some tests that create a dataset with/without this value to check for api backwards compatibility?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good idea, I'll do that 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1
2765dca
to
d3d8655
Compare
d3d8655
to
ddee4b3
Compare
1a715e6
to
fc593d2
Compare
fc593d2
to
114e867
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Thank you for the fix @villebro!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great!
(cherry picked from commit f94dc49)
SUMMARY
The PR #24471, which meant to harmonize column naming for Oracle-like databases like Snowflake, caused issues for deployments that were relying on the current behavior of normalizing column names for physical datasets. This PR changes adds a field
normalize_columns
to the Dataset/SQLA Table models. This defaults toFalse
for new datasets, but for old datsets, this is set toTrue
via a db migration to ensure we don't break existing datasets.For existing datasets, "Normalize columns" is checked:
![image](https://private-user-images.githubusercontent.com/33317356/260529196-02dab6d0-81d9-4639-adc2-60115f270a1e.png?jwt=eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJpc3MiOiJnaXRodWIuY29tIiwiYXVkIjoicmF3LmdpdGh1YnVzZXJjb250ZW50LmNvbSIsImtleSI6ImtleTUiLCJleHAiOjE3MjU5ODgzODcsIm5iZiI6MTcyNTk4ODA4NywicGF0aCI6Ii8zMzMxNzM1Ni8yNjA1MjkxOTYtMDJkYWI2ZDAtODFkOS00NjM5LWFkYzItNjAxMTVmMjcwYTFlLnBuZz9YLUFtei1BbGdvcml0aG09QVdTNC1ITUFDLVNIQTI1NiZYLUFtei1DcmVkZW50aWFsPUFLSUFWQ09EWUxTQTUzUFFLNFpBJTJGMjAyNDA5MTAlMkZ1cy1lYXN0LTElMkZzMyUyRmF3czRfcmVxdWVzdCZYLUFtei1EYXRlPTIwMjQwOTEwVDE3MDgwN1omWC1BbXotRXhwaXJlcz0zMDAmWC1BbXotU2lnbmF0dXJlPWRmZGNmOWQ1ODE3NGNjMDdlY2RmNmZkMjQ0YjVlMTZmZDhiZjdiNmEyMjExMzY5YTU3MzkzZGJlZjhmY2Q3YjQmWC1BbXotU2lnbmVkSGVhZGVycz1ob3N0JmFjdG9yX2lkPTAma2V5X2lkPTAmcmVwb19pZD0wIn0.tslLesSRV5M9HU1gPG770rf5DoNu3jVMNBeMAaiX-5s)
![image](https://private-user-images.githubusercontent.com/33317356/260799805-f54d5e83-ae26-45b7-9d3e-6a793ff33f79.png?jwt=eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJpc3MiOiJnaXRodWIuY29tIiwiYXVkIjoicmF3LmdpdGh1YnVzZXJjb250ZW50LmNvbSIsImtleSI6ImtleTUiLCJleHAiOjE3MjU5ODgzODcsIm5iZiI6MTcyNTk4ODA4NywicGF0aCI6Ii8zMzMxNzM1Ni8yNjA3OTk4MDUtZjU0ZDVlODMtYWUyNi00NWI3LTlkM2UtNmE3OTNmZjMzZjc5LnBuZz9YLUFtei1BbGdvcml0aG09QVdTNC1ITUFDLVNIQTI1NiZYLUFtei1DcmVkZW50aWFsPUFLSUFWQ09EWUxTQTUzUFFLNFpBJTJGMjAyNDA5MTAlMkZ1cy1lYXN0LTElMkZzMyUyRmF3czRfcmVxdWVzdCZYLUFtei1EYXRlPTIwMjQwOTEwVDE3MDgwN1omWC1BbXotRXhwaXJlcz0zMDAmWC1BbXotU2lnbmF0dXJlPTM5ZjhkNzEwZjgzZGUwNzA4ZjQ1NzQ3OTI0NDA5MWRkYzk0NjE3MWYyZmM5MThiZDAyNjJmNTIxNGZmOGFiZDcmWC1BbXotU2lnbmVkSGVhZGVycz1ob3N0JmFjdG9yX2lkPTAma2V5X2lkPTAmcmVwb19pZD0wIn0.PFECqXKJhRoMynWNlNmrlklyr1u4Dj9c3MYrX3zu81Q)
When checked, the behavior is consistent with how it was previously, i.e. physical datasets on Snowflake have normalized column names:
For new datasets, the checkbox is unchecked:
![image](https://private-user-images.githubusercontent.com/33317356/260532664-80a66d3e-a19b-4d45-a368-1767b3602754.png?jwt=eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJpc3MiOiJnaXRodWIuY29tIiwiYXVkIjoicmF3LmdpdGh1YnVzZXJjb250ZW50LmNvbSIsImtleSI6ImtleTUiLCJleHAiOjE3MjU5ODgzODcsIm5iZiI6MTcyNTk4ODA4NywicGF0aCI6Ii8zMzMxNzM1Ni8yNjA1MzI2NjQtODBhNjZkM2UtYTE5Yi00ZDQ1LWEzNjgtMTc2N2IzNjAyNzU0LnBuZz9YLUFtei1BbGdvcml0aG09QVdTNC1ITUFDLVNIQTI1NiZYLUFtei1DcmVkZW50aWFsPUFLSUFWQ09EWUxTQTUzUFFLNFpBJTJGMjAyNDA5MTAlMkZ1cy1lYXN0LTElMkZzMyUyRmF3czRfcmVxdWVzdCZYLUFtei1EYXRlPTIwMjQwOTEwVDE3MDgwN1omWC1BbXotRXhwaXJlcz0zMDAmWC1BbXotU2lnbmF0dXJlPTJjZjU1ZTBiMDJlNGY5YWQ1NjcwMWY5MmU1YTI3MTRlNzkwMjQ0YzQzNmM1OThjNzBkNzBmODBjNmYwY2JhZjYmWC1BbXotU2lnbmVkSGVhZGVycz1ob3N0JmFjdG9yX2lkPTAma2V5X2lkPTAmcmVwb19pZD0wIn0.vkRJ951L3ssYNlRjYqXygu1D_ILIHbLKRnpicj-vsjg)
![image](https://private-user-images.githubusercontent.com/33317356/260799625-da4328be-69f8-4d75-898f-d827c2e193f6.png?jwt=eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJpc3MiOiJnaXRodWIuY29tIiwiYXVkIjoicmF3LmdpdGh1YnVzZXJjb250ZW50LmNvbSIsImtleSI6ImtleTUiLCJleHAiOjE3MjU5ODgzODcsIm5iZiI6MTcyNTk4ODA4NywicGF0aCI6Ii8zMzMxNzM1Ni8yNjA3OTk2MjUtZGE0MzI4YmUtNjlmOC00ZDc1LTg5OGYtZDgyN2MyZTE5M2Y2LnBuZz9YLUFtei1BbGdvcml0aG09QVdTNC1ITUFDLVNIQTI1NiZYLUFtei1DcmVkZW50aWFsPUFLSUFWQ09EWUxTQTUzUFFLNFpBJTJGMjAyNDA5MTAlMkZ1cy1lYXN0LTElMkZzMyUyRmF3czRfcmVxdWVzdCZYLUFtei1EYXRlPTIwMjQwOTEwVDE3MDgwN1omWC1BbXotRXhwaXJlcz0zMDAmWC1BbXotU2lnbmF0dXJlPTJiMDNlMzc1Mzc1OGUzZWU0Mzk0MjBhMTIwODA0NTUwZTRmY2VhZWU0ZTljZmYzMzBmMGM5OGJjOGM0NDNjZTgmWC1BbXotU2lnbmVkSGVhZGVycz1ob3N0JmFjdG9yX2lkPTAma2V5X2lkPTAmcmVwb19pZD0wIn0.jEqFiW94Wc5M7c9NKqNWISYhQG0tjFVr91Nh-H2pcfc)
In this case, a physical dataset on Snowflake will denormalize the columns, usually showing them as UPPERCASE:
This means, that for new datasets, column names are no longer normalized, unless the flag is checked.
TESTING INSTRUCTIONS
ADDITIONAL INFORMATION