You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by GitBox <gi...@apache.org> on 2020/03/16 23:21:27 UTC
[GitHub] [incubator-superset] villebro opened a new pull request #9316: fix:
remove character set and collate column info by default
villebro opened a new pull request #9316: fix: remove character set and collate column info by default
URL: https://github.com/apache/incubator-superset/pull/9316
### CATEGORY
Choose one
- [x] Bug Fix
- [ ] Enhancement (new features, refinement)
- [x] Refactor
- [x] Add tests
- [ ] Build / Development Environment
- [ ] Documentation
### SUMMARY
It is common for SQL engines to offer the option to define a custom character set and collation scheme for columns. The convention is to define this using the following syntax (example from MySQL): `VARCHAR(255) CHARACTER SET LATIN1 COLLATE UTF8MB4_GENERAL_CI`. A few examples:
- https://dev.mysql.com/doc/refman/8.0/en/charset-column.html
- https://docs.oracle.com/cd/E17952_01/mysql-5.1-en/charset-column.html
Currently Superset already removes collation info, but retains the character set info. This often causes an overflow when saving metadata due to the column type only being 32 characters wide. This PR makes removal of collation and character set info the new default behaviour.
### TEST PLAN
Local test + CI
### ADDITIONAL INFORMATION
- [x] Has associated issue: closes #5018
- [ ] Changes UI
- [ ] Requires DB Migration.
- [ ] Confirm DB Migration upgrade and downgrade tested.
- [ ] Introduces new feature or API
- [ ] Removes existing feature or API
### REVIEWERS
@john-bodley @mistercrunch
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] villebro commented on issue #9316: fix: remove
character set and collate column info by default
Posted by GitBox <gi...@apache.org>.
villebro commented on issue #9316: fix: remove character set and collate column info by default
URL: https://github.com/apache/incubator-superset/pull/9316#issuecomment-600216451
Right now the column type column is `VARCHAR(32)`, aka. they overflow and cause an exception when adding a new table. We could potentially make the column wider, say `VARCHAR(100)`, which would solve the problem, but wouldn't really add any value other than the edge case where someone wants to quickly check the collation in Superset (probably not a common use case). So I guess it comes down to removing the additional info to avoid having to make a metadata migration and keeping the type column slightly lighter in the table editor, or just making it wider and providing the users more context of the column types.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] john-bodley commented on a change in pull
request #9316: fix: remove character set and collate column info by default
Posted by GitBox <gi...@apache.org>.
john-bodley commented on a change in pull request #9316: fix: remove character set and collate column info by default
URL: https://github.com/apache/incubator-superset/pull/9316#discussion_r393914718
##########
File path: superset/db_engine_specs/base.py
##########
@@ -911,13 +912,18 @@ def column_datatype_to_string(
) -> str:
"""
Convert sqlalchemy column type to string representation.
- Can be overridden to remove unnecessary details, especially
- collation info (see mysql, mssql).
+ By default removes collation and character encoding info to avoid unnecessarily
+ long datatypes.
:param sqla_column_type: SqlAlchemy column type
:param dialect: Sqlalchemy dialect
:return: Compiled column type
"""
+ sqla_column_type = sqla_column_type.copy()
+ if hasattr(sqla_column_type, "collation"):
Review comment:
This seems way cleaner that the regex approach. My view is if you're using regex you're probably doing something wrong.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] villebro commented on a change in pull request
#9316: fix: remove character set and collate column info by default
Posted by GitBox <gi...@apache.org>.
villebro commented on a change in pull request #9316: fix: remove character set and collate column info by default
URL: https://github.com/apache/incubator-superset/pull/9316#discussion_r393921041
##########
File path: superset/db_engine_specs/base.py
##########
@@ -911,13 +912,18 @@ def column_datatype_to_string(
) -> str:
"""
Convert sqlalchemy column type to string representation.
- Can be overridden to remove unnecessary details, especially
- collation info (see mysql, mssql).
+ By default removes collation and character encoding info to avoid unnecessarily
+ long datatypes.
:param sqla_column_type: SqlAlchemy column type
:param dialect: Sqlalchemy dialect
:return: Compiled column type
"""
+ sqla_column_type = sqla_column_type.copy()
+ if hasattr(sqla_column_type, "collation"):
Review comment:
I agree, this seems obvious in retrospect 👍
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] john-bodley commented on a change in pull
request #9316: fix: remove character set and collate column info by default
Posted by GitBox <gi...@apache.org>.
john-bodley commented on a change in pull request #9316: fix: remove character set and collate column info by default
URL: https://github.com/apache/incubator-superset/pull/9316#discussion_r393914511
##########
File path: superset/db_engine_specs/base.py
##########
@@ -911,13 +912,18 @@ def column_datatype_to_string(
) -> str:
"""
Convert sqlalchemy column type to string representation.
- Can be overridden to remove unnecessary details, especially
- collation info (see mysql, mssql).
+ By default removes collation and character encoding info to avoid unnecessarily
+ long datatypes.
:param sqla_column_type: SqlAlchemy column type
:param dialect: Sqlalchemy dialect
:return: Compiled column type
"""
+ sqla_column_type = sqla_column_type.copy()
Review comment:
I've never seen this pattern of copying a variable and renaming it to itself, but I guess it works for this purpose.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] john-bodley commented on issue #9316: fix:
remove character set and collate column info by default
Posted by GitBox <gi...@apache.org>.
john-bodley commented on issue #9316: fix: remove character set and collate column info by default
URL: https://github.com/apache/incubator-superset/pull/9316#issuecomment-599817900
@villebro just to clarify you're removing the per column character set in favor of the fallback which is set at the table level?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] villebro merged pull request #9316: fix:
remove character set and collate column info by default
Posted by GitBox <gi...@apache.org>.
villebro merged pull request #9316: fix: remove character set and collate column info by default
URL: https://github.com/apache/incubator-superset/pull/9316
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] villebro edited a comment on issue #9316: fix:
remove character set and collate column info by default
Posted by GitBox <gi...@apache.org>.
villebro edited a comment on issue #9316: fix: remove character set and collate column info by default
URL: https://github.com/apache/incubator-superset/pull/9316#issuecomment-600216451
@john-bodley right now the column type column is `VARCHAR(32)`, aka. they overflow and cause an exception when adding a new table. We could potentially make the column wider, say `VARCHAR(100)`, which would solve the problem, but wouldn't really add any value other than the edge case where someone wants to quickly check the collation in Superset (probably not a common use case). So I guess it comes down to removing the additional info to avoid having to make a metadata migration and keeping the type column slightly lighter in the table editor, or just making it wider and providing the users more context of the column types.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] villebro edited a comment on issue #9316: fix:
remove character set and collate column info by default
Posted by GitBox <gi...@apache.org>.
villebro edited a comment on issue #9316: fix: remove character set and collate column info by default
URL: https://github.com/apache/incubator-superset/pull/9316#issuecomment-600065555
@john-bodley what you're saying makes total sense. The columns from which these strings are created are constructed here:
https://github.com/apache/incubator-superset/blob/85e9a4fa990927327755f5743317e848fc230f01/superset/models/core.py#L577-L586
I'm thinking we should loop through these columns prior to returning the `Table` instance, and remove collation and character set details if present. Do yo agree?
**Edit**: on second thought best to not mutate the original table, probably wise to do in `db_engine_spec.column_datatype_to_string` instead
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] villebro commented on a change in pull request
#9316: fix: remove character set and collate column info by default
Posted by GitBox <gi...@apache.org>.
villebro commented on a change in pull request #9316: fix: remove character set and collate column info by default
URL: https://github.com/apache/incubator-superset/pull/9316#discussion_r393364894
##########
File path: superset/db_engine_specs/base.py
##########
@@ -161,6 +161,7 @@ class BaseEngineSpec: # pylint: disable=too-many-public-methods
utils.DbColumnType.STRING: (
re.compile(r".*CHAR.*", re.IGNORECASE),
re.compile(r".*STRING.*", re.IGNORECASE),
+ re.compile(r".*TEXT.*", re.IGNORECASE),
Review comment:
This is mostly unrelated to this PR, but I noticed that the `TEXT` types were not being identified as string column types (I'm surprised this hadn't come up previously).
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] john-bodley commented on issue #9316: fix:
remove character set and collate column info by default
Posted by GitBox <gi...@apache.org>.
john-bodley commented on issue #9316: fix: remove character set and collate column info by default
URL: https://github.com/apache/incubator-superset/pull/9316#issuecomment-600187501
@villebro just to clarify what is the issue with having these collate and character set defined at the column level?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] john-bodley edited a comment on issue #9316:
fix: remove character set and collate column info by default
Posted by GitBox <gi...@apache.org>.
john-bodley edited a comment on issue #9316: fix: remove character set and collate column info by default
URL: https://github.com/apache/incubator-superset/pull/9316#issuecomment-599817900
@villebro looking at the example data (using a MySQL database),
```
mysql> show create table energy_usage;
+--------------+-----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------+
| Table | Create Table |
+--------------+-----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------+
| energy_usage | CREATE TABLE `energy_usage` (
`source` varchar(255) COLLATE utf8_unicode_ci DEFAULT NULL,
`target` varchar(255) COLLATE utf8_unicode_ci DEFAULT NULL,
`value` float DEFAULT NULL
) ENGINE=InnoDB DEFAULT CHARSET=utf8 COLLATE=utf8_unicode_ci |
+--------------+-----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------+
```
it seems that the column are defined with the `COLLATE` information which is reflected here,
```python
>>> from superset.utils.core import get_example_database
>>> example_db = get_example_database()
>>> sqla_table = example_db.get_table("energy_usage")
>>> col = next(iter(sqla_table.columns))
>>> col
Column('source', VARCHAR(collation='utf8_unicode_ci', length=255), table=<energy_usage>)
```
and thus I'm a little perplexed as to why we need `column_datatype_to_string` as surely this collation and character encoding code could (and possibly should) be removed at the column level.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] villebro commented on issue #9316: fix: remove
character set and collate column info by default
Posted by GitBox <gi...@apache.org>.
villebro commented on issue #9316: fix: remove character set and collate column info by default
URL: https://github.com/apache/incubator-superset/pull/9316#issuecomment-600065555
@john-bodley what you're saying makes total sense. The columns from which these strings are created are constructed here:
https://github.com/apache/incubator-superset/blob/85e9a4fa990927327755f5743317e848fc230f01/superset/models/core.py#L577-L586
I'm thinking we should loop through these columns prior to returning the `Table` instance, and remove collation and character set details if present. Do yo agree?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] villebro commented on a change in pull request
#9316: fix: remove character set and collate column info by default
Posted by GitBox <gi...@apache.org>.
villebro commented on a change in pull request #9316: fix: remove character set and collate column info by default
URL: https://github.com/apache/incubator-superset/pull/9316#discussion_r393919628
##########
File path: superset/db_engine_specs/base.py
##########
@@ -911,13 +912,18 @@ def column_datatype_to_string(
) -> str:
"""
Convert sqlalchemy column type to string representation.
- Can be overridden to remove unnecessary details, especially
- collation info (see mysql, mssql).
+ By default removes collation and character encoding info to avoid unnecessarily
+ long datatypes.
:param sqla_column_type: SqlAlchemy column type
:param dialect: Sqlalchemy dialect
:return: Compiled column type
"""
+ sqla_column_type = sqla_column_type.copy()
Review comment:
Slightly dirty, I agree. I just didn't want to mutate the original object, and also didn't see the need to hang on to the original reference, hence this questionable solution 😁
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] john-bodley edited a comment on issue #9316:
fix: remove character set and collate column info by default
Posted by GitBox <gi...@apache.org>.
john-bodley edited a comment on issue #9316: fix: remove character set and collate column info by default
URL: https://github.com/apache/incubator-superset/pull/9316#issuecomment-599817900
@villebro looking at the example data (using a MySQL database),
```
mysql> show create table energy_usage;
+--------------+-----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------+
| Table | Create Table |
+--------------+-----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------+
| energy_usage | CREATE TABLE `energy_usage` (
`source` varchar(255) COLLATE utf8_unicode_ci DEFAULT NULL,
`target` varchar(255) COLLATE utf8_unicode_ci DEFAULT NULL,
`value` float DEFAULT NULL
) ENGINE=InnoDB DEFAULT CHARSET=utf8 COLLATE=utf8_unicode_ci |
+--------------+-----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------+
```
it seems that the column are defined with the `COLLATE` information which is reflected here,
```python
>>> from superset.utils.core import get_example_database
>>> example_db = get_example_database()
>>> sqla_table = example_db.get_table("energy_usage")
>>> col = next(iter(sqla_table.columns))
>>> col
Column('source', VARCHAR(collation='utf8_unicode_ci', length=255), table=<energy_usage>)
```
and thus I'm a little perplexed as to why we need `column_datatype_to_string` as surely this collation and character encoding code (and possibly should) be removed at the column level.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org