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

Read using copy #570

Merged
merged 20 commits into from
Apr 4, 2019
Merged

Read using copy #570

merged 20 commits into from
Apr 4, 2019

Conversation

simon-contreras-deel
Copy link
Contributor

@simon-contreras-deel simon-contreras-deel commented Mar 28, 2019

Solves: #563
Also solves: #212

It needs https://github.com/CartoDB/carto-python/pull/103/files before merging

cartoframes/context.py Outdated Show resolved Hide resolved
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.

Implementation looks good 👍 . Just added two minor comments.

cartoframes/context.py Outdated Show resolved Hide resolved
requirements.txt Outdated Show resolved Hide resolved
@coveralls
Copy link

coveralls commented Apr 3, 2019

Coverage Status

Coverage decreased (-0.3%) to 97.538% when pulling a44ff8c on read-with-copy into 72e6be7 on master.

cartoframes/context.py Outdated Show resolved Hide resolved
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.

For the on-going write PR I finally decided to:

  • Keep the actual context.py code as it is
  • Create a new API (see datasets.py and cartoframes.py in the PR) with the new code.

Let's see how the other PR goes and we might want to first merge the other and then adapt this PR code to include the new read code in the Dataset class and wrap it around in the CartoFrames class.

What do you think?

cartoframes/context.py Outdated Show resolved Hide resolved
cartoframes/context.py Outdated Show resolved Hide resolved
cartoframes/context.py Outdated Show resolved Hide resolved
cartoframes/context.py Outdated Show resolved Hide resolved
cartoframes/context.py Outdated Show resolved Hide resolved
if 'cartodb_id' in df.columns:
df.set_index('cartodb_id', inplace=True)

for column_name in table_columns:
Copy link
Contributor

Choose a reason for hiding this comment

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

Besides dates, are you sure that the schema will be faithfully represented from PG -> pandas.DataFrame? I know sometimes with postcodes/zipcodes leading zeros are removed when the column is erroneously converted to a numeric column instead of a string column.

Copy link
Contributor

Choose a reason for hiding this comment

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

Since now we are using COPY and not the Import API, we are creating the table with the same schema as the Dataframe has (this is being done in the write method, still in CR). So when reading the table, the dataframe should have the same structure as it had before writing.

In summary, no more type/content guessing.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

  • I have tested dates and works fine (we are doing the same in both cases)
  • About the case of postcodes/zipcodes, I want to check if I understand it well: in db we have a string field with something like 01234 and in the DataFrame we should have the same string. Right? This case works fine too

test/test_context.py Outdated Show resolved Hide resolved
test/test_cartoframes.py Outdated Show resolved Hide resolved
cartoframes/datasets.py Show resolved Hide resolved
cartoframes/__init__.py Outdated Show resolved Hide resolved
@simon-contreras-deel simon-contreras-deel changed the base branch from master to 561_copyfrom_write April 4, 2019 12:55
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.

🚀

@alrocar
Copy link
Contributor

alrocar commented Apr 4, 2019

Didn't realize tests were not passing. Let's wait to unblock the related PR.

@simon-contreras-deel
Copy link
Contributor Author

This one needs to wait until the write part will be merged

@simon-contreras-deel simon-contreras-deel changed the base branch from 561_copyfrom_write to master April 4, 2019 14:35
@simon-contreras-deel simon-contreras-deel merged commit 11f51e9 into master Apr 4, 2019
@Jesus89 Jesus89 deleted the read-with-copy branch September 30, 2019 16:47
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.

5 participants