Skip to content
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.

Sign up for GitHub

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

Jump to bottom

BUG: 27453 right merge order #31278

Merged
merged 15 commits into from
Mar 26, 2020
Merged

BUG: 27453 right merge order #31278

merged 15 commits into from
Mar 26, 2020

Conversation

MarcoGorelli
Copy link
Member

@MarcoGorelli MarcoGorelli commented Jan 24, 2020 •
edited
Loading

Resurrection of #27762

I fixed the NameError in the test, and also changed the example given in the whatsnew entry (I couldn't see a difference between the outputs of 0.25.x and 1.0.0 in the original)

@WillAyd
Copy link
Member

WillAyd commented Jan 24, 2020

lgtm. not sure of the 1.0 milestone at this point but let's see what @TomAugspurger and @jreback think

@WillAyd WillAyd added the Reshaping Concat, Merge/Join, Stack/Unstack, Explode label Jan 24, 2020
MarcoGorelli
MarcoGorelli commented Jan 24, 2020
View reviewed changes
doc/source/whatsnew/v1.0.0.rst Outdated
@@ -1156,6 +1183,7 @@ Reshaping
- Bug in :func:`merge_asof` merging on a tz-aware ``left_index`` and ``right_on`` a tz-aware column (:issue:`29864`)
- Improved error message and docstring in :func:`cut` and :func:`qcut` when `labels=True` (:issue:`13318`)
- Bug in missing `fill_na` parameter to :meth:`DataFrame.unstack` with list of levels (:issue:`30740`)
- :meth:`DataFrame.merge` now preserves right frame's row order when executing a right merge (:issue:`27453`)
Copy link
Member Author

@MarcoGorelli MarcoGorelli Jan 24, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just noticed this had been requested to be removed in the original PR (will wait for a decision as to whether to include this in v1 before putting it through another round of CI)

@TomAugspurger
Copy link
Contributor

TomAugspurger commented Jan 25, 2020 via email

@MarcoGorelli
Copy link
Member Author

MarcoGorelli commented Jan 26, 2020 •
edited
Loading

@TomAugspurger as in, is it something that was working in 0.25 but isn't working on master? If so, then no, it's not a regression in 0.25.

Should I make a v1.0.1.rst whatsnew file then?

@jreback
Copy link
Contributor

jreback commented Jan 26, 2020

@TomAugspurger as in, is it something that was working in 0.25 but isn't working on master? If so, then no, it's not a regression in 0.25.

Should I make a v1.0.1.rst whatsnew file then?

no there is already a PR for that

@MarcoGorelli MarcoGorelli force-pushed the 27453-right-merge-order branch 6 times, most recently from 3da1145 to cba4abb Compare January 30, 2020 13:05
@MarcoGorelli
Copy link
Member Author

Otherwise would prefer to keep it for 1.0.1

@TomAugspurger sure, whatsnew entry added in v1.0.1

@WillAyd
Copy link
Member

WillAyd commented Jan 31, 2020

I think this should be 1.1 at this point? AFAIK not a regression

@MarcoGorelli
Copy link
Member Author

I think this should be 1.1 at this point? AFAIK not a regression

@WillAyd Sure, have moved the whatsnew entry to v1.1.0

WillAyd
WillAyd approved these changes Jan 31, 2020
View reviewed changes
Copy link
Member

@WillAyd WillAyd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

jreback
jreback requested changes Feb 9, 2020
View reviewed changes
pandas/core/reshape/merge.py Outdated Show resolved Hide resolved
pandas/tests/reshape/merge/test_merge.py Outdated Show resolved Hide resolved
pandas/tests/reshape/merge/test_merge.py Outdated

@pytest.mark.parametrize("how", ["left", "right"])
def test_merge_preserves_row_order(how):
# GH 27453
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is this the example from the OP (I would rather use that one exactly)

pandas/tests/reshape/merge/test_merge.py Outdated Show resolved Hide resolved
pandas/tests/reshape/merge/test_merge.py Outdated Show resolved Hide resolved
pandas/core/reshape/merge.py Outdated Show resolved Hide resolved
Nico Cernek and others added 4 commits February 13, 2020 11:43
add failing test to check row order preservation
f81c4ee 
correct the imports

broken commit with a bunch of print statements and comments

add test for left merge

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

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

clean up spacing and delete temp code

add whatsnew

replace .from_records with default constructor

add GH issue # to tests

revert commit ed54bec

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

clean formatting

rename vars and add comment for clarity

combine tests into one

update whatsnew

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

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

add before and after examples

linting

cleanup

changes requested by jreback

update docs
🔀 fix conflicts, fix nameerror in tests
25f7e03
📝 add whatsnew entry to v1.0.1
a53cc22
pass to _factorize_keys
2d77a5c
@MarcoGorelli MarcoGorelli mentioned this pull request Feb 13, 2020
Open
@MarcoGorelli
Copy link
Member Author

ping :)

jreback
jreback requested changes Mar 14, 2020
View reviewed changes
pandas/core/reshape/merge.py Outdated
@@ -1847,7 +1847,7 @@ def _right_outer_join(x, y, max_groups):
return left_indexer, right_indexer


def _factorize_keys(lk, rk, sort=True):
def _factorize_keys(lk, rk, sort=True, how="inner"):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you add a doc-string and typing here

pandas/tests/reshape/merge/test_merge.py Outdated
expected["B"] = df1["B"]
tm.assert_frame_equal(result, expected)

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

either reduce the examples here (they look like 3x of the same one), or parameterize over them.

jreback
jreback requested changes Mar 26, 2020
View reviewed changes
Copy link
Contributor

@jreback jreback left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm. some very minor doc comments, ping on green.

doc/source/whatsnew/v1.1.0.rst Outdated
left_df
right_df

*pandas 1.0.x*
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

change to prior behavior

doc/source/whatsnew/v1.1.0.rst Outdated
0 pig 11
1 quetzal 80

*pandas 1.1.0*
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

current behavior

@jreback jreback added this to the 1.1 milestone Mar 26, 2020
@MarcoGorelli
Copy link
Member Author

@jreback thanks - done

@jreback jreback merged commit 8a5f291 into pandas-dev:master Mar 26, 2020
@jreback
Copy link
Contributor

jreback commented Mar 26, 2020

thanks @MarcoGorelli for taking this over the line!

@MarcoGorelli MarcoGorelli deleted the 27453-right-merge-order branch March 26, 2020 12:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@WillAyd WillAyd WillAyd approved these changes

@jreback jreback jreback approved these changes

Assignees
No one assigned
Labels
Reshaping Concat, Merge/Join, Stack/Unstack, Explode
Projects
None yet
Milestone
1.1
Development

Successfully merging this pull request may close these issues.

Right merge not preserve order
4 participants
@MarcoGorelli @WillAyd @TomAugspurger @jreback