ENH:included anti join functionality #43056

debnathshoham · 2021-08-15T18:10:49Z

closes ENH: Add anti-joins to pandas.merge #42916
tests added / passed
Ensure all linting tests pass, see here for how to run them
whatsnew entry

Extremely naive implementation. Happy to take suggestions.

pandas/tests/reshape/merge/test_merge.py

attack68 · 2021-08-18T16:35:34Z

I haven't reviewed fully but my instinct is telling me that you have implemented this in a very complicated way, and I think a search/consideration for a much simpler approach is worthwhile. (see my comment in the underlying issue).

Additionally, your solution uses numpy and numpy array functions. Does that mean this functionality might fail with dtypes that are native to pandas?

debnathshoham · 2021-08-18T19:00:19Z

Hi @attack68 , this #42916 (comment) was really helpful. I have made some changes, let me know how this looks. I believe this should be able to handle pandas dtypes.

jreback

looks good, some comments. needs some tests with other dtypes (datetime, datetime w/timezone, categorical and other EAs), doesnt' have to be on all parameterization a separate test is ok.

update docs where needed (doc-string & in merge.rst)

pandas/core/reshape/merge.py

jreback · 2021-08-19T00:32:30Z

pandas/core/reshape/merge.py

@@ -744,6 +747,57 @@ def get_result(self) -> DataFrame:

        return result.__finalize__(self, method="merge")

+    def _anti_join_update(self):


ok with this being a method on _MergeOperation but it should have NO side effects, e.g. simply return left, right and assign at this level.

not sure if I understand what you mean by, being a method on _MergeOperation.

this needs to execute in the get_result() portion, NOT during validation. validation shouldn't dot he actual computation.

IOW during _get_join_info could dispatch based on how.

the actual calculation is still happening in get_result().
I have just changed the configuration of left, right and how to utilise the already existing joining methods. E.g. For anti_left I have changed the left, right and how to left such that left join would give the result

hi @jreback - wondering if you got a chance to look at this? and if this implementation is fine?

no the implementation is quite convoluted

i am not sure how to fix what u have here

pandas/tests/reshape/merge/test_merge_anti.py

debnathshoham · 2021-08-19T14:17:17Z

This is rendering little odd. I had to split it into two lines due to pep8 length limitation, but not sure how to ensure the render is in a single line.

how : {'left', 'right', 'outer', 'inner', 'cross',
  'anti_left', 'anti_right', 'anti_full'}, default 'inner'

pandas/tests/reshape/merge/test_merge_anti.py

simplified test_basic_anti_index (thx to @attck68) Co-authored-by: attack68 <[email protected]>

pep8speaks · 2021-08-20T07:50:02Z

Hello @debnathshoham! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻

Comment last updated at 2022-01-14 18:16:37 UTC

debnathshoham · 2021-09-19T10:37:52Z

hi guys, just checking if anyone got a chance to look into this..

attack68

The actual mechanics of this and the coding structure look good to me - I think it has been through a few iterations and is in a clean state now.

Since this is fairly complex logic, i.e. you are taking the anti-join input and manipulating it into a structure that can then process the join as usual having been pre-processed, I would, personally, add better comments and documentation, explaining the functions and different cases.

Additionally, the tests, even though they look a little unwieldy, are thorough, but there are no comments here either and I think they would be quite helpful just indicating the purpose of the test.

I would also add a sentence describing the examples you gave in the documentation.

debnathshoham · 2021-09-24T19:12:37Z

thanks @attack68 for the detailed review.
I have added a couple of more comments, would be happy to add more, if you can point somewhere you think is still unclear).

debnathshoham · 2021-09-24T19:30:48Z

I have added a couple of comments in the tests too. In the rest, I think the name of the tests are self-descriptive.

attack68

Im pretty happy with this as is, but I do think it can be streamlined.

pandas/core/reshape/merge.py

attack68 · 2021-09-25T20:00:21Z

pandas/tests/reshape/merge/test_merge_anti.py

+            ),
+        ],
+    )
+    def test_anti_with_nan(self, expected, how, on, left_on, right_on):


what is this test doing that the other tests above do not capture?

this was for mixed dtypes.
(added couple of more comments for these)

attack68 · 2021-09-25T20:03:06Z

