-
Notifications
You must be signed in to change notification settings - Fork 4.1k
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
Fix SAT failing on similar nested strucutres with different order. #7491
Conversation
airbyte-integrations/bases/source-acceptance-test/source_acceptance_test/tests/test_core.py
Show resolved
Hide resolved
/publish connector=bases/source-acceptance-test
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice, like this approach. One concern on persisting hash value in object in my comment
@@ -52,13 +51,20 @@ def diff_dicts(left, right, use_markup) -> Optional[List[str]]: | |||
|
|||
|
|||
@functools.total_ordering | |||
class DictWithHash(dict): | |||
class HashMixin: | |||
_hash = None |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We shouldn't be persisting the hash value in the object because this Mixin is designed to be used with mutable objects (dict and list). e.g.
a = DictWithHashMixin( {"one": 1} )
b = DictWithHashMixin( {"one": 1} )
a == b # this will correctly be True
a["two"] = 2
a == b # this will now INCORRECTLY be True, because we're comparing the same saved hash as before
# meaning hash(a) == hash(b) when it should be different
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I was thinking on this too, but this just an utility function used by test and test data not supposed to be changed, so I choose optimization.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Based on current usage, yes... but ultimately this PR implements a new Dict and List that don't update their hash after being mutated. For obvious reasons that behaviour would not be assumed by a developer in the future utilising these and we could end up with problems. I think we should therefore always recompute.
I would be very surprised if performance from Python's hash function became limiting.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this make sense, updated PR
airbyte-integrations/bases/source-acceptance-test/source_acceptance_test/tests/test_core.py
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great, thanks!
/publish connector=bases/source-acceptance-test
|
What
Resolves #7487