Skip to content
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

[MAINTENANCE] Performance improvement refactor for Spark unexpected values #3368

Merged
Show file tree
Hide file tree
Changes from 9 commits
Commits
Show all changes
32 commits
Select commit Hold shift + click to select a range
ec72b07
[MAINTENANCE] Performance improvement refactor for helper _spark_colu…
Sep 8, 2021
5ad62bc
Merge branch 'develop' into working-branch/DEVREL-154/improve_SparkDF…
NathanFarmer Sep 8, 2021
778051e
Merge branch 'develop' into working-branch/DEVREL-154/improve_SparkDF…
NathanFarmer Sep 16, 2021
4473092
[MAINTENANCE] Performance improvement refactor for helper _spark_colu…
Sep 16, 2021
a784897
Merge branch 'working-branch/DEVREL-154/improve_SparkDFExecutionEngin…
Sep 16, 2021
66b5ae3
Change log
Sep 16, 2021
21eea41
[MAINTENANCE] This test no longer applies to spark because we stopped…
Sep 17, 2021
a39aa19
[MAINTENANCE] Remove all sorting logic from spark provider helpers (#…
Sep 17, 2021
abf47b8
[MAINTENANCE] Sort dictionaries in tests for comparisons (#3368).
Sep 17, 2021
a276eab
Merge branch 'develop' into working-branch/DEVREL-154/improve_SparkDF…
NathanFarmer Sep 17, 2021
fea4e71
Linting
Sep 17, 2021
bd8fc07
Merge branch 'working-branch/DEVREL-154/improve_SparkDFExecutionEngin…
Sep 17, 2021
8050ce9
Clean up
Sep 17, 2021
522a3f3
[MAINTENANCE] Incorrect source of __lt__ in comment (#3368).
Sep 17, 2021
fcb22b8
[MAINTENANCE] Clarify how sorting works for each data type (#3368).
Sep 17, 2021
dd56b24
[MAINTENANCE] Lambda instead of itemgetter for consistency/simplicity…
Sep 17, 2021
ce2105e
Linting
Sep 17, 2021
8b48961
Accidentally re-used variable name
Sep 17, 2021
2e2ff25
Linting
Sep 17, 2021
7a72aaa
[MAINTENANCE] Change final use of boolean_mapped_unexpected_values to…
Sep 21, 2021
19f3bd4
[MAINTENANCE] Helper function for sorting unexpected_values during te…
Sep 21, 2021
7adf3b9
[MAINTENANCE] When exact_match_out is True we still need to sort unex…
Sep 21, 2021
b9bb7cf
[MAINTENANCE] Moved sort logic into helper function (#3368).
Sep 21, 2021
099802a
Cleanup
Sep 21, 2021
1f9d8d3
[MAINTENANCE] Sort should also be applied to partial_unexpected_list …
Sep 21, 2021
93b5d25
[MAINTENANCE] Revert broken test back to its original state (#3368).
Sep 21, 2021
80074e9
Linting
Sep 21, 2021
e1ddfee
Merge branch 'develop' into working-branch/DEVREL-154/improve_SparkDF…
NathanFarmer Sep 21, 2021
341b769
[MAINTENANCE] Consolidate sorting to make it clear that we do it whet…
Sep 21, 2021
d338d03
Merge branch 'working-branch/DEVREL-154/improve_SparkDFExecutionEngin…
Sep 21, 2021
51f2005
Linting
Sep 21, 2021
753e44d
Merge branch 'develop' into working-branch/DEVREL-154/improve_SparkDF…
NathanFarmer Sep 21, 2021
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
Expand Up @@ -2477,7 +2477,7 @@ def _spark_column_pair_map_condition_values(
):
"""Return values from the specified domain that match the map-style metric in the metrics dictionary."""
(
boolean_mapped_unexpected_values,
unexpected_condition,
compute_domain_kwargs,
accessor_domain_kwargs,
) = metrics["unexpected_condition"]
Expand All @@ -2503,7 +2503,9 @@ def _spark_column_pair_map_condition_values(
message=f'Error: The column "{column_name}" in BatchData does not exist.'
)

filtered = df.filter(boolean_mapped_unexpected_values)
# withColumn is required to transform window functions returned by some metrics to boolean mask
data = df.withColumn("__unexpected", unexpected_condition)
filtered = data.filter(F.col("__unexpected") == True).drop(F.col("__unexpected"))

result_format = metric_value_kwargs["result_format"]
if result_format["result_format"] == "COMPLETE":
Expand Down
88 changes: 68 additions & 20 deletions great_expectations/self_check/util.py
Expand Up @@ -1766,6 +1766,30 @@ def generate_expectation_tests(
return parametrized_tests


def sort_unexpected_values(test_value_list, result_value_list):
# check if value can be sorted; if so, sort so arbitrary ordering of results does not cause failure
if (isinstance(test_value_list, list)) & (len(test_value_list) >= 1):
# __lt__ is not implemented for python dictionaries making sorting trickier
# in our case, we will sort on the values for each key sequentially
if isinstance(test_value_list[0], dict):
test_value_list = sorted(
test_value_list,
key=lambda x: tuple(x[k] for k in list(test_value_list[0].keys())),
)
result_value_list = sorted(
result_value_list,
key=lambda x: tuple(x[k] for k in list(test_value_list[0].keys())),
)
# if python built-in class has __lt__ then sorting can always work this way
elif type(test_value_list[0].__lt__(test_value_list[0])) != type(
NotImplemented
):
test_value_list = sorted(test_value_list, key=lambda x: str(x))
result_value_list = sorted(result_value_list, key=lambda x: str(x))

return test_value_list, result_value_list


def evaluate_json_test(data_asset, expectation_type, test):
"""
This method will evaluate the result of a test build using the Great Expectations json test format.
Expand Down Expand Up @@ -1879,7 +1903,32 @@ def evaluate_json_test_cfe(validator, expectation_type, test):

def check_json_test_result(test, result, data_asset=None):
# Check results
if test["exact_match_out"] is True:
# For Spark we cannot guarantee the order in which values are returned, so we sort for testing purposes
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@NathanFarmer Naive thought: Why not sort for all backends? I do not see detriment in pre-sorting in a well-defined order these unexpected values for all backends indiscriminately. What do you think? Thanks!

if (test["exact_match_out"] is True) and isinstance(
data_asset, (SparkDFDataset, SparkDFBatchData)
):
if ("unexpected_list" in result["result"]) and (
"unexpected_list" in test["out"]["result"]
):
(
test["out"]["result"]["unexpected_list"],
result["result"]["unexpected_list"],
) = sort_unexpected_values(
test["out"]["result"]["unexpected_list"],
result["result"]["unexpected_list"],
)
if ("partial_unexpected_list" in result["result"]) and (
"partial_unexpected_list" in test["out"]["result"]
):
(
test["out"]["result"]["partial_unexpected_list"],
result["result"]["partial_unexpected_list"],
) = sort_unexpected_values(
test["out"]["result"]["partial_unexpected_list"],
result["result"]["partial_unexpected_list"],
)
assert result == expectationValidationResultSchema.load(test["out"])
elif test["exact_match_out"] is True:
assert result == expectationValidationResultSchema.load(test["out"])
else:
# Convert result to json since our tests are reading from json so cannot easily contain richer types (e.g. NaN)
Expand Down Expand Up @@ -1928,25 +1977,9 @@ def check_json_test_result(test, result, data_asset=None):
assert result["result"]["unexpected_index_list"] == value

elif key == "unexpected_list":
# check if value can be sorted; if so, sort so arbitrary ordering of results does not cause failure
if (isinstance(value, list)) & (len(value) >= 1):
# __lt__ is not implemented for python dictionaries making sorting trickier
# in our case, we will sort on the values for each key sequentially
if isinstance(value[0], dict):
value = sorted(
value,
key=lambda x: tuple(x[k] for k in list(value[0].keys())),
)
result["result"]["unexpected_list"] = sorted(
result["result"]["unexpected_list"],
key=lambda x: tuple(x[k] for k in list(value[0].keys())),
)
# if python built-in class has __lt__ then sorting can always work this way
elif type(value[0].__lt__(value[0])) != type(NotImplemented):
value = sorted(value, key=lambda x: str(x))
result["result"]["unexpected_list"] = sorted(
result["result"]["unexpected_list"], key=lambda x: str(x)
)
value, result["result"]["unexpected_list"] = sort_unexpected_values(
value, result["result"]["unexpected_list"]
)

assert result["result"]["unexpected_list"] == value, (
"expected "
Expand All @@ -1955,6 +1988,21 @@ def check_json_test_result(test, result, data_asset=None):
+ str(result["result"]["unexpected_list"])
)

elif key == "partial_unexpected_list":
(
value,
result["result"]["partial_unexpected_list"],
) = sort_unexpected_values(
value, result["result"]["partial_unexpected_list"]
)

assert result["result"]["partial_unexpected_list"] == value, (
"expected "
+ str(value)
+ " but got "
+ str(result["result"]["partial_unexpected_list"])
)

elif key == "details":
assert result["result"]["details"] == value

Expand Down
Expand Up @@ -213,7 +213,7 @@
},{
"title": "unexpected_values_exact_match_out_without_unexpected_index_list",
"exact_match_out" : true,
"only_for": ["sqlalchemy"],
"suppress_test_for": ["pandas"],
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@NathanFarmer Unfortunately, GitHub is still showing me the apparently incorrectly changed code, so reviewing the affected modules via screen sharing with the focus on what had to be changed would be great. Thanks!

"in": {
"column_list": ["a", "b"]
},
Expand Down