Skip to content
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 bug with empty dataframe number of rows #205

Merged

Conversation

mesejo
Copy link
Contributor

@mesejo mesejo commented May 4, 2020

This is an attempt at solving the bug with the wrong shape, instead of only passing the sort field, one could pass the index and query the len in the constructor. An enhancement will be to create a unique constructor in SizeTask with the logic.

Closes #152

@elasticmachine
Copy link

Since this is a community submitted pull request, a Jenkins build has not been kicked off automatically. Can an Elastic organization member please verify the contents of this patch and then kick off a build manually?

@sethmlarson
Copy link
Contributor

jenkins test this please

@sethmlarson
Copy link
Contributor

@mesejo Nice! Let me know when you're ready for review. :)

@mesejo
Copy link
Contributor Author

mesejo commented May 5, 2020

@sethmlarson For my part the only thing I think is missing is refactoring the SizeTask constructor, given that all the class children share the same behavior. Let me know what do you think.

@sethmlarson
Copy link
Contributor

sethmlarson commented May 5, 2020

@mesejo That sounds fine to me! Also don't worry about the CI failure, it's a transient error.

@mesejo mesejo changed the title [WIP] Fix bug with empty dataframe number of rows Fix bug with empty dataframe number of rows May 6, 2020
@mesejo
Copy link
Contributor Author

mesejo commented May 6, 2020

@sethmlarson Feel free to review it when possible

@sethmlarson
Copy link
Contributor

jenkins test this please

Copy link
Contributor

@sethmlarson sethmlarson left a comment

Choose a reason for hiding this comment

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

This looks pretty good to me, two small comments.
Also I think we have an extraneous import, should be caught by nox -s lint?

eland/tests/dataframe/test_head_tail_pytest.py Outdated Show resolved Hide resolved
Copy link
Contributor

@sethmlarson sethmlarson left a comment

Choose a reason for hiding this comment

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

LGTM :shipit:

@sethmlarson sethmlarson merged commit bfd0ee6 into elastic:master May 6, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Empty DataFrames don't have proper number of rows after .head()/.tail()
3 participants