ENH: Improve error reporting for wrong merge cols #37547

phofl · 2020-10-31T22:05:53Z

closes ERR: left_index AND left_on or right_index AND right_on should raise #16228
tests added / passed
passes black pandas
passes git diff upstream/master -u -- "*.py" | flake8 --diff
whatsnew entry

Fixed error reporting when double conditions are given. Additionally I found, that something like

left = DataFrame({"a": [1, 2], "b": [3, 4]})
right = DataFrame({"a": [1, 1], "c": [5, 6]})
pd.merge(left, right, right_on="a")

raised an uncontrolled error:

Traceback (most recent call last):
  File "/home/developer/.config/JetBrains/PyCharm2020.2/scratches/scratch_4.py", line 25, in <module>
    print(pd.merge(left, right, right_on="a"))
  File "/home/developer/PycharmProjects/pandas/pandas/core/reshape/merge.py", line 72, in merge
    op = _MergeOperation(
  File "/home/developer/PycharmProjects/pandas/pandas/core/reshape/merge.py", line 643, in __init__
    self._validate_specification()
  File "/home/developer/PycharmProjects/pandas/pandas/core/reshape/merge.py", line 1247, in _validate_specification
    if len(self.right_on) != len(self.left_on):
TypeError: object of type 'NoneType' has no len()

ivanovmg

Looks good to me, in general. Please see comments.
Nice tests parametrization.
Is there a plan to somehow make the logic less complex here (if possible)? Not a topic of the present PR, though.

ivanovmg · 2020-11-06T11:36:45Z

pandas/tests/reshape/merge/test_merge.py

+        r"Can only pass argument \"on\" OR \"left_index\" "
+        r"and \"right_index\", not a combination of both\."


This would be more readable in single quotes. Or are they rejected by black?

Thx. That is a good point. Changed it

ivanovmg · 2020-11-06T11:38:01Z

pandas/tests/reshape/merge/test_merge.py

+    # GH: 16228
+    left = DataFrame({"a": [1, 2], "b": [3, 4]})
+    right = DataFrame({"a": [1, 1], "c": [5, 6]})
+    msg = rf"Can only pass argument \"{err_msg[0]}\" OR \"{err_msg[1]}\" not both\."


This would be more readable in single quotes.

ivanovmg · 2020-11-06T11:42:33Z

pandas/core/reshape/merge.py

-                if self.right_on is None:
-                    raise MergeError("Must pass right_on or right_index=True")
+                raise MergeError("Must pass right_on or right_index=True")


Nice catch.

ivanovmg · 2020-11-06T11:43:16Z

pandas/tests/reshape/merge/test_merge.py

+    # GH: 16228
+    left = DataFrame({"a": [1, 2], "b": [3, 4]})
+    right = DataFrame({"a": [1, 1], "c": [5, 6]})
+    msg = rf"Must pass \"{err_msg[0]}\" OR \"{err_msg[1]}\"\."


Same in regards to quotes.

� Conflicts: � doc/source/whatsnew/v1.2.0.rst

phofl · 2020-11-06T20:57:38Z

Unfortunately I don't have an idea how to simplify this significantly right now, but will try to look into this into the future.

jreback · 2020-11-08T03:08:34Z

thanks @phofl very nice, this has been an open issue for a while!

yeah happy to take refactoring PRs (but not sure this is that easy), but i am sure there are parts that can be cleaned.

… (#37655) * Moving the file test_frame.py to a new directory * Сreated file test_frame_color.py * Transfer tests of test_frame.py to test_frame_color.py * PEP 8 fixes * Transfer tests of test_frame.py to test_frame_groupby.py and test_frame_subplots.py * Removing unnecessary imports * PEP 8 fixes * Fixed class name * Transfer tests of test_frame.py to test_frame_subplots.py * Transfer tests of test_frame.py to test_frame_groupby.py, test_frame_subplots.py, test_frame_color.py * Changed class names * Removed unnecessary imports * Removed import * catch FutureWarnings (#37587) * TST/REF: collect indexing tests by method (#37590) * REF: prelims for single-path setitem_with_indexer (#37588) * ENH: __repr__ for 2D DTA/TDA (#37164) * CLN: de-duplicate _validate_where_value with _validate_setitem_value (#37595) * TST/REF: collect tests by method (#37589) * TST/REF: move remaining setitem tests from test_timeseries * TST/REF: rehome test_timezones test * move misplaced arithmetic test * collect tests by method * move misplaced file * REF: Categorical.is_dtype_equal -> categories_match_up_to_permutation (#37545) * CLN refactor non-core (#37580) * refactor core/computation (#37585) * TST/REF: share method tests between DataFrame and Series (#37596) * BUG: Index.where casting ints to str (#37591) * REF: IntervalArray comparisons (#37124) * regression fix for merging DF with datetime index with empty DF (#36897) * ERR: fix error message in Period for invalid frequency (#37602) * CLN: remove rebox_native (#37608) * TST/REF: tests.generic (#37618) * TST: collect tests by method (#37617) * TST/REF: collect test_timeseries tests by method * misplaced DataFrame.values tst * misplaced dataframe.values test * collect test by method * TST/REF: share tests across Series/DataFrame (#37616) * Gh 36562 typeerror comparison not supported between float and str (#37096) * docs: fix punctuation (#37612) * REGR: pd.to_hdf(..., dropna=True) not dropping missing rows (#37564) * parametrize set_axis tests (#37619) * CLN: clean color selection in _matplotlib/style (#37203) * DEPR: DataFrame/Series.slice_shift (#37601) * REF: re-use validate_setitem_value in Categorical.fillna (#37597) * PERF: release gil for ewma_time (#37389) * BUG: Groupy dropped nan groups from result when grouping over single column (#36842) * ENH: implement timeszones support for read_json(orient='table') and astype() from 'object' (#35973) * REF/BUG/TYP: read_csv shouldn't close user-provided file handles (#36997) * BUG/REF: read_csv shouldn't close user-provided file handles * get_handle: typing, returns is_wrapped, use dataclass, and make sure that all created handlers are returned * remove unused imports * added IOHandleArgs.close * added IOArgs.close * mostly comments * move memory_map from TextReader to CParserWrapper * moved IOArgs and IOHandles * more comments Co-authored-by: Jeff Reback <jeff@reback.net> * more typing checks to pre-commit (#37539) * TST: 32bit dtype compat test_groupby_dropna (#37623) * BUG: Metadata propagation for groupby iterator (#37461) * BUG: read-only values in cython funcs (#37613) * CLN refactor core/arrays (#37581) * Fixed Metadata Propogation in DataFrame (#37381) * TYP: add Shape alias to pandas._typing (#37128) * DOC: Fix typo (#37630) * CLN: parametrize test_nat_comparisons (#37195) * dataframe dataclass docstring updated (#37632) * refactor core/groupby (#37583) * BUG: set index of DataFrame.apply(f) when f returns dict (#37544) (#37606) * BUG: to_dict should return a native datetime object for NumPy backed dataframes (#37571) * ENH: memory_map for compressed files (#37621) * DOC: add example & prose of slicing with labels when index has duplicate labels (#36814) * DOC: add example & prose of slicing with labels when index has duplicate labels #36251 * DOC: proofread the sentence. Co-authored-by: Jun Kudo <jun-lab@junnoMacBook-Pro.local> * DOC: Fix typo (#37636) "columns(s)" sounded odd, I believe it was supposed to be just "column(s)". * CI: troubleshoot win py38 builds (#37652) * TST/REF: collect indexing tests by method (#37638) * TST/REF: collect tests for get_numeric_data (#37634) * misplaced loc test * TST/REF: collect get_numeric_data tests * REF: de-duplicate _validate_insert_value with _validate_scalar (#37640) * CI: catch windows py38 OSError (#37659) * share test (#37679) * TST: match matplotlib warning message (#37666) * TST: match matplotlib warning message * TST: match full message * pd.Series.loc.__getitem__ promotes to float64 instead of raising KeyError (#37687) * REF/TST: misplaced Categorical tests (#37678) * REF/TST: collect indexing tests by method (#37677) * CLN: only call _wrap_results one place in nanmedian (#37673) * TYP: Index._concat (#37671) * BUG: CategoricalIndex.equals casting non-categories to np.nan (#37667) * CLN: _replace_single (#37683) * REF: simplify _replace_single by noting regex kwarg is bool * Annotate * CLN: remove never-False convert kwarg * TYP: make more internal funcs keyword-only (#37688) * REF: make Series._replace_single a regular method (#37691) * REF: simplify cycling through colors (#37664) * REF: implement _wrap_reduction_result (#37660) * BUG: preserve fold in Timestamp.replace (#37644) * CLN: Clean indexing tests (#37689) * TST: fix warning for pie chart (#37669) * PERF: reverted change from commit 7d257c6 to solve issue #37081 (#37426) * DataFrameGroupby.boxplot fails when subplots=False (#28102) * ENH: Improve error reporting for wrong merge cols (#37547) * Transfer tests of test_frame.py to test_frame_color.py * PEP8 * Fixes for linter * Сhange pd.DateFrame to DateFrame * Move inconsistent namespace check to pre-commit, fixup more files (#37662) * check for inconsistent namespace usage * doc * typos * verbose regex * use verbose flag * use verbose flag * match both directions * add test * don't import annotations from future * update extra couple of cases * 🚚 rename * typing * don't use subprocess * don't type tests * use pathlib * REF: simplify NDFrame.replace, ObjectBlock.replace (#37704) * REF: implement Categorical.encode_with_my_categories (#37650) * REF: implement Categorical.encode_with_my_categories * privatize * BUG: unpickling modifies Block.ndim (#37657) * REF: dont support dt64tz in nanmean (#37658) * CLN: Simplify groupby head/tail tests (#37702) * Bug in loc raised for numeric label even when label is in Index (#37675) * REF: implement replace_regex, remove unreachable branch in ObjectBlock.replace (#37696) * TYP: Check untyped defs (except vendored) (#37556) * REF: remove ObjectBlock._replace_single (#37710) * Transfer tests of test_frame.py to test_frame_color.py * TST/REF: collect indexing tests by method (#37590) * PEP8 * Сhange DateFrame to pd.DateFrame * Сhange pd.DateFrame to DateFrame * Removing imports * Bug fixes * Bug fixes * Fix incorrect merge * test_frame_color.py edit * Transfer tests of test_frame.py to test_frame_color.py, test_frame_groupby.py and test_frame_subplots.py * Removing unnecessary imports * PEP8 * # Conflicts: # pandas/tests/plotting/frame/test_frame.py # pandas/tests/plotting/frame/test_frame_color.py # pandas/tests/plotting/frame/test_frame_subplots.py * Moving the file test_frame.py to a new directory * Transfer tests of test_frame.py to test_frame_color.py, test_frame_groupby.py and test_frame_subplots.py * Removing unnecessary imports * PEP8 * CLN: clean categorical indexes tests (#37721) * Fix merge error * PEP 8 fixes * Fix merge error * Removing unnecessary imports * PEP 8 fixes * Fixed class name * Transfer tests of test_frame.py to test_frame_subplots.py * Transfer tests of test_frame.py to test_frame_groupby.py, test_frame_subplots.py, test_frame_color.py * Changed class names * Removed unnecessary imports * Removed import * TST/REF: collect indexing tests by method (#37590) * TST: match matplotlib warning message (#37666) * TST: match matplotlib warning message * TST: match full message * TST: fix warning for pie chart (#37669) * Transfer tests of test_frame.py to test_frame_color.py * PEP8 * Fixes for linter * Сhange pd.DateFrame to DateFrame * Transfer tests of test_frame.py to test_frame_color.py * PEP8 * Сhange DateFrame to pd.DateFrame * Сhange pd.DateFrame to DateFrame * Removing imports * Bug fixes * Bug fixes * Fix incorrect merge * test_frame_color.py edit * Fix merge error * Fix merge error * Removing unnecessary features * Resolving Commit Conflicts daf999f 365d843 * black fix Co-authored-by: jbrockmendel <jbrockmendel@gmail.com> Co-authored-by: Marco Gorelli <m.e.gorelli@gmail.com> Co-authored-by: Philip Cerles <philip.cerles@gmail.com> Co-authored-by: Joris Van den Bossche <jorisvandenbossche@gmail.com> Co-authored-by: Sven <sven.schellenberg@paradynsystems.com> Co-authored-by: Micael Jarniac <micael@jarniac.com> Co-authored-by: Andrew Wieteska <48889395+arw2019@users.noreply.github.com> Co-authored-by: Maxim Ivanov <41443370+ivanovmg@users.noreply.github.com> Co-authored-by: Erfan Nariman <34067903+erfannariman@users.noreply.github.com> Co-authored-by: Fangchen Li <fangchen.li@outlook.com> Co-authored-by: patrick <61934744+phofl@users.noreply.github.com> Co-authored-by: attack68 <24256554+attack68@users.noreply.github.com> Co-authored-by: Torsten Wörtwein <twoertwein@users.noreply.github.com> Co-authored-by: Jeff Reback <jeff@reback.net> Co-authored-by: Janus <janus@insignificancegalore.net> Co-authored-by: Joel Whittier <rootbeerfriend@gmail.com> Co-authored-by: taytzehao <jtth95@gmail.com> Co-authored-by: ma3da <34522496+ma3da@users.noreply.github.com> Co-authored-by: junk <juntrp0207@gmail.com> Co-authored-by: Jun Kudo <jun-lab@junnoMacBook-Pro.local> Co-authored-by: Alex Kirko <alexander.kirko@gmail.com> Co-authored-by: Yassir Karroum <ukarroum17@gmail.com> Co-authored-by: Kaiqi Dong <kaiqi@kth.se> Co-authored-by: Richard Shadrach <45562402+rhshadrach@users.noreply.github.com> Co-authored-by: Simon Hawkins <simonjayhawkins@gmail.com>

phofl added 5 commits October 31, 2020 22:33

Improve error reporting in merge

87835fa

Merge branch 'master' of https://github.com/pandas-dev/pandas

f6d9776

Add tests

490062b

Add whatsnew

8f787d3

Fix raising condition

e865ae7

phofl added Error Reporting Incorrect or improved errors from pandas Reshaping Concat, Merge/Join, Stack/Unstack, Explode labels Oct 31, 2020

ivanovmg reviewed Nov 6, 2020

View reviewed changes

phofl added 2 commits November 6, 2020 21:55

Adress review comments

3981fb6

Merge branch 'master' of https://github.com/pandas-dev/pandas into 16228

152e70c

� Conflicts: � doc/source/whatsnew/v1.2.0.rst

jreback added this to the 1.2 milestone Nov 8, 2020

jreback merged commit 0755915 into pandas-dev:master Nov 8, 2020

phofl deleted the 16228 branch November 8, 2020 12:06

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ENH: Improve error reporting for wrong merge cols #37547

ENH: Improve error reporting for wrong merge cols #37547

phofl commented Oct 31, 2020

ivanovmg left a comment

ivanovmg Nov 6, 2020

phofl Nov 6, 2020

ivanovmg Nov 6, 2020

phofl Nov 6, 2020

ivanovmg Nov 6, 2020

ivanovmg Nov 6, 2020 •

edited

Loading

phofl Nov 6, 2020

phofl commented Nov 6, 2020 •

edited

Loading

jreback commented Nov 8, 2020

		r"Can only pass argument \"on\" OR \"left_index\" "
		r"and \"right_index\", not a combination of both\."

ENH: Improve error reporting for wrong merge cols #37547

ENH: Improve error reporting for wrong merge cols #37547

Conversation

phofl commented Oct 31, 2020

ivanovmg left a comment

Choose a reason for hiding this comment

ivanovmg Nov 6, 2020

Choose a reason for hiding this comment

phofl Nov 6, 2020

Choose a reason for hiding this comment

ivanovmg Nov 6, 2020

Choose a reason for hiding this comment

phofl Nov 6, 2020

Choose a reason for hiding this comment

ivanovmg Nov 6, 2020

Choose a reason for hiding this comment

ivanovmg Nov 6, 2020 • edited Loading

Choose a reason for hiding this comment

phofl Nov 6, 2020

Choose a reason for hiding this comment

phofl commented Nov 6, 2020 • edited Loading

jreback commented Nov 8, 2020

ivanovmg Nov 6, 2020 •

edited

Loading

phofl commented Nov 6, 2020 •

edited

Loading