great_expectations
great_expectations copied to clipboard
[BUGFIX] issue-4295-fix-issue
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!
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...Use your smartphone camera to open QR code link. |
To edit notification comments on pull requests, go to your Netlify site settings.
Thank you so much for the contribution @YevgeniyaLee! We will review and get back to you.
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 :)
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?
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?
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?
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?