great_expectations icon indicating copy to clipboard operation
great_expectations copied to clipboard

[BUGFIX] issue-4295-fix-issue

Open YevgeniyaLee opened this issue 2 years ago • 3 comments

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:

  • fixed parsing partial_unexpected_count if there are unexpected items

After submitting your PR, CI checks will run and @cla-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).

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 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.

Thank you for submitting!

YevgeniyaLee avatar Oct 17 '22 09:10 YevgeniyaLee

Deploy Preview for niobium-lead-7998 ready!

Name Link
Latest commit a268e3016f6f2ff625fb1b2c0ea97a8c2fdfc05b
Latest deploy log https://app.netlify.com/sites/niobium-lead-7998/deploys/63850faad2c49f00087fef9a
Deploy Preview https://deploy-preview-6164--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.

netlify[bot] avatar Oct 17 '22 09:10 netlify[bot]

Thank you so much for the contribution @YevgeniyaLee! We will review and get back to you.

anthonyburdi avatar Oct 17 '22 21:10 anthonyburdi

Hi @YevgeniyaLee - I commented also on #4295 but I'm having a hard time replicating the issue after @joshua-stauffer's fix in #4336. I tried the following: https://github.com/great-expectations/great_expectations/compare/b/dx-9/testing_if_issue_still_exists. Your PR is great, but would you mind helping me understand better what the underlying issue is that it is fixing? Or if you would like to contribute a test as well that replicates the issue, then we can make sure it stays fixed :)

anthonyburdi avatar Oct 18 '22 19:10 anthonyburdi

hello, @anthonyburdi , sorry for late reply, just managed to return back to this PR. I reproduced the issue for spark engine and put it here, not sure if it is the proper place. Excuse me, this is my first experience of open source contribution. I assume the problem occurs here, when trying to get most common unexpected error and it might be inside dictionary. And Counter object rejects unhashaeble object so I added implementation of dictionary with hash method. If it goes about testing - agree with you, I'll try to provide them but I'd like to know your opinion: do you agree that issue is still actual and the way I fixed is ok for you?

YevgeniyaLee avatar Oct 21 '22 19:10 YevgeniyaLee

No problem @YevgeniyaLee! And thank you for your work on this - you are absolutely right that there is an issue, thank you for reproducing it! I wanted to check to see if it affected the v3 api so I added a few more examples in this gist. It looks like the issue doesn't appear in v3 but we should fix it in v2. I would like to approve your fix and close #4295, but first would you be willing to contribute a test to ensure we don't re-introduce this issue? I think a good place for the test would be tests/expectations/test_format_map_output.py.

Also it looks like you'll need to run the automated linter - from the repository root you can run all of our hooks by running invoke hooks which will run black, isort, etc.

Thank you again, this is a great find :)

hello, @anthonyburdi , sorry for late reply, just managed to return back to this PR. I reproduced the issue for spark engine and put it here, not sure if it is the proper place. Excuse me, this is my first experience of open source contribution. I assume the problem occurs here, when trying to get most common unexpected error and it might be inside dictionary. And Counter object rejects unhashaeble object so I added implementation of dictionary with hash method. If it goes about testing - agree with you, I'll try to provide them but I'd like to know your opinion: do you agree that issue is still actual and the way I fixed is ok for you?

anthonyburdi avatar Oct 26 '22 20:10 anthonyburdi

Hi @YevgeniyaLee - I had a chance to look back at this and am realizing the UnexpectedItem class will need to be adjusted to handle non-dictionary args. For example, I am seeing this error in our CI when trying to initialize with an int because of the subsequent call to update:

self = <[AttributeError("'UnexpectedItem' object has no attribute 'data'") raised in repr()] UnexpectedItem object at 0x7f947a4dd310>
args = (2,), kwargs = {}, tmp = 2

    def __init__(self, *args, **kwargs) -> None:
        if args[0]:
            tmp = args[0]
>           tmp.update(**kwargs)
E           AttributeError: 'int' object has no attribute 'update'

great_expectations/data_asset/data_asset.py:1181: AttributeError

Would you be able to take a look at this?

anthonyburdi avatar Nov 28 '22 17:11 anthonyburdi

Actually - in looking at our code for v3 we already have a fix in place that we can replicate here, https://github.com/great-expectations/great_expectations/blob/develop/great_expectations/expectations/expectation.py#L3023-L3064. What are your thoughts on implementing this instead @YevgeniyaLee?

anthonyburdi avatar Nov 28 '22 17:11 anthonyburdi