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.

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

BUG:Sanity check on merge parameters for correct exception #26824 #26855

Closed
wants to merge 28 commits into from
Closed
Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
28 commits
Select commit Hold shift + click to select a range
c3064e6
BUG:Sanity check on merge parameters for correct exception #26824
harshit-py Jun 14, 2019
c862ec0
added tests
harshit-py Jun 15, 2019
0df85f0
updated tests for better coverage
harshit-py Jun 15, 2019
dea59fa
requested changes on the test
harshit-py Jun 15, 2019
53aee36
updated whatsnew v0250rst
harshit-py Jun 15, 2019
6396947
requested changes
harshit-py Jun 15, 2019
11b1894
further changes
harshit-py Jun 15, 2019
7b4aa22
updated test
harshit-py Jun 15, 2019
bfb7984
Merge branch 'master' into merge_py_bug
harshit-py Jun 17, 2019
1e2b276
BUG:Sanity check on merge parameters for correct exception #26824
harshit-py Jun 14, 2019
0f88c32
added tests
harshit-py Jun 15, 2019
a9905bd
updated tests for better coverage
harshit-py Jun 15, 2019
a939d8e
requested changes on the test
harshit-py Jun 15, 2019
83f8cda
updated whatsnew v0250rst
harshit-py Jun 15, 2019
962882d
requested changes
harshit-py Jun 15, 2019
139d696
further changes
harshit-py Jun 15, 2019
4e6bd89
updated test
harshit-py Jun 15, 2019
a0680e0
Trying original patch
harshit-py Jun 17, 2019
0a894a9
Revert "Trying original patch"
harshit-py Jun 19, 2019
0cb8843
Trying original patch
harshit-py Jun 19, 2019
4e45c75
Revert "Trying original patch"
harshit-py Jun 19, 2019
500e374
Trying original patch
harshit-py Jun 19, 2019
773dec2
Original patch
harshit-py Jun 19, 2019
7770b1d
further changes
harshit-py Jun 19, 2019
efb0761
Merge remote-tracking branch 'upstream/master' into harshit-py-merge_…
TomAugspurger Jun 25, 2019
6b7f5a2
re-revert
TomAugspurger Jun 25, 2019
b7c482e
Merge pull request #1 from TomAugspurger/harshit-py-merge_py_bug
harshit-py Jun 28, 2019
e4e486c
Merge branch 'master' into PR_TOOL_MERGE_PR_26855
jreback Jun 28, 2019
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions doc/source/whatsnew/v0.25.0.rst
Original file line number Diff line number Diff line change
Expand Up @@ -99,6 +99,7 @@ Other Enhancements
- Error message for missing required imports now includes the original import error's text (:issue:`23868`)
- :class:`DatetimeIndex` and :class:`TimedeltaIndex` now have a ``mean`` method (:issue:`24757`)
- :meth:`DataFrame.describe` now formats integer percentiles without decimal point (:issue:`26660`)
- :func:`pandas.merge` now raises ``ValueError`` when either of ``left_on`` or ``right_on`` is not provided (:issue:`26855`)
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 elaborate slightly; this only matters if on itself is not provided. Do we have a check if on is provide and either of left_on or right_on is not None?


.. _whatsnew_0250.api_breaking:

Expand Down
2 changes: 2 additions & 0 deletions pandas/core/reshape/merge.py
Original file line number Diff line number Diff line change
Expand Up @@ -1089,13 +1089,15 @@ def _validate_specification(self):
raise ValueError('len(left_on) must equal the number '
'of levels in the index of "right"')
self.right_on = [None] * n
self.right_on = self.right_on or [None] * len(self.left_on)
elif self.right_on is not None:
n = len(self.right_on)
if self.left_index:
if len(self.right_on) != self.left.index.nlevels:
raise ValueError('len(right_on) must equal the number '
'of levels in the index of "left"')
self.left_on = [None] * n
self.left_on = self.left_on or [None] * len(self.right_on)
Copy link
Contributor

Choose a reason for hiding this comment

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

you don’t need the lines above (not both cases)
also can use n

Copy link
Author

Choose a reason for hiding this comment

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

not sure I get it. The previous line needs to go?

Copy link
Contributor

Choose a reason for hiding this comment

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

try it - i think it’s unnecessary

Copy link
Contributor

Choose a reason for hiding this comment

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

u can also combine the condition in 1095 and 1096 i think as well

Copy link
Author

Choose a reason for hiding this comment

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

left_index and right_index are False by default (in the _MergeOperation instantiation), so both left_on and right_on would need to be defined out of the if clause at 1095 and 1087 (correct me if I'm wrong)

Copy link
Contributor

@jreback jreback Jun 15, 2019

Choose a reason for hiding this comment

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

ok see if u can simplify things with passing tests

if len(self.right_on) != len(self.left_on):
raise ValueError("len(right_on) must equal len(left_on)")

Expand Down
17 changes: 17 additions & 0 deletions pandas/tests/reshape/merge/test_merge.py
Original file line number Diff line number Diff line change
Expand Up @@ -1763,3 +1763,20 @@ def test_merge_equal_cat_dtypes2():

# Categorical is unordered, so don't check ordering.
tm.assert_frame_equal(result, expected, check_categorical=False)


@pytest.mark.parametrize('merge_type', ['left_on', 'right_on'])
Copy link
Contributor

Choose a reason for hiding this comment

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

move this near the test_validation test

def test_missing_on_raises(merge_type):
# GH26824
df1 = DataFrame({
Copy link
Contributor

Choose a reason for hiding this comment

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

call these left & right

'A': [1, 2, 3, 4, 5, 6],
'B': ['P', 'Q', 'R', 'S', 'T', 'U']
})
df2 = DataFrame({
'A': [1, 2, 4, 5, 7, 8],
'C': ['L', 'M', 'N', 'O', 'P', 'Q']
})
msg = 'both left_on and right_on should be passed'
kwargs = {merge_type: 'A'}
with pytest.raises(ValueError, match=msg):
pd.merge(df1, df2, how='left', **kwargs)