pandas/tests/reshape/merge/test_merge_anti.py

+            ),
+        ],
+    )
+    def test_anti_with_nan_and_NA(self, expected, how, left_on, right_on):


Is this test purely testing pd.NA?

In this case it seems overkill. Logically speaking if the tests above imply everything is working for nan, and a simple test shows pd.Na works equivalently to nan then all tests above should work for pd.NA so we don't have to repeat everything again.

actually this was to check np.nan is not matched with pd.NA.
(added comment)

pandas/tests/reshape/merge/test_merge_anti.py

attack68 · 2021-09-25T20:12:01Z

pandas/core/reshape/merge.py

+        elif self.on is not None or (
+            None not in self.left_on and None not in self.right_on
+        ):
+            # Merge using `on` or `left_on` and `right_on`


to be honest these comments are obvious, as you said. it was more the general concept that you could have expanded in the main docstring, why you have implemented it this way and what is the advantage. Just in case you come back to look at it after two years, or another developer comes in and tries to figure out what you are doing.

the comments in merge.py looks sufficient to me. But again, I wrote it and a second pair of eyes can give a fresh perspective.
Do you think the logic in merge.py is too difficult to follow? (if you could point to any specific portion)

jreback

this needs to follow the existing workflow. as-is its very hard to follow what you are trying to do. the pattern is validate the merge, then actually do the operation. you have mixed the two.

jreback · 2021-09-25T21:23:17Z

pandas/core/reshape/merge.py

@@ -744,6 +747,57 @@ def get_result(self) -> DataFrame:

        return result.__finalize__(self, method="merge")

+    def _anti_join_update(self):


this needs to execute in the get_result() portion, NOT during validation. validation shouldn't dot he actual computation.

IOW during _get_join_info could dispatch based on how.

folder/subfolder/out.csv

Co-authored-by: JHM Darbyshire <[email protected]>

This reverts commit 411bcaa.

github-actions · 2021-11-07T00:03:40Z

This pull request is stale because it has been open for thirty days with no activity. Please update or respond to this comment if you're still interested in working on this.

jreback · 2021-11-28T21:18:58Z

conceptually ok with adding this. but the implementation needs tight integration with the existing. you are bolting it on in an unmaintainable manner. if you want to continue, merge master and update.

mroeschke · 2022-02-13T00:49:28Z

Thanks for the PR, but it appears to have gone stale. Feel free to reopen if you have time to continue with addressing the review (splitting the validate and merge operations).

ENH:included anti join functionality

fba0bb5

phofl reviewed Aug 16, 2021

View reviewed changes

pandas/tests/reshape/merge/test_merge.py Outdated Show resolved Hide resolved

simonjayhawkins added Enhancement Reshaping Concat, Merge/Join, Stack/Unstack, Explode labels Aug 16, 2021

included multicol join

53fd41d

debnathshoham marked this pull request as draft August 16, 2021 18:56

debnathshoham added 5 commits August 17, 2021 00:35

Merge branch 'master' into 42916

627c31e

handling index and col

448373b

added test on nan

4dd802d

Merge branch 'master' into 42916

3b17c59

removed tests cases with warning

6e3d1a4

debnathshoham marked this pull request as ready for review August 17, 2021 14:38

phofl reviewed Aug 18, 2021

View reviewed changes

pandas/tests/reshape/merge/test_merge.py Outdated Show resolved Hide resolved

seperated antijoin tests to another file

6427f09

replaced np funcs with pd

86ddac9

added test with pd.NA

84294e4

jreback requested changes Aug 19, 2021

View reviewed changes

debnathshoham added 2 commits August 19, 2021 13:33

suggested changes

43ae0a1

added tests covering Categorcal, EA, datetime,datetime w tz, EA+multicol

c36705c

debnathshoham requested review from jreback, phofl and attack68 August 19, 2021 13:45

attack68 reviewed Aug 20, 2021

View reviewed changes

pandas/tests/reshape/merge/test_merge_anti.py Outdated Show resolved Hide resolved

Update pandas/tests/reshape/merge/test_merge_anti.py

951406a

simplified test_basic_anti_index (thx to @attck68) Co-authored-by: attack68 <[email protected]>

