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

[BUGFIX] ID/PK - Rendering ColumnPair and MultiColumn Expectations in DataDocs #7041

Conversation

Shinnnyshinshin
Copy link
Contributor

@Shinnnyshinshin Shinnnyshinshin commented Feb 3, 2023

What is in this PR?

  • _convert_unexpected_indices_to_df() has been refactored to take in domain column list, and generate counts, so there is no need to map back to partial_unexpected_counts dict.
  • build_count_and_index_table() has been refactored to use the DF outputted by _convert_unexpected_indices_to_df().
  • Tests have been added to test_util.py to test MultiColumnExpectation and ColumnPairExpectation.

Definition of Done

Please delete options that are not relevant.

  • My code follows the Great Expectations style guide
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • I have added unit tests where applicable and made sure that new and existing tests are passing.
  • I have run any local integration tests and made sure that nothing is broken.

@netlify
Copy link

netlify bot commented Feb 3, 2023

Deploy Preview for niobium-lead-7998 ready!

Name Link
🔨 Latest commit a98ed97
🔍 Latest deploy log https://app.netlify.com/sites/niobium-lead-7998/deploys/63dd4001e49c1d0009b5e7c2
😎 Deploy Preview https://deploy-preview-7041--niobium-lead-7998.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

@ghost
Copy link

ghost commented Feb 3, 2023

👇 Click on the image for a new way to code review
  • Make big changes easier — review code in small groups of related files

  • Know where to start — see the whole change at a glance

  • Take a code tour — explore the change with an interactive tour

  • Make comments and review — all fully sync’ed with github

    Try it now!

Review these changes using an interactive CodeSee Map

Legend

CodeSee Map Legend

@Shinnnyshinshin Shinnnyshinshin marked this pull request as ready for review February 3, 2023 06:00
@Shinnnyshinshin Shinnnyshinshin self-assigned this Feb 3, 2023
@@ -103,6 +103,7 @@ def _prescriptive_renderer(
runtime_configuration: Optional[dict] = None,
**kwargs,
) -> None:
# TODO: Need for prescriptive renderer for Expectation if we want to render in DataDocs.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Found this Expectation does not actually have any renderers implemented. Added TODO

Copy link
Member

@anthonyburdi anthonyburdi left a comment

Choose a reason for hiding this comment

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

LGTM! Thank you for fixing this. I think the new implementation is great and leverages pandas for the heavy lifting.

@Shinnnyshinshin Shinnnyshinshin enabled auto-merge (squash) February 3, 2023 16:48
@Shinnnyshinshin Shinnnyshinshin merged commit 3a60721 into develop Feb 3, 2023
@Shinnnyshinshin Shinnnyshinshin deleted the b/dx-244/dx-279/column-pair-and-multi-column-rendering branch February 3, 2023 17:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants