-
Notifications
You must be signed in to change notification settings - Fork 98
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
Remove PandasToCSVCollector
, PandasToDataFrameCollector
#378
Conversation
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? |
PandasToCSVCollector
, PandasToCSVCollector
PandasToCSVCollector
, PandasToDataFrameCollector
jenkins test this please |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a good change, I've got a few comments for you.
Generally this seems like an in-between change towards making _es_results
a true iterator but that can still be handled in a separate change too.
eland/operations.py
Outdated
|
||
return collector._ret | ||
df = self._es_results(query_compiler, show_progress) | ||
df.to_csv(**kwargs) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
return df.to_csv()
? We should add a type and test for this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So, we actually won't return this, the path of the file is passed in kwargs, where the csv has to be ingested to.
We already have good tests in tests\dataframe\test_to_csv_pytest.py
What do you think ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From reading pandas.DataFrame.to_csv()
it looks like if path_or_buf
is None
then the CSV is returned as a string. We should probably do and test that as well.
3d14588
to
dad552a
Compare
jenkins test this please |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks for this! Simplifies this logic immensely.
This PR is a small change that removes
PandasToCSVCollector
,PandasToDataFrameCollector
This has a basic functionality, But at the moment I think its not required.
If any additional functionality comes, I think we can achieve it without a class!
@sethmlarson Take a look at it :)