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

convert directly to spark dataframe from download #113

Merged
merged 2 commits into from
Jul 14, 2023
Merged

Conversation

clu0
Copy link
Contributor

@clu0 clu0 commented Jul 11, 2023

convert directly from json to pyspark dataframe in download, avoid going through pandas dataframe and dealing with

@clu0 clu0 requested a review from axl1313 July 11, 2023 23:02
Copy link
Collaborator

@axl1313 axl1313 left a comment

Choose a reason for hiding this comment

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

code lgtm! just wanted to confirm that you tested that the pandas version still works as expected and that the pyspark version works in databricks? will approve on confirmation

(also have one small nit)

"""
res = requests.get(
cli_base_url + f"/cleansets/{cleanset_id}/columns?all={all}",
params=dict(to_spark=to_spark),
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: can we move the all query param to this argument as well instead of adding it to the url manually

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sounds good updated

@clu0
Copy link
Contributor Author

clu0 commented Jul 13, 2023

code lgtm! just wanted to confirm that you tested that the pandas version still works as expected and that the pyspark version works in databricks? will approve on confirmation

(also have one small nit)

yup things still work

Copy link
Collaborator

@axl1313 axl1313 left a comment

Choose a reason for hiding this comment

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

lgtm!

@clu0 clu0 merged commit a00820b into main Jul 14, 2023
22 checks passed
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.

2 participants