Skip to content

Commit

Permalink
fix(migrations): Fix the time comparison migration (apache#30029)
Browse files Browse the repository at this point in the history
  • Loading branch information
@Antonio-RiveroMartnez
Antonio-RiveroMartnez committed Aug 27, 2024
1 parent 07a90ad commit d80f23e
Show file tree
Hide file tree
Showing 2 changed files with 75 additions and 157 deletions.
28 changes: 15 additions & 13 deletions ...grations/versions/2024-05-10_18-02_f84fde59123a_update_charts_with_old_time_comparison.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -65,16 +65,17 @@ class Slice(Base):
def upgrade_comparison_params(slice_params: dict[str, Any]) -> dict[str, Any]:
params = deepcopy(slice_params)

if "enable_time_comparison" in params:
# Remove enable_time_comparison
del params["enable_time_comparison"]

# Update time_comparison to time_compare
if "time_comparison" in params:
time_comp = params.pop("time_comparison")
params["time_compare"] = time_map.get(
time_comp, "inherit"
) # Default to 'inherit' if not found
params["time_compare"] = (
[time_map.get(time_comp, "inherit")]
if "enable_time_comparison" in params and params["enable_time_comparison"]
else []
)

if "enable_time_comparison" in params:
del params["enable_time_comparison"]

# Add comparison_type
params["comparison_type"] = "values"
Expand Down Expand Up @@ -119,20 +120,21 @@ def upgrade():

def downgrade_comparison_params(slice_params: dict[str, Any]) -> dict[str, Any]:
params = deepcopy(slice_params)
params["enable_time_comparison"] = False

reverse_time_map = {
v: k for k, v in time_map.items()
} # Reverse the map from the upgrade function

# Add enable_time_comparison
params["enable_time_comparison"] = True

# Revert time_compare to time_comparison
if "time_compare" in params:
time_comp = params.pop("time_compare")
params["time_comparison"] = reverse_time_map.get(
time_comp, "r"
) # Default to 'r' if not found
# Max one element in the time_compare list
time_comp = time_comp[0] if time_comp else ""
params["time_comparison"] = reverse_time_map.get(time_comp, "r")
# If the chart was using any time compare, enable time comparison
if time_comp:
params["enable_time_comparison"] = True

# Remove comparison_type
if "comparison_type" in params:
Expand Down
204 changes: 60 additions & 144 deletions ...integration_tests/migrations/f84fde59123a_update_charts_with_old_time_comparison__test.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,8 @@
migrate_time_comparison_to_new_format.upgrade_comparison_params
)

params_v1_with_custom: dict[str, Any] = {
# Base object containing common properties
base_params: dict[str, Any] = {
"datasource": "2__table",
"viz_type": "pop_kpi",
"metric": {
Expand Down Expand Up @@ -57,20 +58,18 @@
"datasourceWarning": False,
"hasCustomLabel": False,
"label": "SUM(num_boys)",
"optionName": "metric_o6rj1h6jty_3t6mrruogfv",
},
"adhoc_filters": [
{
"expressionType": "SIMPLE",
"subject": "ds",
"operator": "TEMPORAL_RANGE",
"comparator": "1984 : 1986",
"comparator": "1984 : 2000",
"clause": "WHERE",
"sqlExpression": None,
"isExtra": False,
"isNew": False,
"datasourceWarning": False,
"filterOptionName": "filter_p50i4xw50d_8x8e4ypwjs8",
}
],
"row_limit": 10000,
Expand All @@ -81,6 +80,22 @@
"comparison_color_scheme": "Green",
"extra_form_data": {},
"dashboards": [],
}

# Specific parameter objects overriding only the differing properties
params_v1_with_custom: dict[str, Any] = {
**base_params,
"metric": {
**base_params["metric"],
"optionName": "metric_o6rj1h6jty_3t6mrruogfv",
},
"adhoc_filters": [
{
**base_params["adhoc_filters"][0],
"comparator": "1984 : 1986",
"filterOptionName": "filter_p50i4xw50d_8x8e4ypwjs8",
}
],
"time_comparison": "c",
"enable_time_comparison": True,
"adhoc_custom": [
Expand All @@ -97,58 +112,13 @@
}
],
}

params_v1_other_than_custom: dict[str, Any] = {
"datasource": "2__table",
"viz_type": "pop_kpi",
**base_params,
"metric": {
"expressionType": "SIMPLE",
"column": {
"advanced_data_type": None,
"certification_details": None,
"certified_by": None,
"column_name": "num_boys",
"description": None,
"expression": None,
"filterable": True,
"groupby": True,
"id": 334,
"is_certified": False,
"is_dttm": False,
"python_date_format": None,
"type": "BIGINT",
"type_generic": 0,
"verbose_name": None,
"warning_markdown": None,
},
"aggregate": "SUM",
"sqlExpression": None,
"datasourceWarning": False,
"hasCustomLabel": False,
"label": "SUM(num_boys)",
**base_params["metric"],
"optionName": "metric_96s7b8iypsr_4wrlgm0i7il",
},
"adhoc_filters": [
{
"expressionType": "SIMPLE",
"subject": "ds",
"operator": "TEMPORAL_RANGE",
"comparator": "1984 : 2000",
"clause": "WHERE",
"sqlExpression": None,
"isExtra": False,
"isNew": False,
"datasourceWarning": False,
"filterOptionName": "filter_2sefqq1rwb7_lhqvw7ukc6",
}
],
"row_limit": 10000,
"y_axis_format": "SMART_NUMBER",
"percentDifferenceFormat": "SMART_NUMBER",
"header_font_size": 0.2,
"subheader_font_size": 0.125,
"comparison_color_scheme": "Green",
"extra_form_data": {},
"dashboards": [],
"time_comparison": "r",
"enable_time_comparison": True,
"adhoc_custom": [
Expand All @@ -161,118 +131,45 @@
}
],
}

