Skip to content

dataframe.filter should preserve order of items - #283

Merged
sethmlarson merged 4 commits into
elastic:masterfrom
V1NAY8:issue/245
Oct 13, 2020
Merged

dataframe.filter should preserve order of items#283
sethmlarson merged 4 commits into
elastic:masterfrom
V1NAY8:issue/245

Conversation

@V1NAY8

@V1NAY8 V1NAY8 commented Oct 12, 2020

Copy link
Copy Markdown
Contributor

Closes #245

  • Added typehints wherever necessary w.r.t filter
    Example:
>>> ed_flights = ed.DataFrame("localhost", "flights")
>>> ed_flights.filter(axis="index", items = ["1", "2", "0"])
   AvgTicketPrice  Cancelled           Carrier                                          Dest  ... OriginRegion OriginWeather dayOfWeek           timestamp
1      882.982662      False  Logstash Airways                     Venice Marco Polo Airport  ...        SE-BD         Clear         0 2018-01-01 18:27:00
2      190.636904      False  Logstash Airways                     Venice Marco Polo Airport  ...        IT-34          Rain         0 2018-01-01 17:11:14
0      841.265642      False   Kibana Airlines  Sydney Kingsford Smith International Airport  ...        DE-HE         Sunny         0 2018-01-01 00:00:00

@sethmlarson Please review. 😄

@elasticmachine

elasticmachine commented Oct 12, 2020

Copy link
Copy Markdown

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
sethmlarson suggested changes Oct 12, 2020
View reviewed changes

@sethmlarson sethmlarson left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This mostly looks good to me, thanks! One comment:

Comment thread eland/actions.py Outdated
@sethmlarson

sethmlarson commented Oct 12, 2020

Copy link
Copy Markdown
Contributor

jenkins test this please

@V1NAY8

V1NAY8 commented Oct 13, 2020 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

There's a recent change to mypy which is causing issues
I see from build output eland/utils.py:70: error: Value of type variable "_LT" of "sorted" cannot be "Item".
Trying to resolve this.
Reference:
python/mypy#9582

@V1NAY8

V1NAY8 commented Oct 13, 2020

Copy link
Copy Markdown
Contributor Author

I have resolved the mypy issue.

@V1NAY8
V1NAY8 requested a review from sethmlarson October 13, 2020 12:50
sethmlarson
sethmlarson reviewed Oct 13, 2020
View reviewed changes
Comment thread eland/utils.py


def try_sort(iterable: Iterable[Item]) -> Iterable[Item]:
def try_sort(iterable: Iterable[str]) -> Iterable[str]:

@sethmlarson sethmlarson Oct 13, 2020

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This works for us I believe but I wonder what the general solution to this issue is.

@V1NAY8 V1NAY8 Oct 13, 2020

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I tried looking for solution, Tried a lot of combinations :) But couldnt find any. 😕

sethmlarson
sethmlarson approved these changes Oct 13, 2020
View reviewed changes

@sethmlarson sethmlarson left a comment •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM, will merge once test suite passes :) Thanks so much!!

(Btw I renamed the parameter maintain_index_order to sort_index_by_ids)

@sethmlarson

sethmlarson commented Oct 13, 2020

Copy link
Copy Markdown
Contributor

jenkins test this please

@V1NAY8

V1NAY8 commented Oct 13, 2020

Copy link
Copy Markdown
Contributor Author

Thanks, I need to work on naming variables! 😮

@sethmlarson

sethmlarson commented Oct 13, 2020

Copy link
Copy Markdown
Contributor

@V1NAY8 Naming is one of the famous unsolved problems in software engineering ;)

@sethmlarson
sethmlarson merged commit b7c6c26 into elastic:master Oct 13, 2020
@V1NAY8
V1NAY8 deleted the issue/245 branch October 13, 2020 16:00
@V1NAY8

V1NAY8 commented Oct 13, 2020

Copy link
Copy Markdown
Contributor Author

Could you please add hacktoberfest-accepted label ? Thanks

@hauntsaninja hauntsaninja mentioned this pull request May 19, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer
@sethmlarson sethmlarson sethmlarson approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

hacktoberfest-accepted

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

DataFrame.filter(axis="index", items=[...]) should preserve order of items

3 participants

@V1NAY8 @elasticmachine @sethmlarson