-
Notifications
You must be signed in to change notification settings - Fork 63
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
Check if_exists options #1325
Check if_exists options #1325
Conversation
Jesus89
commented
Dec 10, 2019
- Check if_exists options (fail, replace, append)
- Refactor encode_geometry function
- Add param validation unit tests
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.
Nice tests.
Some minors, and a doubt about the wkt
condition in the encode_geometry method.
@@ -57,6 +58,9 @@ def copy_from(self, cdf, table_name, if_exists='fail', cartodbfy=True, log_enabl | |||
'Please choose a different `table_name` or use ' | |||
'if_exists="replace" to overwrite it'.format( | |||
table_name=table_name, schema=schema)) | |||
else: | |||
# 'append' |
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.
A nice case where a comment adds meaningful information. Thanks ❤️
cartoframes/utils/geom_utils.py
Outdated
"""Encode geometry: wkt or wkb""" | ||
wkt = True | ||
if isinstance(geom, shapely.geometry.base.BaseGeometry): | ||
if wkt: |
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.
mmm, I don't get this condition, wkt
is always True, right?
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.
Sorry, I left the flag. I will split this into two functions.
I have tested both ways of uploading data:
EWKT
EWKB hex
There is no clear winner :S. The second one is ~5% smaller, but it is not a big win. Do you think there is an advantage in the server-side using one over the other?
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.
I'd vote for EKWB hex
if it's smaller. Also I guess that format is closer to the one PostGIS uses to store geometries (if not the same) so we might be saving computing cycles as well.
|
||
# When | ||
with pytest.raises(ValueError) as e: | ||
to_carto(df, '__table_name__', if_exists='keep_calm') |
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.
keep_calm... and carry on
Co-Authored-By: Alberto Romeu <alrocar@users.noreply.github.com>
acceptance 🍏 I've resolved conflicts and waiting for green tests to merge to develop |