Skip to content

BUG: Merge on CategoricalIndex fails if left_index=True & right_index=True, but not if on={index} #28189 #28296

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 9 commits into from
Closed

BUG: Merge on CategoricalIndex fails if left_index=True & right_index=True, but not if on={index} #28189 #28296

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
11cb1f8
cython fix
gabriellm1 Aug 29, 2019
95c5cf8
added unit test
guipleite Sep 5, 2019
fd1d3f1
cython fix
gabriellm1 Aug 29, 2019
8321075
Fixing reviwed unit test
guipleite Sep 5, 2019
d938b1b
Futher fixing reviwed unit test
guipleite Sep 5, 2019
2bbc491
int16fix
hugoecarl Sep 5, 2019
4e8bf1f
whatsnew format
hugoecarl Sep 6, 2019
be85da6
merge
gabriellm1 Oct 10, 2019
9915105
merge conflits
gabriellm1 Oct 10, 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
2 changes: 1 addition & 1 deletion doc/source/whatsnew/v1.0.0.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -223,7 +223,7 @@ Categorical
- Bug where :func:`merge` was unable to join on categorical and extension dtype columns (:issue:`28668`)
- :meth:`Categorical.searchsorted` and :meth:`CategoricalIndex.searchsorted` now work on unordered categoricals also (:issue:`21667`)
- Added test to assert roundtripping to parquet with :func:`DataFrame.to_parquet` or :func:`read_parquet` will preserve Categorical dtypes for string types (:issue:`27955`)
-
- Bug in merge on CategoricalIndex fails if ``left_index=True`` & ``right_index=True``, but not if ``on={index}`` (:issue:`28189`)-


Datetimelike
Expand Down
7 changes: 7 additions & 0 deletions pandas/_libs/join.pyx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -242,6 +242,8 @@ ctypedef fused join_t:
float64_t
float32_t
object
int8_t
int16_t
int32_t
int64_t
uint64_t
Expand Down Expand Up @@ -410,6 +412,11 @@ def left_join_indexer(ndarray[join_t] left, ndarray[join_t] right):
left_join_indexer_float64 = left_join_indexer["float64_t"]
left_join_indexer_float32 = left_join_indexer["float32_t"]
left_join_indexer_object = left_join_indexer["object"]
left_join_indexer_int8 = left_join_indexer["int8_t"]
<<<<<<< HEAD
=======
left_join_indexer_int16 = left_join_indexer["int16_t"]
>>>>>>> 4e8bf1f62a82e6fec532c91297b0b5a908351c3d
left_join_indexer_int32 = left_join_indexer["int32_t"]
left_join_indexer_int64 = left_join_indexer["int64_t"]
left_join_indexer_uint64 = left_join_indexer["uint64_t"]
Expand Down
18 changes: 18 additions & 0 deletions pandas/tests/test_join.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -348,3 +348,21 @@ def test_merge_join_categorical_multiindex():
result = a.join(b, on=["Cat1", "Int1"])
expected = expected.drop(["Cat", "Int"], axis=1)
assert_frame_equal(expected, result)


def test_left_index_and_right_index_true():
# From issue 28189
df = DataFrame(
range(4), columns=["value"], index=Index(Categorical(["1"] * 4), name="idx")
)
df2 = DataFrame(
[[6]], columns=["value"], index=Index(Categorical(["1"]), name="idx")
)

result = merge(df, df2, how="left", left_index=True, right_index=True)
result = result.reset_index(drop=True)
Copy link
Member

Choose a reason for hiding this comment

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

Is the reset_index required or can the index be included as part of expected?

Copy link
Contributor

Choose a reason for hiding this comment

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

I left it out of the test because of the issues @hugoecarl mentioned on the PR. If the index are included, the test will break once they're fixed, but if you want, it could be included and I would need to change the expected df.

Copy link
Member

Choose a reason for hiding this comment

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

What do you mean by "the test will break once they're fixed"?

Copy link
Contributor

Choose a reason for hiding this comment

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

What @guipleite is trying to say is that when you use the left-join, there is a problem with the index values output. The bug is related with the issues @hugoecarl pointed here in the PR. With that been said, if we include index as a part of the expected df, we would have to force the wrong output to prove that the bug pointed on this PR is solved, then when those other issues are corrected, this test would be outdated. Does it make sense to you?

Copy link
Member

Choose a reason for hiding this comment

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

IIUC you are saying there are multiple bugs and this solves one of them, but the other would ned to be solved separately right? If so is there an open issue for that? Might have missed it in this PR

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes! This is particularly solving issue #28189 . This other bug that we accidentally found have already been reported in issues #28220 and #28243 .

Copy link
Member

Choose a reason for hiding this comment

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

Hmm but don't those issues have to do with missing data on merge? Maybe missing the point but don't see how applicable here. Isn't the expected index still just Index(Categorical(["1"] * 4), name="idx")?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think maybe I'm missing the point here, so I'll take a step back. We worked on issue #28189 and the problem was that merged = pd.merge(pdf, agg, how="left", left_index=True, right_index=True) with groupby and categorical index was returning an error when it should do the same thing as pd.merge(pdf, agg, how="left", on="idx") which worked. We understood that in a Cython file there was no support ford type int8 and int16, so we added. After that we discovered the for some unknown reason the index values output was int64 where it should be categorical. After a lot of debugging we concluded that we could not find the problem but we decided do make this pull request for the int8 and int16 support. This other issues mentioned show some bugs related to merge function and index output. We thought that maybe this is relatable with this bugs we are having, that were actually hidden by the int8 and int16 non-support.
At this point we think we reached our limit and to continue this, help would be needed.

Copy link
Member

Choose a reason for hiding this comment

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

Hmm OK - I think I understand now. So this change doesn't raise an error but left_index=True and right_index=True still would not yield the same output as on="idx" since the former would drop the index, right?

Copy link
Contributor

Choose a reason for hiding this comment

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

Exactly that! We just added the support to int8 and int16 type that uncovered this output bug.

expected = DataFrame(
np.array([[0, 6], [1, 6], [2, 6], [3, 6]]), columns=["value_x", "value_y"]
)

assert_frame_equal(expected, result)