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 column names in df upload v2 #974

Merged
merged 37 commits into from
Sep 24, 2019

Conversation

simon-contreras-deel
Copy link
Contributor

@simon-contreras-deel simon-contreras-deel commented Sep 12, 2019

fix: #922

In addition to the issue info, I am solving another problem: until now, we were filtering column names, getting the geometry and getting column names in different places (table creation, copy query, _rows, ...) As a result, we could get a different number of columns in different places. Now, we are going to use DataframeColumnsInfo to get all the info once at the begining.

@simon-contreras-deel
Copy link
Contributor Author

simon-contreras-deel commented Sep 19, 2019

There are several changes, so it will need acceptance in addition to the CR

Copy link
Contributor

@alrocar alrocar left a comment

Choose a reason for hiding this comment

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

Nice tests for a cumbersome problem.

I left a couple of comments and a question.

Feel free to merge this as it is.

cartoframes/data/dataset/registry/dataframe_dataset.py Outdated Show resolved Hide resolved
cartoframes/data/dataset/registry/dataframe_dataset.py Outdated Show resolved Hide resolved
test/data/dataset/test_dataset.py Outdated Show resolved Hide resolved
@alrocar
Copy link
Contributor

alrocar commented Sep 19, 2019

Wait, I leave acceptance for someone else.

Maybe @esloho can help with acceptance.

@simon-contreras-deel
Copy link
Contributor Author

Tips for acceptance:

@Jesus89
Copy link
Member

Jesus89 commented Sep 24, 2019

OK LGTM. It seems that the upload is working 🎉 I have tested the publication too.

I have some recommendations:

  • I have tried incorrect values of if_exists, like if_exists="replacex", and there is no error with the available values of the parameter. It would be nice if we have some error or warning to clarify the user.
  • I have published a map with a SQL + Maps API key. I would expect also a warning message telling the user that this API key has also SQL permissions or access to other datasets. I don't know if this is possible or not, but it would be nice to avoid security issues.

@simon-contreras-deel
Copy link
Contributor Author

Ok cool. I am going to merge it. About the suggestions, let us discuss them in next iterations. Thanks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Dataset(df).upload() doesn't work if the only columns are the_geom and cartodb_id
5 participants