-
Notifications
You must be signed in to change notification settings - Fork 13.6k
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
perf: improve perf in SIP-68 migration #19416
Changes from all commits
4d025ca
69dc87f
b3a84d1
f349047
8d2318c
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -78,6 +78,7 @@ | |
from superset.connectors.sqla.utils import ( | ||
get_physical_table_metadata, | ||
get_virtual_table_metadata, | ||
load_or_create_tables, | ||
validate_adhoc_subquery, | ||
) | ||
from superset.datasets.models import Dataset as NewDataset | ||
|
@@ -2242,7 +2243,10 @@ def write_shadow_dataset( # pylint: disable=too-many-locals | |
if column.is_active is False: | ||
continue | ||
|
||
extra_json = json.loads(column.extra or "{}") | ||
try: | ||
extra_json = json.loads(column.extra or "{}") | ||
except json.decoder.JSONDecodeError: | ||
extra_json = {} | ||
for attr in {"groupby", "filterable", "verbose_name", "python_date_format"}: | ||
value = getattr(column, attr) | ||
if value: | ||
|
@@ -2269,7 +2273,10 @@ def write_shadow_dataset( # pylint: disable=too-many-locals | |
|
||
# create metrics | ||
for metric in dataset.metrics: | ||
extra_json = json.loads(metric.extra or "{}") | ||
try: | ||
extra_json = json.loads(metric.extra or "{}") | ||
except json.decoder.JSONDecodeError: | ||
extra_json = {} | ||
for attr in {"verbose_name", "metric_type", "d3format"}: | ||
value = getattr(metric, attr) | ||
if value: | ||
|
@@ -2300,8 +2307,7 @@ def write_shadow_dataset( # pylint: disable=too-many-locals | |
) | ||
|
||
# physical dataset | ||
tables = [] | ||
if dataset.sql is None: | ||
if not dataset.sql: | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Some of our example datasets have |
||
physical_columns = [column for column in columns if column.is_physical] | ||
|
||
# create table | ||
|
@@ -2314,7 +2320,7 @@ def write_shadow_dataset( # pylint: disable=too-many-locals | |
is_managed_externally=dataset.is_managed_externally, | ||
external_url=dataset.external_url, | ||
) | ||
tables.append(table) | ||
tables = [table] | ||
|
||
# virtual dataset | ||
else: | ||
|
@@ -2325,18 +2331,14 @@ def write_shadow_dataset( # pylint: disable=too-many-locals | |
# find referenced tables | ||
parsed = ParsedQuery(dataset.sql) | ||
referenced_tables = parsed.tables | ||
|
||
# predicate for finding the referenced tables | ||
predicate = or_( | ||
*[ | ||
and_( | ||
NewTable.schema == (table.schema or dataset.schema), | ||
NewTable.name == table.table, | ||
) | ||
for table in referenced_tables | ||
] | ||
tables = load_or_create_tables( | ||
session, | ||
dataset.database_id, | ||
dataset.schema, | ||
referenced_tables, | ||
conditional_quote, | ||
engine, | ||
) | ||
tables = session.query(NewTable).filter(predicate).all() | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We were only assigning tables that already exist. The |
||
|
||
# create the new dataset | ||
new_dataset = NewDataset( | ||
|
@@ -2345,7 +2347,7 @@ def write_shadow_dataset( # pylint: disable=too-many-locals | |
expression=dataset.sql or conditional_quote(dataset.table_name), | ||
tables=tables, | ||
columns=columns, | ||
is_physical=dataset.sql is None, | ||
is_physical=not dataset.sql, | ||
is_managed_externally=dataset.is_managed_externally, | ||
external_url=dataset.external_url, | ||
) | ||
|
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 needed to run the bechmark migration script on the SIP-68 migration.