-
Notifications
You must be signed in to change notification settings - Fork 14.3k
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
feat(explore): Postgres datatype conversion #13294
feat(explore): Postgres datatype conversion #13294
Conversation
Codecov Report
@@ Coverage Diff @@
## master #13294 +/- ##
==========================================
- Coverage 77.31% 71.24% -6.08%
==========================================
Files 903 826 -77
Lines 45926 41326 -4600
Branches 5624 4265 -1359
==========================================
- Hits 35508 29442 -6066
- Misses 10282 11884 +1602
+ Partials 136 0 -136
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
@nikolagigic Hey Nikola, thanks for the PR, I understand there's no UI changes involved, but could you add more details and context in description? 🙏 |
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.
A few first pass comments
superset/db_engine_specs/base.py
Outdated
postgres_types_map: Dict[utils.GenericDataType, List[str]] = { | ||
utils.GenericDataType.NUMERIC: [ | ||
"smallint", | ||
"integer", | ||
"bigint", | ||
"decimal", | ||
"numeric", | ||
"real", | ||
"double precision", | ||
"smallserial", | ||
"serial", | ||
"bigserial", | ||
], | ||
utils.GenericDataType.STRING: ["varchar", "char", "text",], | ||
utils.GenericDataType.TEMPORAL: [ | ||
"DATE", | ||
"TIME", | ||
"TIMESTAMP", | ||
"TIMESTAMPTZ", | ||
"INTERVAL", | ||
], | ||
utils.GenericDataType.BOOLEAN: ["boolean",], | ||
} |
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.
I would break this out so that the base engine only provides a default mapping (can return None
now), and then each engine would implement the method as they want. An example of similar logic: convert_dttm
: BaseEngineSpec
returns None
(
superset/superset/db_engine_specs/base.py
Lines 533 to 542 in 99a0c8a
@classmethod | |
def convert_dttm(cls, target_type: str, dttm: datetime) -> Optional[str]: | |
""" | |
Convert Python datetime object to a SQL expression | |
:param target_type: The target type of expression | |
:param dttm: The datetime object | |
:return: The SQL expression | |
""" | |
return None |
BigQueryEngineSpec
implements it (superset/superset/db_engine_specs/bigquery.py
Lines 89 to 100 in 99a0c8a
@classmethod | |
def convert_dttm(cls, target_type: str, dttm: datetime) -> Optional[str]: | |
tt = target_type.upper() | |
if tt == utils.TemporalType.DATE: | |
return f"CAST('{dttm.date().isoformat()}' AS DATE)" | |
if tt == utils.TemporalType.DATETIME: | |
return f"""CAST('{dttm.isoformat(timespec="microseconds")}' AS DATETIME)""" | |
if tt == utils.TemporalType.TIME: | |
return f"""CAST('{dttm.strftime("%H:%M:%S.%f")}' AS TIME)""" | |
if tt == utils.TemporalType.TIMESTAMP: | |
return f"""CAST('{dttm.isoformat(timespec="microseconds")}' AS TIMESTAMP)""" | |
return None |
For matching the native type to specific types I would probably use a sequence of regexp to to find the first match. Something similar has been done on Presto t map the database type to SQLAlchemy types (this new functionality would replace the old logic):
superset/superset/db_engine_specs/presto.py
Lines 358 to 384 in 99a0c8a
column_type_mappings = ( | |
(re.compile(r"^boolean.*", re.IGNORECASE), types.Boolean()), | |
(re.compile(r"^tinyint.*", re.IGNORECASE), TinyInteger()), | |
(re.compile(r"^smallint.*", re.IGNORECASE), types.SmallInteger()), | |
(re.compile(r"^integer.*", re.IGNORECASE), types.Integer()), | |
(re.compile(r"^bigint.*", re.IGNORECASE), types.BigInteger()), | |
(re.compile(r"^real.*", re.IGNORECASE), types.Float()), | |
(re.compile(r"^double.*", re.IGNORECASE), types.Float()), | |
(re.compile(r"^decimal.*", re.IGNORECASE), types.DECIMAL()), | |
( | |
re.compile(r"^varchar(\((\d+)\))*$", re.IGNORECASE), | |
lambda match: types.VARCHAR(int(match[2])) if match[2] else types.String(), | |
), | |
( | |
re.compile(r"^char(\((\d+)\))*$", re.IGNORECASE), | |
lambda match: types.CHAR(int(match[2])) if match[2] else types.CHAR(), | |
), | |
(re.compile(r"^varbinary.*", re.IGNORECASE), types.VARBINARY()), | |
(re.compile(r"^json.*", re.IGNORECASE), types.JSON()), | |
(re.compile(r"^date.*", re.IGNORECASE), types.DATE()), | |
(re.compile(r"^timestamp.*", re.IGNORECASE), types.TIMESTAMP()), | |
(re.compile(r"^time.*", re.IGNORECASE), types.Time()), | |
(re.compile(r"^interval.*", re.IGNORECASE), Interval()), | |
(re.compile(r"^array.*", re.IGNORECASE), Array()), | |
(re.compile(r"^map.*", re.IGNORECASE), Map()), | |
(re.compile(r"^row.*", re.IGNORECASE), Row()), | |
) |
__time
is actually TIMESTAMP
despite being returned as STRING
on the cursor description), so these should probably be caught before doing the matching from the database type.
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.
I like the new ColumnSpec
class. It would be nice if you could describe in PR description more in details how you plan to use it.
superset/db_engine_specs/base.py
Outdated
@@ -41,7 +41,8 @@ | |||
import sqlparse | |||
from flask import g | |||
from flask_babel import lazy_gettext as _ | |||
from sqlalchemy import column, DateTime, select | |||
from sqlalchemy import column, DateTime, select, types | |||
from sqlalchemy.dialects.postgresql import DOUBLE_PRECISION |
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.
Seems like an extraneous import
superset/db_engine_specs/postgres.py
Outdated
@@ -45,6 +48,28 @@ class PostgresBaseEngineSpec(BaseEngineSpec): | |||
engine = "" | |||
engine_name = "PostgreSQL" | |||
|
|||
column_type_mappings = ( |
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.
Would it make sense to consolidate the structure of db_column_types
and column_type_mappings
as they are basically doing very similar things (one to convert db column type to generic types; one to convert to SQLA types)?
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.
Yes - this is the ultimate objective. However, we want to leave the existing structures untouched until we're certain this doesn't break existing functionality. We'll be including other types in this mapping that are missing in db_column_types
, like DbColumnType
, PyArrow types etc to make sure this is a one-stop-shop for making available all necessary types in one place.
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.
A few comments
superset/db_engine_specs/postgres.py
Outdated
column_type_mappings = ( | ||
(re.compile(r"^smallint", re.IGNORECASE), types.SMALLINT), | ||
(re.compile(r"^integer", re.IGNORECASE), types.INTEGER), | ||
(re.compile(r"^bigint", re.IGNORECASE), types.BIGINT), | ||
(re.compile(r"^decimal", re.IGNORECASE), types.DECIMAL), | ||
(re.compile(r"^numeric", re.IGNORECASE), types.NUMERIC), | ||
(re.compile(r"^real", re.IGNORECASE), types.REAL), | ||
(re.compile(r"^double precision", re.IGNORECASE), DOUBLE_PRECISION), | ||
(re.compile(r"^smallserial", re.IGNORECASE), types.SMALLINT), | ||
(re.compile(r"^serial", re.IGNORECASE), types.INTEGER), | ||
(re.compile(r"^bigserial", re.IGNORECASE), types.BIGINT), | ||
(re.compile(r"^varchar", re.IGNORECASE), types.VARCHAR), | ||
(re.compile(r"^char", re.IGNORECASE), types.CHAR), | ||
(re.compile(r"^text", re.IGNORECASE), types.TEXT), | ||
(re.compile(r"^date", re.IGNORECASE), types.DATE), | ||
(re.compile(r"^time", re.IGNORECASE), types.TIME), | ||
(re.compile(r"^timestamp", re.IGNORECASE), types.TIMESTAMP), | ||
(re.compile(r"^timestamptz", re.IGNORECASE), types.TIMESTAMP(timezone=True)), | ||
(re.compile(r"^interval", re.IGNORECASE), types.Interval), | ||
(re.compile(r"^boolean", re.IGNORECASE), types.BOOLEAN), | ||
) |
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.
We should add at least DbColumnType
in this mapping, too (should be added to ColumnSpec
), as we want to pass that when returning chart query data to the frontend. This will make is_dttm
redundant, as that info is already covered by DbColumnType.TEMPORAL
.
superset/db_engine_specs/postgres.py
Outdated
(re.compile(r"^timestamp", re.IGNORECASE), types.TIMESTAMP), | ||
(re.compile(r"^timestamptz", re.IGNORECASE), types.TIMESTAMP(timezone=True)), |
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.
Were you able to get these from inspector.get_columns()
or cursor.description
? When I check a table with timestamps it seems to return the unabbreviated format TIMESTAMP WITHOUT TIME ZONE
, not TIMESTAMP
:
I'm assuming the abbreviations are usually relevant when creating tables, not fetching table metadata, but I may be wrong (this may have changed over the versions/years):
fc0a7a1
to
488a840
Compare
641a16a
to
c894b90
Compare
superset/db_engine_specs/base.py
Outdated
dttm_types = [ | ||
types.TIME, | ||
types.TIMESTAMP, | ||
types.TIMESTAMP(timezone=True), | ||
types.Interval, | ||
] |
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.
I don't believe this is needed anymore.
superset/db_engine_specs/base.py
Outdated
def get_sqla_column_type(cls, type_: Optional[str]) -> Optional[TypeEngine]: | ||
def get_sqla_column_type( | ||
cls, column_type: Optional[str] | ||
) -> Tuple[Union[TypeEngine, utils.GenericDataType, None]]: |
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.
I would consider just leaving the signature of this method unchanged for now; otherwise I'd just remove this one and start using the new get_column_spec
method and implement that wherever get_sqla_column_type
is being used, as that's the end state we want to aim for.
superset/db_engine_specs/base.py
Outdated
return None | ||
return sqla_type(match), generic_type | ||
return sqla_type, generic_type | ||
return None, None |
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.
For cases where we want to return an empty value (=no match was made), I'd perhaps prefer returning a pure None
instead of a Tuple
with None
s.
superset/db_engine_specs/base.py
Outdated
if ( | ||
source == utils.ColumnTypeSource.CURSOR_DESCRIPION | ||
): # Further logic to be implemented | ||
pass |
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.
For now we can assume GET_TABLE
and CURSOR_DESCRIPTION
is handled with the same matching logic - this can later be refined for engines where this makes a difference. Also, we might consider keeping the base implementation as simple as possible, and leaving the more complex logic to the individual db engine specs.
superset/utils/core.py
Outdated
ARRAY = 4 | ||
JSON = 5 | ||
MAP = 6 | ||
ROW = 7 |
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.
We need to make sure this logic is in synced with superset-ui/core
- for now it might be a good idea to just map all complex types to STRING
as has previously been done, and later introduce more complex generic types once we add proper support for them.
superset/db_engine_specs/postgres.py
Outdated
( | ||
re.compile(r"^smallint", re.IGNORECASE), | ||
types.SMALLINT, | ||
utils.GenericDataType.NUMERIC, |
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.
Nit: can we import GenericDataType
directly and get rid of the utils.
prefix? Just want the code the look cleaner.
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.
But, is adding GenericDataType
here really necessary, though? I'd imagine all SQLA types could be definitively mapped to a GenericDataType
, wouldn't they? Maybe ColumnSpec
can just have an instance @property
to map all known SQLA types to GenericDataType
?
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.
@ktmud Yes, it could've been mapped as you proposed. The reason behind the current way is that if we get multiple matches we can prioritise those at the start of the list without necessarily sorting the results for the most part.
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.
I understand the need to use a list to have priority for RegExp matching, but will there be a case where two SQLA types may be matched to different GenericDataType
? If not, isn't GenericDataType
already inferred by SQLA types
?
superset/db_engine_specs/presto.py
Outdated
), | ||
( | ||
re.compile(r"^smallint.*", re.IGNORECASE), | ||
types.SmallInteger(), |
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.
Are types.SMALLINT
and types.SmallInteger()
interchangeable? If yes, can we stick to just one of them?
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.
Changed it to keep it consistent
column_type_mappings: Tuple[ | ||
Tuple[ | ||
Pattern[str], | ||
Union[TypeEngine, Callable[[Match[str]], TypeEngine]], | ||
GenericDataType, | ||
], | ||
..., | ||
] = column_type_mappings, |
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.
Instead of passing the mapping to the method, we can probably just call the cls.column_type_mappings
property in the method call.
superset/db_engine_specs/mssql.py
Outdated
(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, | ||
), |
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.
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.
tests/sqla_models_tests.py
Outdated
# "NVARCHAR": GenericDataType.STRING, # MSSQL types; commeented out for now and will address in another PR | ||
# "STRING": GenericDataType.STRING, |
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.
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.
51b7a82
to
bfdc994
Compare
superset/db_engine_specs/base.py
Outdated
( | ||
re.compile(r"^timestamp", re.IGNORECASE), | ||
types.TIMESTAMP(), | ||
GenericDataType.TEMPORAL, | ||
), | ||
( | ||
re.compile(r"^timestamptz", re.IGNORECASE), | ||
types.TIMESTAMP(timezone=True), | ||
GenericDataType.TEMPORAL, | ||
), |
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.
^timestamptz
would never be caught as it's already matching ^timestamp
above. I suggest removing timestamptz
support for now (timezones aren't really properly supported yet).
superset/db_engine_specs/base.py
Outdated
db_column_types: Dict[utils.GenericDataType, Tuple[Pattern[str], ...]] = { | ||
utils.GenericDataType.NUMERIC: ( | ||
db_column_types: Dict[GenericDataType, Tuple[Pattern[str], ...]] = { | ||
GenericDataType.NUMERIC: ( |
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.
Do we need this map anymore? I believe this can be achieved with get_column_spec
superset/db_engine_specs/base.py
Outdated
cls, db_column_type: Optional[str], target_column_type: utils.GenericDataType | ||
cls, db_column_type: Optional[str], target_column_type: GenericDataType |
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.
I believe we can remove this method, as I don't see it being used anymore
tests/databases/commands_tests.py
Outdated
self.maxDiff = None | ||
self.assertEquals(metadata, expected_metadata) |
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.
Let's not change this
self.maxDiff = None | |
self.assertEquals(metadata, expected_metadata) | |
assert metadata == expected_metadata |
tests/db_engine_specs/mssql_tests.py
Outdated
assert_type("INT", None) | ||
assert_type("STRING", String) | ||
assert_type("CHAR(10)", String) | ||
assert_type("VARCHAR(10)", String) | ||
assert_type("TEXT", String) | ||
assert_type("NCHAR(10)", UnicodeText) | ||
assert_type("NVARCHAR(10)", UnicodeText) | ||
assert_type("NTEXT", UnicodeText) | ||
# assert_type("STRING", String, GenericDataType.STRING) | ||
# assert_type("CHAR(10)", String, GenericDataType.STRING) | ||
# assert_type("VARCHAR(10)", String, GenericDataType.STRING) | ||
# assert_type("TEXT", String, GenericDataType.STRING) | ||
# assert_type("NCHAR(10)", UnicodeText, GenericDataType.STRING) | ||
# assert_type("NVARCHAR(10)", UnicodeText, GenericDataType.STRING) | ||
# assert_type("NTEXT", UnicodeText, GenericDataType.STRING) |
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.
Why are these removed? couldn't we assert this using get_column_spec
?
tests/db_engine_specs/mssql_tests.py
Outdated
def test_where_clause_n_prefix(self): | ||
dialect = mssql.dialect() | ||
spec = MssqlEngineSpec | ||
str_col = column("col", type_=spec.get_sqla_column_type("VARCHAR(10)")) | ||
unicode_col = column("unicode_col", type_=spec.get_sqla_column_type("NTEXT")) | ||
tbl = table("tbl") | ||
sel = ( | ||
select([str_col, unicode_col]) | ||
.select_from(tbl) | ||
.where(str_col == "abc") | ||
.where(unicode_col == "abc") | ||
) | ||
# def test_where_clause_n_prefix(self): | ||
# dialect = mssql.dialect() | ||
# spec = MssqlEngineSpec | ||
# type_, _ = spec.get_sqla_column_type("VARCHAR(10)") | ||
# str_col = column("col", type_=type_) | ||
# type_, _ = spec.get_sqla_column_type("NTEXT") | ||
# unicode_col = column("unicode_col", type_=type_) | ||
# tbl = table("tbl") | ||
# sel = ( | ||
# select([str_col, unicode_col]) | ||
# .select_from(tbl) | ||
# .where(str_col == "abc") | ||
# .where(unicode_col == "abc") | ||
# ) | ||
|
||
query = str( | ||
sel.compile(dialect=dialect, compile_kwargs={"literal_binds": True}) | ||
) | ||
query_expected = ( | ||
"SELECT col, unicode_col \n" | ||
"FROM tbl \n" | ||
"WHERE col = 'abc' AND unicode_col = N'abc'" | ||
) | ||
self.assertEqual(query, query_expected) | ||
# query = str( | ||
# sel.compile(dialect=dialect, compile_kwargs={"literal_binds": True}) | ||
# ) | ||
# query_expected = ( | ||
# "SELECT col, unicode_col \n" | ||
# "FROM tbl \n" | ||
# "WHERE col = 'abc' AND unicode_col = N'abc'" | ||
# ) | ||
# self.assertEqual(query, query_expected) |
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.
This is an important test - if it doesn't work we need to make sure it does
tests/db_engine_specs/mssql_tests.py
Outdated
(NVARCHAR(length=128), "NVARCHAR(128)"), | ||
# (NVARCHAR(length=128), "NVARCHAR(128)"), | ||
(TEXT(), "TEXT"), | ||
(NTEXT(collation="utf8_general_ci"), "NTEXT"), | ||
# (NTEXT(collation="utf8_general_ci"), "NTEXT"), |
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.
This functionality+test can be removed later, but I believe we need to make a db migration to resize the type column in the metadata (not sure if it was already).
tests/db_engine_specs/mysql_tests.py
Outdated
("INT", GenericDataType.NUMERIC), | ||
("INTEGER", GenericDataType.NUMERIC), |
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.
Isn't INT
an official datatype in Mysql? https://dev.mysql.com/doc/refman/8.0/en/integer-types.html I think we need to keep this here.
tests/db_engine_specs/mysql_tests.py
Outdated
type_str, GenericDataType.TEMPORAL | ||
) is (col_type == GenericDataType.TEMPORAL) | ||
for type_str, col_type in type_expectations: | ||
print(">>> ", type_str) |
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.
assuming a leftover:
print(">>> ", type_str) | |
print(">>> ", type_str) |
GenericDataType.NUMERIC, | ||
), | ||
( | ||
re.compile(r"^integer", re.IGNORECASE), |
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.
To make sure we match Mysql INT
type, we could just change this to ^INT
to match both INT
and INTEGER
, unless there are any known incompatible types that could cause a collision.
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.
There is an INTEGER
type in mysql
dialect.
* test * unnecessary import * fix lint * changes * fix lint * changes * changes * changes * changes * answering comments & changes * answering comments * answering comments * changes * changes * changes * fix tests * fix tests * fix tests * fix tests * fix tests * fix tests * fix tests * fix tests * fix tests * fix tests * fix tests * fix tests * fix tests * fix tests * fix tests * fix tests * fix tests * fix tests
SUMMARY
In effort to make
db_engine_spec
more stable, we are moving all the type conversion logic to one place ->BaseEngineSpec
and then reusing it per engine level as needed. This PR should give an PostgreSQL POC, to learn from it and adapt for the rest of the engines.BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
TEST PLAN
ADDITIONAL INFORMATION