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

Add cartodbfy step for create_table_from_query and copy_table functions #1744

Conversation

Mmoncadaisla
Copy link
Contributor

@Mmoncadaisla Mmoncadaisla commented Aug 27, 2021

Context

After this PR, the cartodbfication step for to_carto was removed from the ContextManager's _drop_create_table_from_query method to be performed later on this function instead.

However, this has introduced a bug, causing that both create_table_from_query and copy_table functions (which relied on this step for cartodbfying tables) result in non-cartodbfied tables.

Further context can be found here: https://app.clubhouse.io/cartoteam/story/175987/economicalinsurance-cartoframes-datasets-generated-from-query-are-not-cartodbfyed

Relevant PR changes

  • cartoframes/io/managers/context_manager.py Add cartodbfy parameter with default value True for create_table_from_query method to perform the table cartodbfication

  • cartoframes/io/carto.py Add cartodbfy parameter for create_table_from_query and copy_table functions and pass it to ContextManager's create_table_from_query method

  • tests/unit/io/managers/test_context_manager.py Add test to verify that the cartodbfication step is executed via execute_long_running_query method

  • tests/unit/io/test_carto.py Add tests to verify that the cartodbfy parameter is passed as expected to ContextManager's create_table_from_query method

@shortcut-integration
Copy link

@Mmoncadaisla Mmoncadaisla requested a review from Jesus89 August 27, 2021 11:00
@Mmoncadaisla Mmoncadaisla self-assigned this Aug 27, 2021
Copy link
Member

@Jesus89 Jesus89 left a comment

Choose a reason for hiding this comment

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

I think this logic can be moved to context_manager.create_table_from_query with a parameter cartodbfy=True like in the context_manager.copy_* functions. It would be nice to add tests for the affected functions.

Copy link
Member

@Jesus89 Jesus89 left a comment

Choose a reason for hiding this comment

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

LGTM.

Note: we should add some tests for create_table_from_query. It's good to have tests to check the default values, in the case cartodbfy=True

@Mmoncadaisla
Copy link
Contributor Author

Mmoncadaisla commented Aug 27, 2021

@Jesus89 I've just added a test to verify the cartodbfication step is executed when calling the ContextManager's create_table_from_query method, please let me know if you think it's ok and/or if there is any other additional test we should add as part of this PR 🙏🏼

@Jesus89
Copy link
Member

Jesus89 commented Aug 27, 2021

I would add also tests without the cartodbfy parameter, to check that the default value is properly set in all the functions

@Mmoncadaisla
Copy link
Contributor Author

Thank you @Jesus89 (sorry I had misread it at first!) added tests to verify that the input default cartodbfy parameter is True in copy_table, create_table_from_query and ContextManager's create_table_from_query 🙂

@Mmoncadaisla Mmoncadaisla merged commit 30709bf into develop Sep 1, 2021
@Mmoncadaisla Mmoncadaisla deleted the feature/ch175987/economicalinsurance-cartoframes-datasets branch September 1, 2021 16:08
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