BUG: right merge not preserve row order (#27453) #27762

bongolegend · 2019-08-05T19:13:16Z

closes Right merge not preserve order #27453
tests added / passed
passes black pandas
passes git diff upstream/master -u -- "*.py" | flake8 --diff
whatsnew entry

bongolegend · 2019-08-05T20:17:08Z

Not sure how to interpret these errors. Azure is reporting a failure on test_drop_duplicates_categorical_non_bool, but that is marked to xfail in the test code. Any advice appreciated. @TomAugspurger

WillAyd

Could you add a whatsnew for v1.0.0

pandas/tests/reshape/merge/test_merge.py

WillAyd · 2019-08-07T03:47:27Z

pandas/tests/reshape/merge/test_merge.py

+    assert_frame_equal(expected, result)
+
+
+def test_left_merge_preserves_row_order():


Is this not already accounted for in other tests?

None that I could find.

Instead of having these as separate tests can you just parametrize on how = left and right?

can you do this

also outer and inner should match left ordering

Sorry - yeah I can do this. Just started a new job so my schedule got crammed all of a sudden.

WillAyd · 2019-08-07T03:50:28Z

pandas/core/reshape/merge.py

@@ -1276,6 +1276,11 @@ def _get_join_indexers(left_keys, right_keys, sort=False, how="inner", **kwargs)
        indexers into the left_keys, right_keys

    """
+    _how = how


Hmm this is rather difficult to grok - could you alternately just change _right_outer_join within the same module to accept a sort keyword? I think that would make things easier

the sorting actually happens in _factorize_keys a few lines down, not downstream in _right_outer_merge. So the left and right keys need to be swapped before that call. _right_outer_join just implements _left_outer_join with left and right swapped; no sorting happens there.

I guess what I'm saying is that sorting already happens, but it happens before a distinction is made between left and right joins. So this PR doesn't move where the sorting happens, it just moves where the distinction between left and right joins is made.

TomAugspurger · 2019-08-12T18:45:50Z

Azure is reporting a failure on test_drop_duplicates_categorical_non_bool, but that is marked to xfail in the test code. Any advice appreciated.

That xfail references #7996 if it's any help.

The 3.7 CI failures should be fixed if you merge master & repush.

bongolegend · 2019-08-13T03:23:27Z

@WillAyd I cleaned up the sloppy logic here. Should be much better now.

bongolegend · 2019-08-19T22:38:56Z

The failing tests are unrelated to this PR.

=================================== FAILURES ===================================
_ TestCategoricalSeriesAnalytics.test_drop_duplicates_categorical_non_bool[True-datetime64[D]] _
[gw1] linux -- Python 3.7.3 /home/travis/miniconda3/envs/pandas-dev/bin/python
[XPASS(strict)] GH#7996
_ TestCategoricalSeriesAnalytics.test_drop_duplicates_categorical_non_bool[False-datetime64[D]] _
[gw1] linux -- Python 3.7.3 /home/travis/miniconda3/envs/pandas-dev/bin/python
[XPASS(strict)] GH#7996
_ TestCategoricalSeriesAnalytics.test_drop_duplicates_categorical_non_bool[None-datetime64[D]] _
[gw1] linux -- Python 3.7.3 /home/travis/miniconda3/envs/pandas-dev/bin/python
[XPASS(strict)] GH#7996

source

WillAyd

Sorry for the delayed response as I've been on vacation but this looks to be moving in the right direction!

WillAyd · 2019-08-24T07:56:21Z

pandas/core/reshape/merge.py


+    if how == "right":
+        rkey, lkey, count = fkeys(rkey, lkey)


Is there a more appropriate variable name to use here for the left hand sides? General approach looks good but I think variable reuse can be confusing for future debugging. OK with more appropriate names even if it increases diff

pandas/tests/reshape/merge/test_merge.py

WillAyd · 2019-08-24T07:58:00Z

pandas/tests/reshape/merge/test_merge.py

+    assert_frame_equal(expected, result)
+
+
+def test_left_merge_preserves_row_order():


Instead of having these as separate tests can you just parametrize on how = left and right?

doc/source/whatsnew/v1.0.0.rst

pandas/core/reshape/merge.py

pandas/tests/reshape/merge/test_merge.py

jreback · 2019-09-08T16:39:47Z

pandas/tests/reshape/merge/test_merge.py

+    assert_frame_equal(expected, result)
+
+
+def test_left_merge_preserves_row_order():


can you do this

jreback · 2019-09-08T16:39:58Z

pandas/tests/reshape/merge/test_merge.py

+    assert_frame_equal(expected, result)
+
+
+def test_left_merge_preserves_row_order():


also outer and inner should match left ordering

bongolegend · 2019-09-25T02:55:23Z

I addressed all the issues that were brought up. Ready for another round of review.

WillAyd

implementation and tests lgtm just need some updates on whatsnew

doc/source/whatsnew/v1.0.0.rst

WillAyd · 2019-10-11T21:50:17Z

Can you resolve merge conflict? Also should only need one whatsnew entry

bongolegend · 2019-10-13T18:03:11Z

Can you resolve merge conflict? Also should only need one whatsnew entry

AS per @jreback 's request, I have a whatsnew entry to both the breaking API changes and the Reshaping sections.

pep8speaks · 2019-10-13T18:04:29Z

Hello @ncernek! 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 2020-01-23 03:21:01 UTC

WillAyd · 2019-10-14T15:28:29Z

AS per @jreback 's request, I have a whatsnew entry to both the breaking API changes and the Reshaping sections.

I think the request there was to have this show up in Backwards Incompatible API Changes with its own section, not to duplicate it. You can find examples of how to do that in prior whatsnew notes, like here:

https://github.com/pandas-dev/pandas/blob/master/doc/source/whatsnew/v0.25.0.rst#backwards-incompatible-api-changes

WillAyd · 2019-10-14T15:28:45Z

Also looks like a few lint issues need to be addressed

bongolegend · 2019-10-24T02:04:50Z

Does anyone understand what the error is here? This should be good to merge.

WillAyd · 2019-10-25T16:36:58Z

This should be good to merge.

Still need to address the comment around the whatsnew. Please take a look at #27762 (comment)

The existing CI failure will probably go away on next push - we've been having intermittent timeouts the past few days that I think should be over now

bongolegend · 2019-11-11T00:55:36Z

Still need to address the comment around the whatsnew. Please take a look at #27762 (comment)

I already took care of this is in this commit.

TomAugspurger · 2019-11-12T17:25:19Z

@ncernek some of the CI failures look related. e.g. https://dev.azure.com/pandas-dev/pandas/_build/results?buildId=20073

WillAyd · 2019-12-09T18:09:53Z

@ncernek any chance you can check CI failures and resolve conflicts?

bongolegend · 2019-12-09T20:00:16Z

hey, yeah I can take a look later this week.

On Mon, Dec 9, 2019 at 11:09 William Ayd ***@***.***> wrote: @ncernek <https://github.com/ncernek> any chance you can check CI failures and resolve conflicts? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#27762?email_source=notifications&email_token=ADNDK64BEGE53SA6QDMEXNDQX2CXHA5CNFSM4IJOCUZKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEGKDLAQ#issuecomment-563361154>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ADNDK6Z5SI6IHYX4GWLLN7LQX2CXHANCNFSM4IJOCUZA> .

-- - 312.450.5311

TomAugspurger · 2019-12-30T18:40:55Z

@ncernek able to update? We're planning to release the 1.0 rc this week.

Co-Authored-By: William Ayd <william.ayd@icloud.com>

bongolegend · 2020-01-01T02:28:44Z

Hmm I'm getting an error locally when running tests. Any ideas?

╰─$ pytest pandas/tests/reshape/merge/test_merge.py::test_merge_preserves_row_order                                      1 ↵
ImportError while loading conftest '/Users/nico/code/pandas-ncernek/pandas/conftest.py'.
pandas/__init__.py:56: in <module>
    from pandas.core.api import (
pandas/core/api.py:4: in <module>
    from pandas._libs.missing import NA
E   ImportError: cannot import name 'NA' from 'pandas._libs.missing' (/Users/nico/code/pandas-ncernek/pandas/_libs/missing.cpython-37m-darwin.so)

jreback

looks pretty good, can you merge master, ping on green.

jreback · 2020-01-01T17:54:01Z

doc/source/whatsnew/v1.0.0.rst

@@ -274,6 +274,7 @@ New repr for :class:`~pandas.arrays.IntervalArray`
 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

 - :class:`pandas.arrays.IntervalArray` adopts a new ``__repr__`` in accordance with other array classes (:issue:`25022`)
+- :class:`pandas.core.arrays.IntervalArray` adopts a new ``__repr__`` in accordance with other array classes (:issue:`25022`)


looks like a rebasing dupe

jreback · 2020-01-01T17:54:26Z

doc/source/whatsnew/v1.0.0.rst

@@ -292,6 +293,32 @@ New repr for :class:`~pandas.arrays.IntervalArray`

   pd.arrays.IntervalArray.from_tuples([(0, 1), (2, 3)])

+- :meth:`DataFrame.merge` now preserves right frame's row order when executing a right merge (:issue:`27453`)


make this a sub-section (need a line under the title), and put a shorter title, move what you have to the first sentence.

jreback · 2020-01-01T17:55:08Z

doc/source/whatsnew/v1.0.0.rst

+
+.. ipython:: python
+
+    left_df = pd.DataFrame({"colors": ["blue", "red"]}, index=pd.Index([0, 1]))


show these as well, call then just left and rigth

jreback · 2020-01-01T17:55:21Z

doc/source/whatsnew/v1.0.0.rst

+
+*pandas 0.25.x*
+
+.. ipython:: python


this needs to be a code-block

jreback · 2020-01-01T17:55:36Z

doc/source/whatsnew/v1.0.0.rst

+
+.. ipython:: python
+
+    left_df.merge(right_df, left_index=True, right_index=True, how="right")


let this execute (don't put the results in)

jreback · 2020-01-01T17:55:52Z

doc/source/whatsnew/v1.0.0.rst

@@ -949,6 +976,7 @@ Reshaping
 - Bug in :func:`melt` where supplying mixed strings and numeric values for ``id_vars`` or ``value_vars`` would incorrectly raise a ``ValueError`` (:issue:`29718`)
 - Dtypes are now preserved when transposing a ``DataFrame`` where each column is the same extension dtype (:issue:`30091`)
 - Bug in :func:`merge_asof` merging on a tz-aware ``left_index`` and ``right_on`` a tz-aware column (:issue:`29864`)
+- :meth:`DataFrame.merge` now preserves right frame's row order when executing a right merge (:issue:`27453`)


don't put this here, you already have a sub-section

MarcoGorelli · 2020-01-15T18:32:35Z

Hmm I'm getting an error locally when running tests. Any ideas?

╰─$ pytest pandas/tests/reshape/merge/test_merge.py::test_merge_preserves_row_order                                      1 ↵
ImportError while loading conftest '/Users/nico/code/pandas-ncernek/pandas/conftest.py'.
pandas/__init__.py:56: in <module>
    from pandas.core.api import (
pandas/core/api.py:4: in <module>
    from pandas._libs.missing import NA
E   ImportError: cannot import name 'NA' from 'pandas._libs.missing' (/Users/nico/code/pandas-ncernek/pandas/_libs/missing.cpython-37m-darwin.so)

Can you try rebuilding the C extensions?

bongolegend · 2020-01-23T03:48:40Z

I'm not working on this PR anymore. This dev experience is painful. I would have appreciated commits from others to help me get across the finish line a long time ago, esp. when the only issues remaining for the past few months were concerning documentation.

Separately, uncertain as to why there are hundreds of test failures. These aren't caused by my contribution that I can tell. That doesn't leave me excited to resolve anything anymore.

Anyone else is free to take ownership of this PR.

MarcoGorelli · 2020-01-23T17:48:10Z

I'm not working on this PR anymore.

That's OK, thanks for letting us know :)

Separately, uncertain as to why there are hundreds of test failures. These aren't caused by my contribution that I can tell. That doesn't leave me excited to resolve anything anymore.

For a start, it looks like you're calling assert_frame_equal(expected, result) (should be tm.assert_frame_equal(expected, result)), which is causing a NameError

Anyone else is free to take ownership of this PR.

Sure, will do

WillAyd requested changes Aug 7, 2019

View reviewed changes

WillAyd added the Reshaping Concat, Merge/Join, Stack/Unstack, Explode label Aug 7, 2019

WillAyd added this to the 1.0 milestone Aug 7, 2019

bongolegend force-pushed the 27453-right-merge-order branch from 46fc6ed to 9007839 Compare August 12, 2019 21:21

stuarteberg mentioned this pull request Aug 14, 2019

COMPAT: unique() should preserve the dtype of the input #27874

Merged

5 tasks

WillAyd requested changes Aug 24, 2019

View reviewed changes

jreback requested changes Sep 8, 2019

View reviewed changes

bongolegend force-pushed the 27453-right-merge-order branch from 3f6d119 to a97763a Compare September 21, 2019 12:51

WillAyd requested changes Sep 25, 2019

View reviewed changes

doc/source/whatsnew/v1.0.0.rst Outdated Show resolved Hide resolved

doc/source/whatsnew/v1.0.0.rst Outdated Show resolved Hide resolved

bongolegend force-pushed the 27453-right-merge-order branch from f6bf25a to 258df2e Compare January 1, 2020 01:30

Nico Cernek added 3 commits December 31, 2019 20:14

add failing test to check row order preservation

c3565c7

correct the imports

0e481a7

change order of args in test assertion

42eaad1

Nico Cernek and others added 16 commits December 31, 2019 20:15

add test for left merge

803243c

swap left and right keys when how == "right"

cc269f3

correct old test: right-merge row order is now the same as the right df

f2249df

clean up spacing and delete temp code

5f728a4

add whatsnew

7ccebb2

replace .from_records with default constructor

90984e4

add GH issue # to tests

de44eae

revert commit ed54bec

a8954fe

change logic to swap left and right if how==right

d328470

clean formatting

3c1a7cb

rename vars and add comment for clarity

7fc6441

combine tests into one

1e9df2b

update whatsnew

d017bd5

Update doc/source/whatsnew/v1.0.0.rst

71736b8

Co-Authored-By: William Ayd <william.ayd@icloud.com>

add before and after examples

11ec7d0

linting

c238b50

bongolegend force-pushed the 27453-right-merge-order branch from 258df2e to c238b50 Compare January 1, 2020 02:19

cleanup

e29ed1c

jreback requested changes Jan 1, 2020

View reviewed changes

jreback removed this from the 1.0 milestone Jan 5, 2020

Nico Cernek added 3 commits January 22, 2020 20:03

changes requested by jreback

2b1b675

update docs

ef14ba0

Merge branch 'master' into 27453-right-merge-order

978f91f

bongolegend closed this Jan 23, 2020

MarcoGorelli mentioned this pull request Jan 24, 2020

BUG: 27453 right merge order #31278

Merged

5 tasks

jorisvandenbossche mentioned this pull request Jul 8, 2020

Check API changes in whatsnew #34801

Closed

		assert_frame_equal(expected, result)


		def test_left_merge_preserves_row_order():

		@@ -292,6 +293,32 @@ New repr for :class:`~pandas.arrays.IntervalArray`

		pd.arrays.IntervalArray.from_tuples([(0, 1), (2, 3)])

		- :meth:`DataFrame.merge` now preserves right frame's row order when executing a right merge (:issue:`27453`)


		.. ipython:: python

		left_df = pd.DataFrame({"colors": ["blue", "red"]}, index=pd.Index([0, 1]))


		.. ipython:: python

		left_df.merge(right_df, left_index=True, right_index=True, how="right")

BUG: right merge not preserve row order (#27453) #27762

BUG: right merge not preserve row order (#27453) #27762

Conversation

bongolegend commented Aug 5, 2019

bongolegend commented Aug 5, 2019 • edited Loading

WillAyd 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TomAugspurger commented Aug 12, 2019

bongolegend commented Aug 13, 2019

bongolegend commented Aug 19, 2019

WillAyd 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

bongolegend commented Sep 25, 2019

WillAyd left a comment

Choose a reason for hiding this comment

WillAyd commented Oct 11, 2019

bongolegend commented Oct 13, 2019

pep8speaks commented Oct 13, 2019 • edited Loading

Comment last updated at 2020-01-23 03:21:01 UTC

WillAyd commented Oct 14, 2019

WillAyd commented Oct 14, 2019

bongolegend commented Oct 24, 2019

WillAyd commented Oct 25, 2019

bongolegend commented Nov 11, 2019

TomAugspurger commented Nov 12, 2019

WillAyd commented Dec 9, 2019

bongolegend commented Dec 9, 2019 via email

TomAugspurger commented Dec 30, 2019

bongolegend commented Jan 1, 2020

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

MarcoGorelli commented Jan 15, 2020

bongolegend commented Jan 23, 2020

MarcoGorelli commented Jan 23, 2020 • edited Loading

bongolegend commented Aug 5, 2019 •

edited

Loading

pep8speaks commented Oct 13, 2019 •

edited

Loading

MarcoGorelli commented Jan 23, 2020 •

edited

Loading