villebro commented on a change in pull request #13294:
URL: https://github.com/apache/superset/pull/13294#discussion_r590459217
##########
File path: tests/sqla_models_tests.py
##########
@@ -79,16 +79,16 @@ def test_db_column_types(self):
# string
"CHAR": GenericDataType.STRING,
"VARCHAR": GenericDataType.STRING,
- "NVARCHAR": GenericDataType.STRING,
- "STRING": GenericDataType.STRING,
+ # "NVARCHAR": GenericDataType.STRING, # MSSQL types; commeented
out for now and will address in another PR
+ # "STRING": GenericDataType.STRING,
Review comment:
Were these causing problems in tests? This test might need some
refactoring, as it will potentially give different results on different
engines. We could potentially simplify this a bit by only checking types that
are common for all databases supported by CI, and later potentially adding a
few db specific tests.
##########
File path: superset/db_engine_specs/base.py
##########
@@ -967,24 +1040,35 @@ def make_label_compatible(cls, label: str) -> Union[str,
quoted_name]:
return label_mutated
@classmethod
- def get_sqla_column_type(cls, type_: Optional[str]) ->
Optional[TypeEngine]:
+ def get_sqla_column_type(
+ cls,
+ column_type: Optional[str],
+ column_type_mappings: Tuple[
+ Tuple[
+ Pattern[str],
+ Union[TypeEngine, Callable[[Match[str]], TypeEngine]],
+ GenericDataType,
+ ],
+ ...,
+ ] = column_type_mappings,
Review comment:
Instead of passing the mapping to the method, we can probably just call
the `cls.column_type_mappings` property in the method call.
##########
File path: superset/db_engine_specs/mssql.py
##########
@@ -78,8 +78,16 @@ def fetch_data(
return cls.pyodbc_rows_to_tuples(data)
column_type_mappings = (
- (re.compile(r"^N((VAR)?CHAR|TEXT)", re.IGNORECASE), UnicodeText()),
- (re.compile(r"^((VAR)?CHAR|TEXT|STRING)", re.IGNORECASE), String()),
+ (
+ re.compile(r"^N((VAR)?CHAR|TEXT)", re.IGNORECASE),
+ UnicodeText(),
+ utils.GenericDataType.STRING,
+ ),
+ (
+ re.compile(r"^((VAR)?CHAR|TEXT|STRING)", re.IGNORECASE),
+ String(),
+ utils.GenericDataType.STRING,
+ ),
Review comment:
I would probably design this so that an engine spec can extend the base
mapping. In the case of MSSQL, I believe the base mapping is a good fallback.
Also, we might consider incorporating these types into the base spec, as I
assume fairly many engines support N-prefixed character types, and some of
those engines might also benefit from the `UnicodeText` SQLA type over the
regular `String` one.
----------------------------------------------------------------
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:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