params_v1_other_than_custom_false: dict[str, Any] = {
**params_v1_other_than_custom,
"enable_time_comparison": False,
}

params_v2_with_custom: dict[str, Any] = {
"datasource": "2__table",
"viz_type": "pop_kpi",
**base_params,
"metric": {
"expressionType": "SIMPLE",
"column": {
"advanced_data_type": None,
"certification_details": None,
"certified_by": None,
"column_name": "num_boys",
"description": None,
"expression": None,
"filterable": True,
"groupby": True,
"id": 334,
"is_certified": False,
"is_dttm": False,
"python_date_format": None,
"type": "BIGINT",
"type_generic": 0,
"verbose_name": None,
"warning_markdown": None,
},
"aggregate": "SUM",
"sqlExpression": None,
"datasourceWarning": False,
"hasCustomLabel": False,
"label": "SUM(num_boys)",
**base_params["metric"],
"optionName": "metric_o6rj1h6jty_3t6mrruogfv",
},
"adhoc_filters": [
{
"expressionType": "SIMPLE",
"subject": "ds",
"operator": "TEMPORAL_RANGE",
**base_params["adhoc_filters"][0],
"comparator": "1984 : 1986",
"clause": "WHERE",
"sqlExpression": None,
"isExtra": False,
"isNew": False,
"datasourceWarning": False,
"filterOptionName": "filter_p50i4xw50d_8x8e4ypwjs8",
}
],
"row_limit": 10000,
"y_axis_format": "SMART_NUMBER",
"percentDifferenceFormat": "SMART_NUMBER",
"header_font_size": 0.2,
"subheader_font_size": 0.125,
"comparison_color_scheme": "Green",
"extra_form_data": {},
"dashboards": [],
"time_compare": "custom",
"time_compare": ["custom"],
"comparison_type": "values",
"start_date_offset": "1981-01-01",
}

params_v2_other_than_custom: dict[str, Any] = {
"datasource": "2__table",
"viz_type": "pop_kpi",
**base_params,
"metric": {
"expressionType": "SIMPLE",
"column": {
"advanced_data_type": None,
"certification_details": None,
"certified_by": None,
"column_name": "num_boys",
"description": None,
"expression": None,
"filterable": True,
"groupby": True,
"id": 334,
"is_certified": False,
"is_dttm": False,
"python_date_format": None,
"type": "BIGINT",
"type_generic": 0,
"verbose_name": None,
"warning_markdown": None,
},
"aggregate": "SUM",
"sqlExpression": None,
"datasourceWarning": False,
"hasCustomLabel": False,
"label": "SUM(num_boys)",
**base_params["metric"],
"optionName": "metric_96s7b8iypsr_4wrlgm0i7il",
},
"adhoc_filters": [
{
"expressionType": "SIMPLE",
"subject": "ds",
"operator": "TEMPORAL_RANGE",
"comparator": "1984 : 2000",
"clause": "WHERE",
"sqlExpression": None,
"isExtra": False,
"isNew": False,
"datasourceWarning": False,
"filterOptionName": "filter_2sefqq1rwb7_lhqvw7ukc6",
}
],
"row_limit": 10000,
"y_axis_format": "SMART_NUMBER",
"percentDifferenceFormat": "SMART_NUMBER",
"header_font_size": 0.2,
"subheader_font_size": 0.125,
"comparison_color_scheme": "Green",
"extra_form_data": {},
"dashboards": [],
"time_compare": "inherit",
"time_compare": ["inherit"],
"comparison_type": "values",
}

params_v2_other_than_custom_false: dict[str, Any] = {
**params_v2_other_than_custom,
"time_compare": [],
}


def test_upgrade_chart_params_with_custom():
"""
Expand Down Expand Up @@ -313,3 +210,22 @@ def test_downgrade_chart_params_other_than_custom():
original_params = deepcopy(params_v2_other_than_custom)
downgraded_params = downgrade_comparison_params(original_params)
assert downgraded_params == params_v1_other_than_custom


def test_upgrade_chart_params_other_than_custom_false():
"""
ensure that the new time comparison params are added
"""
original_params = deepcopy(params_v1_other_than_custom_false)
upgraded_params = upgrade_comparison_params(original_params)
assert upgraded_params == params_v2_other_than_custom_false


def test_downgrade_chart_params_other_than_custom_false():
"""
ensure that the params downgrade operation produces an almost identical dict
as the original value
"""
original_params = deepcopy(params_v2_other_than_custom_false)
downgraded_params = downgrade_comparison_params(original_params)
assert downgraded_params == params_v1_other_than_custom_false

0 comments on commit d80f23e

Please sign in to comment.