[BUGFIX] fix incorrect pandas top rows usage #3091
Conversation
…incorrect_pandas_top_rows_usage-2021_07-21-22
|
✔️ Deploy Preview for knoxpod ready! 🔨 Explore the source changes: 3d5e065 🔍 Inspect the deploy log: https://app.netlify.com/sites/knoxpod/deploys/60f89377fcc10500084ad83c 😎 Browse the preview: https://deploy-preview-3091--knoxpod.netlify.app |
HOWDY! This is your friendly 🤖 CHANGELOG bot 🤖Please don't forget to add a clear and succinct description of your change under the Develop header in ✨ Thank you! ✨ |
alexsherstinsky
requested review from
jcampbell,
cdkini,
fjork3,
petermoyer and
Shinnnyshinshin
…incorrect_pandas_top_rows_usage-2021_07-21-22
There was a problem hiding this comment.
I reviewed from a purely Python perspective since my knowledge of the underlying systems is lacking. I see the pandas related issue and see how it's resolved so as long as tests pass, I'll sign off.
Please do take a look at my comments (non-blocking) and let me know your thoughts.
| domain_values = df[column_name] | ||
|
|
||
| domain_values = domain_values[boolean_mapped_unexpected_values == True] |
There was a problem hiding this comment.
Can we consolidate this into a single filtered df? Is that too complex to put on a single line?
|
|
||
| if result_format["result_format"] == "COMPLETE": |
There was a problem hiding this comment.
Is there any chance of result_format missing the "result_format" key? Should we make this a get()?
| list(domain_values), | ||
| list(map_series), | ||
| ) |
There was a problem hiding this comment.
Thoughts on using df.columns.tolist() or df.columns.values.tolist()? The latter is more performant but I'm just thinking from a readability standpoint. I wasn't sure what list(df) did but that might just be from not having used pandas in a while.
| unexpected_rows_metric = MetricConfiguration( | ||
| metric_name="column_values.unique.unexpected_rows", | ||
| metric_domain_kwargs={"column": "a"}, | ||
| metric_value_kwargs={ | ||
| "result_format": {"result_format": "SUMMARY", "partial_unexpected_count": 1} | ||
| }, | ||
| metric_dependencies={ | ||
| "unexpected_condition": condition_metric, | ||
| "table.columns": table_columns_metric, | ||
| }, | ||
| ) | ||
| results = engine.resolve_metrics( | ||
| metrics_to_resolve=(unexpected_rows_metric,), metrics=metrics | ||
| ) | ||
| metrics.update(results) | ||
|
|
||
| assert metrics[unexpected_rows_metric.id]["a"].index == [2] | ||
| assert metrics[unexpected_rows_metric.id]["a"].values == [3] | ||
|
|
There was a problem hiding this comment.
What is this test doing? Does it add coverage over this pandas issue that was not covered before? Should it be a separate test entirely or are we okay with appending it here?
I don't have the requisite knowledge to definitively say but I thought it was prudent to bring up such questions so you could evaluate.
There was a problem hiding this comment.
This tests goes into a metric and the result format that triggers the behavior, where the number of rows returned from the pandas dataframe has to be less than its total number of rows.
alexsherstinsky
deleted the
bugfix/GE-320/GE-364/alexsherstinsky/fix_incorrect_pandas_top_rows_usage-2021_07-21-22
branch
…https://github.com/great-expectations/great_expectations into docs/GDOC-199/snowflake-connections-closed-correctly * 'docs/GDOC-199/snowflake-connections-closed-correctly' of https://github.com/great-expectations/great_expectations: Update util.py [MAINTENANCE] rename map_metric.py to map_metric_provider.py (with DeprecationWarning) for a better code readability/interpretability (#3103) [MAINTENANCE] rename ColumnMetricProvider to ColumnAggregateMetricProvider (with DeprecationWarning) (#3100) use correct variable name (#3069) [RELEASE] release candidate for 2021-07-22 (#3101) [FEATURE] SqlAlchemy engine support for column.most_common_value metric (#3020) [BUGFIX] Fix run_diagnostics for contrib expectations (#3096) [BUGFIX] Fix typos discovered by codespell (#3064) [DOCS] Migrating pages under guides/miscellaneous (#3094) [DOCS] Port over "How to run a Checkpoint in Airflow" from RTD to Docusaurus (#3074) disable snowflake tests temporarily (#3093) [BUGFIX] fix incorrect pandas top rows usage (#3091)
Please annotate your PR title to describe what the PR does, then give a brief bulleted description of your PR below. PR titles should begin with [BUGFIX], [FEATURE], [DOCS], or [MAINTENANCE]. If a new feature introduces breaking changes for the Great Expectations API or configuration files, please also add [BREAKING]. You can read about the tags in our contributor checklist.
Changes proposed in this pull request:
After submitting your PR, CI checks will run and @tiny-tim-bot will check for your CLA signature.
For a PR with nontrivial changes, we review with both design-centric and code-centric lenses.
In a design review, we aim to ensure that the PR is consistent with our relationship to the open source community, with our software architecture and abstractions, and with our users' needs and expectations. That review often starts well before a PR, for example in github issues or slack, so please link to relevant conversations in notes below to help reviewers understand and approve your PR more quickly (e.g.
closes #123).Previous Design Review notes:
Definition of Done
Please delete options that are not relevant.
Thank you for submitting!