debnathshoham added 2 commits August 20, 2021 13:29

formatted with black

db80abf

changed a few test setup

d93c0ac

debnathshoham added 2 commits September 10, 2021 20:58

Merge branch 'master' into 42916

ee7cc16

Merge branch 'master' into 42916

76cd5c6

attack68 requested changes Sep 24, 2021

View reviewed changes

debnathshoham added 2 commits September 25, 2021 00:13

Merge branch 'master' into 42916

d358efc

more comments

14d0d4c

added in merge.rst

3fe64f4

debnathshoham added 2 commits September 25, 2021 16:40

removed comments from example; failing doctests

fc50027

reversed mm dd order in test_anti_datetime_tz to prevent UserWarning

aba9a30

attack68 requested changes Sep 25, 2021

View reviewed changes

jreback requested changes Sep 25, 2021

View reviewed changes

attack68 reviewed Sep 26, 2021

View reviewed changes

folder/subfolder/out.csv Outdated Show resolved Hide resolved

debnathshoham and others added 8 commits September 26, 2021 16:35

Update pandas/core/reshape/merge.py

411bcaa

Co-authored-by: JHM Darbyshire <[email protected]>

Delete out.csv

417ea13

Revert "Update pandas/core/reshape/merge.py"

09426c6

This reverts commit 411bcaa.

removed files added by mistake

b6e72aa

more comments on tests

8338a48

Merge branch 'master' into 42916

cc6c8ea

Merge branch 'master' into 42916

f33fe48

Merge branch 'master' into 42916

74e172b

github-actions bot added the Stale label Nov 7, 2021

debnathshoham added 2 commits December 15, 2021 22:10

Merge branch 'master' into 42916

594f80a

Merge branch 'main' of https://github.com/pandas-dev/pandas into 42916

f395cbb

mroeschke closed this Feb 13, 2022

debnathshoham mentioned this pull request Oct 27, 2022

ENH: anti joins #49328

Closed

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ENH:included anti join functionality #43056

ENH:included anti join functionality #43056

debnathshoham commented Aug 15, 2021

attack68 commented Aug 18, 2021

debnathshoham commented Aug 18, 2021

jreback left a comment

jreback Aug 19, 2021

debnathshoham Aug 19, 2021

jreback Sep 25, 2021

debnathshoham Sep 26, 2021

debnathshoham Oct 4, 2021

jreback Oct 4, 2021

debnathshoham commented Aug 19, 2021

pep8speaks commented Aug 20, 2021 •

edited

Loading

debnathshoham commented Sep 19, 2021

attack68 left a comment •

edited

Loading

debnathshoham commented Sep 24, 2021

debnathshoham commented Sep 24, 2021

attack68 left a comment

attack68 Sep 25, 2021

debnathshoham Sep 26, 2021

attack68 Sep 25, 2021

debnathshoham Sep 26, 2021 •

edited

Loading

attack68 Sep 25, 2021

debnathshoham Sep 26, 2021

jreback left a comment

jreback Sep 25, 2021

github-actions bot commented Nov 7, 2021

jreback commented Nov 28, 2021

mroeschke commented Feb 13, 2022

		@@ -744,6 +747,57 @@ def get_result(self) -> DataFrame:

		return result.__finalize__(self, method="merge")

		def _anti_join_update(self):

ENH:included anti join functionality #43056

ENH:included anti join functionality #43056

Conversation

debnathshoham commented Aug 15, 2021

attack68 commented Aug 18, 2021

debnathshoham commented Aug 18, 2021

jreback left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

debnathshoham commented Aug 19, 2021

pep8speaks commented Aug 20, 2021 • edited Loading

Comment last updated at 2022-01-14 18:16:37 UTC

debnathshoham commented Sep 19, 2021

attack68 left a comment • edited Loading

Choose a reason for hiding this comment

debnathshoham commented Sep 24, 2021

debnathshoham commented Sep 24, 2021

attack68 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

debnathshoham Sep 26, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jreback left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Nov 7, 2021

jreback commented Nov 28, 2021

mroeschke commented Feb 13, 2022

pep8speaks commented Aug 20, 2021 •

edited

Loading

attack68 left a comment •

edited

Loading

debnathshoham Sep 26, 2021 •

edited

Loading