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

Fix left join turning into outer join #19624

Merged
merged 4 commits into from Feb 10, 2018

Conversation

Projects
None yet
3 participants
@elrubio
Contributor

elrubio commented Feb 9, 2018

  • closes #19607
  • tests added / passed
  • passes git diff upstream/master -u -- "*.py" | flake8 --diff
  • whatsnew entry
@TomAugspurger

Could you also add a release note under bug fixes?

@@ -31,6 +31,11 @@ def right():
return DataFrame({'b': [300, 100, 200]}, index=[3, 1, 2])
@pytest.fixture
def right_non_unique():

This comment has been minimized.

@TomAugspurger

TomAugspurger Feb 9, 2018

Contributor

Why'd you make this a fixture? If it's only used in one place then I'd prefer it be defined where it's used.

This comment has been minimized.

@elrubio

elrubio Feb 9, 2018

Contributor

Well, the below test is just the bare minimum, I think test_join needs to cover many more situations. But I will pull it into the method for now.

def test_join_left_sequence_non_unique_index(left, right, right_non_unique):
# left join sequence of dataframes with non-unique indices (issue #19607)
joined = left.join([right_non_unique], how='left')

This comment has been minimized.

@TomAugspurger

TomAugspurger Feb 9, 2018

Contributor

Could you also build the expected output and tm.assert_frame_equal against that?

This comment has been minimized.

@elrubio

elrubio Feb 9, 2018

Contributor

That should definitely be possible

@codecov

This comment has been minimized.

codecov bot commented Feb 9, 2018

Codecov Report

Merging #19624 into master will increase coverage by 0.02%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #19624      +/-   ##
==========================================
+ Coverage   91.59%   91.62%   +0.02%     
==========================================
  Files         150      150              
  Lines       48795    48798       +3     
==========================================
+ Hits        44696    44711      +15     
+ Misses       4099     4087      -12
Flag Coverage Δ
#multiple 89.99% <100%> (+0.02%) ⬆️
#single 41.73% <0%> (ø) ⬆️
Impacted Files Coverage Δ
pandas/core/frame.py 97.42% <100%> (ø) ⬆️
pandas/plotting/_converter.py 66.95% <0%> (+1.73%) ⬆️
pandas/core/arrays/base.py 60.6% <0%> (+3.93%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a214915...0a068a0. Read the comment docs.

@jreback jreback added this to the 0.23.0 milestone Feb 10, 2018

@jreback jreback merged commit e2ea151 into pandas-dev:master Feb 10, 2018

3 checks passed

ci/circleci Your tests passed on CircleCI!
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@jreback

This comment has been minimized.

Contributor

jreback commented Feb 10, 2018

thanks @elrubio

if you'd have a look at the xref issues to see if they are closable with this fix (if they are not, would appreciate a PR for those as well!)

I think this is the same issue as #17257. maybe also related to #16304. Welcome to have a look / PR to fix. These should be the same.

harisbal pushed a commit to harisbal/pandas that referenced this pull request Feb 28, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment