Skip to content

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

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

Closed
wants to merge 28 commits into from
Closed

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

Show file tree
Hide file tree
Changes from 8 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
View file
Open in desktop
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
View file
Open in desktop
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] * (n + 1)
Copy link
Contributor

Choose a reason for hiding this comment

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

why do you need to add 1 this seems odd

Copy link
Author

Choose a reason for hiding this comment

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

so, the thinking here was to not add additional check and force the cases where either of left_on or right_on is absent to the if clause at 1099.

Copy link
Contributor

Choose a reason for hiding this comment

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

right

but i think this is actually changing things slightly

maybe let’s go back to your original patch

Copy link
Member

Choose a reason for hiding this comment

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

but i think this is actually changing things slightly

additional message checks maybe should be added to the following tests as a precursor. xref #23922

pandas/tests/reshape/merge/test_merge_asof.py:        with pytest.raises(MergeError):
pandas/tests/reshape/merge/test_merge_asof.py:        with pytest.raises(MergeError):
pandas/tests/reshape/merge/test_merge_asof.py:        with pytest.raises(MergeError):
pandas/tests/reshape/merge/test_merge_asof.py:        with pytest.raises(MergeError):
pandas/tests/reshape/merge/test_merge_asof.py:        with pytest.raises(MergeError):
pandas/tests/reshape/merge/test_merge_asof.py:        with pytest.raises(MergeError):
pandas/tests/reshape/merge/test_merge_asof.py:        with pytest.raises(MergeError):
pandas/tests/reshape/merge/test_merge_asof.py:        with pytest.raises(MergeError):
pandas/tests/reshape/merge/test_merge_asof.py:        with pytest.raises(MergeError):
pandas/tests/reshape/merge/test_merge_asof.py:        with pytest.raises(MergeError):
pandas/tests/reshape/merge/test_merge_asof.py:        with pytest.raises(MergeError):
pandas/tests/reshape/merge/test_merge_asof.py:        with pytest.raises(MergeError):
pandas/tests/reshape/merge/test_merge_asof.py:        with pytest.raises(MergeError):
pandas/tests/reshape/merge/test_merge_asof.py:        with pytest.raises(ValueError):
pandas/tests/reshape/merge/test_merge_asof.py:        with pytest.raises(ValueError):
pandas/tests/reshape/merge/test_multi.py:        with pytest.raises(ValueError):
pandas/tests/reshape/merge/test_multi.py:        with pytest.raises(ValueError):

also the following tests perhaps should be moved to pandas/tests/reshape/merge/test_merge.py ( could potentially add additional parametrisation to these tests rather than creating new)

pandas/pandas/tests/reshape/merge/test_join.py

Lines 202 to 247 in a738223

def test_join_on_fails_with_different_right_index(self):
df = DataFrame({'a': np.random.choice(['m', 'f'], size=3),
'b': np.random.randn(3)})
df2 = DataFrame({'a': np.random.choice(['m', 'f'], size=10),
'b': np.random.randn(10)},
index=tm.makeCustomIndex(10, 2))
msg = (r'len\(left_on\) must equal the number of levels in the index'
' of "right"')
with pytest.raises(ValueError, match=msg):
merge(df, df2, left_on='a', right_index=True)
def test_join_on_fails_with_different_left_index(self):
df = DataFrame({'a': np.random.choice(['m', 'f'], size=3),
'b': np.random.randn(3)},
index=tm.makeCustomIndex(3, 2))
df2 = DataFrame({'a': np.random.choice(['m', 'f'], size=10),
'b': np.random.randn(10)})
msg = (r'len\(right_on\) must equal the number of levels in the index'
' of "left"')
with pytest.raises(ValueError, match=msg):
merge(df, df2, right_on='b', left_index=True)
def test_join_on_fails_with_different_column_counts(self):
df = DataFrame({'a': np.random.choice(['m', 'f'], size=3),
'b': np.random.randn(3)})
df2 = DataFrame({'a': np.random.choice(['m', 'f'], size=10),
'b': np.random.randn(10)},
index=tm.makeCustomIndex(10, 2))
msg = r"len\(right_on\) must equal len\(left_on\)"
with pytest.raises(ValueError, match=msg):
merge(df, df2, right_on='a', left_on=['a', 'b'])
@pytest.mark.parametrize("wrong_type", [2, 'str', None, np.array([0, 1])])
def test_join_on_fails_with_wrong_object_type(self, wrong_type):
# GH12081 - original issue
# GH21220 - merging of Series and DataFrame is now allowed
# Edited test to remove the Series object from test parameters
df = DataFrame({'a': [1, 1]})
msg = ("Can only merge Series or DataFrame objects, a {} was passed"
.format(str(type(wrong_type))))
with pytest.raises(TypeError, match=msg):
merge(wrong_type, df, left_on='a', right_on='a')
with pytest.raises(TypeError, match=msg):
merge(df, wrong_type, left_on='a', right_on='a')

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] * (n + 1)
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
View file
Open in desktop
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 = 'must equal'
kwargs = {merge_type: 'A'}
with pytest.raises(ValueError, match=msg):
pd.merge(df1, df2, how='left', **kwargs)