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

- [x] closes #26824
- [x] tests added / passed
- [ ] passes `git diff upstream/master -u -- "*.py" | flake8 --diff`
- [x] whatsnew entry

Labels:
name: Error Reporting
description: Incorrect or improved errors from pandas

Comments:
# [Codecov](https://codecov.io/gh/pandas-dev/pandas/pull/26855?src=pr&el=h1) Report
> Merging [#26855](https://codecov.io/gh/pandas-dev/pandas/pull/26855?src=pr&el=desc) into [master](https://codecov.io/gh/pandas-dev/pandas/commit/430f0fd04646a0ab44ccbfbad706dfe22e9fabfe?src=pr&el=desc) will **decrease** coverage by `0.01%`.
> The diff coverage is `50%`.

[![Impacted file tree graph](https://codecov.io/gh/pandas-dev/pandas/pull/26855/graphs/tree.svg?width=650&token=eZ4WkYLtcO&height=150&src=pr)](https://codecov.io/gh/pandas-dev/pandas/pull/26855?src=pr&el=tree)

```diff
@@            Coverage Diff             @@
##           master   #26855      +/-   ##
==========================================
- Coverage   91.88%   91.87%   -0.02%     
==========================================
  Files         179      179              
  Lines       50696    50700       +4     
==========================================
- Hits        46581    46579       -2     
- Misses       4115     4121       +6
```

| Flag | Coverage Δ | |
|---|---|---|
| #multiple | `90.46% <50%> (-0.01%)` | :arrow_down: |
| #single | `41.08% <0%> (-0.09%)` | :arrow_down: |

| [Impacted Files](https://codecov.io/gh/pandas-dev/pandas/pull/26855?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [pandas/core/reshape/merge.py](https://codecov.io/gh/pandas-dev/pandas/pull/26855/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvcmVzaGFwZS9tZXJnZS5weQ==) | `94.23% <50%> (-0.25%)` | :arrow_down: |
| [pandas/io/gbq.py](https://codecov.io/gh/pandas-dev/pandas/pull/26855/diff?src=pr&el=tree#diff-cGFuZGFzL2lvL2dicS5weQ==) | `88.88% <0%> (-11.12%)` | :arrow_down: |
| [pandas/core/frame.py](https://codecov.io/gh/pandas-dev/pandas/pull/26855/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvZnJhbWUucHk=) | `96.88% <0%> (-0.12%)` | :arrow_down: |

------

[Continue to review full report at Codecov](https://codecov.io/gh/pandas-dev/pandas/pull/26855?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/pandas-dev/pandas/pull/26855?src=pr&el=footer). Last update [430f0fd...c3064e6](https://codecov.io/gh/pandas-dev/pandas/pull/26855?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).

# [Codecov](https://codecov.io/gh/pandas-dev/pandas/pull/26855?src=pr&el=h1) Report
> :exclamation: No coverage uploaded for pull request base (`master@45ea267`). [Click here to learn what that means](https://docs.codecov.io/docs/error-reference#section-missing-base-commit).
> The diff coverage is `0%`.

[![Impacted file tree graph](https://codecov.io/gh/pandas-dev/pandas/pull/26855/graphs/tree.svg?width=650&token=eZ4WkYLtcO&height=150&src=pr)](https://codecov.io/gh/pandas-dev/pandas/pull/26855?src=pr&el=tree)

```diff
@@            Coverage Diff            @@
##             master   #26855   +/-   ##
=========================================
  Coverage          ?   41.09%           
=========================================
  Files             ?      180           
  Lines             ?    50747           
  Branches          ?        0           
=========================================
  Hits              ?    20856           
  Misses            ?    29891           
  Partials          ?        0
```

| Flag | Coverage Δ | |
|---|---|---|
| #single | `41.09% <0%> (?)` | |

| [Impacted Files](https://codecov.io/gh/pandas-dev/pandas/pull/26855?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [pandas/core/reshape/merge.py](https://codecov.io/gh/pandas-dev/pandas/pull/26855/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvcmVzaGFwZS9tZXJnZS5weQ==) | `9.34% <0%> (ø)` | |

------

[Continue to review full report at Codecov](https://codecov.io/gh/pandas-dev/pandas/pull/26855?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/pandas-dev/pandas/pull/26855?src=pr&el=footer). Last update [45ea267...e4e486c](https://codecov.io/gh/pandas-dev/pandas/pull/26855?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).

> Can you add a test for this? Typically the first thing we ask for as part of any PR

Sure, just to confirm this test would go in pandas/tests/reshape/merge/test_merge.py?
@simonjayhawkins if you care to take a look
I think the `has no len()` part of the message in issue #26824 could be a red herring here.

An exception with the `len(right_on) must equal len(left_on)` message is raised when the length of the `right_on` and `left_on` parameters is not the same, eg.

```python
pd.merge(left, right, left_on=['key1'], right_on=['key1', 'key2']) # ValueError: len(right_on) must equal len(left_on)
```

In cases such as this issue where neither `right_on` or `right_index` parameters are passed, we should expect to see:

```python
pd.merge(left, right, left_on=['key1']) # MergeError: Must pass right_on or right_index=True
```

So it could be that, this exception is not being raised, since in #26824, as well as not passing either `right_on` or `right_index`, the example does not have the key passed in `left_on`.

If we add the right_on to the issue:

```
pd.merge(df2, df1, how='left', left_on='A', right_on='boro') # KeyError: 'A'
```

we get the more informative `KeyError`

So probably this should also be the exception expected in this scenarios even when neither `right_on` or `right_index` parameters are passed

Hello @harshit-py! Thanks for updating this PR. We checked the lines you've touched for [PEP 8](https://www.python.org/dev/peps/pep-0008) issues, and found:





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

##### Comment last updated at 2019-06-28 14:17:00 UTC
@jreback Hey I think I messed this up while trying to resolve conflicts in my PR. Can I open a separate PR for this?
@harshit-py I made https://github.com/harshit-py/pandas/pull/1. I think if you merge that, it'll merge cleanly against master.

<details>

```diff
diff --git a/doc/source/whatsnew/v0.25.0.rst b/doc/source/whatsnew/v0.25.0.rst
index a58cdc8c9..01aa9ad97 100644
--- a/doc/source/whatsnew/v0.25.0.rst
+++ b/doc/source/whatsnew/v0.25.0.rst
@@ -74,7 +74,6 @@ a dict to a Series groupby aggregation (:ref:`whatsnew_0200.api_breaking.depreca
 
 See :ref:`groupby.aggregate.named` for more.
 
-
 .. _whatsnew_0250.enhancements.multi_index_repr:
 
 Better repr for MultiIndex
@@ -105,7 +104,6 @@ than :attr:`options.display.max_seq_items` (default: 100 items). Horizontally,
 the output will truncate, if it's wider than :attr:`options.display.width`
 (default: 80 characters).
 
-
 .. _whatsnew_0250.enhancements.other:
 
 Other Enhancements
@@ -131,6 +129,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`)
 - Added support for reading SPSS .sav files using :func:`read_spss` (:issue:`26537`)
 - Added new option ``plotting.backend`` to be able to select a plotting backend different than the existing ``matplotlib`` one. Use ``pandas.set_option('plotting.backend', '<backend-module>')`` where ``<backend-module`` is a library implementing the pandas plotting API (:issue:`14130`)
 
@@ -538,6 +537,7 @@ Other Deprecations
 - :meth:`Timedelta.resolution` is deprecated and replaced with :meth:`Timedelta.resolution_string`.  In a future version, :meth:`Timedelta.resolution` will be changed to behave like the standard library :attr:`timedelta.resolution` (:issue:`21344`)
 - :meth:`Series.to_sparse`, :meth:`DataFrame.to_sparse`, :meth:`Series.to_dense` and :meth:`DataFrame.to_dense` are deprecated and will be removed in a future version. (:issue:`26557`).
 
+
 .. _whatsnew_0250.prior_deprecations:
 
 Removal of prior version deprecations/changes
diff --git a/pandas/core/reshape/merge.py b/pandas/core/reshape/merge.py
index d21ad58e7..decc21ed4 100644
--- a/pandas/core/reshape/merge.py
+++ b/pandas/core/reshape/merge.py
@@ -1091,6 +1091,9 @@ class _MergeOperation:
                     raise ValueError('len(left_on) must equal the number '
                                      'of levels in the index of "right"')
                 self.right_on = [None] * n
+            if not self.right_on:
+                raise ValueError('both "right_on" and "left_on" '
+                                 'should be passed')
         elif self.right_on is not None:
             n = len(self.right_on)
             if self.left_index:
@@ -1098,6 +1101,9 @@ class _MergeOperation:
                     raise ValueError('len(right_on) must equal the number '
                                      'of levels in the index of "left"')
                 self.left_on = [None] * n
+            if not self.left_on:
+                raise ValueError('both "right_on" and "left_on" '
+                                 'should be passed')
         if len(self.right_on) != len(self.left_on):
             raise ValueError("len(right_on) must equal len(left_on)")
 
diff --git a/pandas/tests/reshape/merge/test_merge.py b/pandas/tests/reshape/merge/test_merge.py
index 8eb414155..940e4d09a 100644
--- a/pandas/tests/reshape/merge/test_merge.py
+++ b/pandas/tests/reshape/merge/test_merge.py
@@ -1026,6 +1026,22 @@ class TestMerge:
         result = merge(left, right, on=['a', 'b'], validate='1:1')
         assert_frame_equal(result, expected_multi)
 
+    @pytest.mark.parametrize('merge_type', ['left_on', 'right_on'])
+    def test_missing_on_raises(self, merge_type):
+        # GH26824
+        left = DataFrame({
+            'A': [1, 2, 3, 4, 5, 6],
+            'B': ['P', 'Q', 'R', 'S', 'T', 'U']
+        })
+        right = 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(left, right, how='left', **kwargs)
+
     def test_merge_two_empty_df_no_division_error(self):
         # GH17776, PR #17846
         a = pd.DataFrame({'a': [], 'b': [], 'c': []})
@@ -1763,3 +1779,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'])
+def test_missing_on_raises(merge_type):
+    # GH26824
+    df1 = DataFrame({
+        '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)
```

</details>

lgtm. @WillAyd if any other comments; merge on green.
Looks like CI is failing here. 
@harshit-py can you merge master and move the release note to 1.0
Nice PR but CI is all red and I think this has gone stale. Closing for now but @harshit-py please ping if you'd like to pick back up