Repository: CartoDB/cartoframes (language: python)

Issue:
Add cartodbfy step for create_table_from_query and copy_table functions
### Context

After [this PR](https://github.com/CartoDB/cartoframes/pull/1735), the cartodbfication step for `to_carto` was removed from the [ContextManager's `_drop_create_table_from_query` method](https://github.com/CartoDB/cartoframes/blob/develop/cartoframes/io/managers/context_manager.py#L299-L304) 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 


Discussion / hints:

Fix patch:
diff --git a/.github/workflows/cartoframes-ci.yml b/.github/workflows/cartoframes-ci.yml
index 03527e729..eb8b8ef71 100644
--- a/.github/workflows/cartoframes-ci.yml
+++ b/.github/workflows/cartoframes-ci.yml
@@ -1,4 +1,4 @@
-name: Run CARTOFrames tests
+name: CI
 
 on:
   push:
diff --git a/CHANGELOG.md b/CHANGELOG.md
index 821e243bb..8e2e9ba7b 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -5,6 +5,12 @@ All notable changes to this project will be documented in this file.
 The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/),
 and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html).
 
+## [1.2.4] - 2021-09-02
+
+### Changed
+
+- Add cartodbfication step for create_table_from_query and copy_table functions (#1744)
+
 ## [1.2.3] - 2021-08-18
 
 ### Changed
diff --git a/README.rst b/README.rst
index e09de0432..4db4a6c1c 100644
--- a/README.rst
+++ b/README.rst
@@ -4,8 +4,8 @@ CARTOframes
 
 .. image:: https://github.com/CartoDB/cartoframes/actions/workflows/cartoframes-ci.yml/badge.svg?branch=develop
     :target: https://github.com/CartoDB/cartoframes/actions/workflows/cartoframes-ci.yml
-.. image:: https://img.shields.io/badge/pypi-v1.2.3-orange
-    :target: https://pypi.org/project/cartoframes/1.2.3
+.. image:: https://img.shields.io/badge/pypi-v1.2.4-orange
+    :target: https://pypi.org/project/cartoframes/1.2.4
 
 A Python package for integrating `CARTO <https://carto.com/>`__ maps, analysis, and data services into data science workflows.
 
@@ -14,11 +14,11 @@ Python data analysis workflows often rely on the de facto standards `pandas <htt
 Try it Out
 ==========
 
-* Stable (1.2.3): |stable|
+* Stable (1.2.4): |stable|
 * Latest (develop branch): |develop|
 
 .. |stable| image:: https://mybinder.org/badge_logo.svg
-    :target: https://mybinder.org/v2/gh/cartodb/cartoframes/v1.2.3?filepath=examples
+    :target: https://mybinder.org/v2/gh/cartodb/cartoframes/v1.2.4?filepath=examples
 
 .. |develop| image:: https://mybinder.org/badge_logo.svg
     :target: https://mybinder.org/v2/gh/cartodb/cartoframes/develop?filepath=examples
diff --git a/cartoframes/_version.py b/cartoframes/_version.py
index 5a5df3bef..daab838ba 100644
--- a/cartoframes/_version.py
+++ b/cartoframes/_version.py
@@ -1,1 +1,1 @@
-__version__ = '1.2.3'
+__version__ = '1.2.4'
diff --git a/cartoframes/io/carto.py b/cartoframes/io/carto.py
index 25fe0d5c2..30272b3de 100644
--- a/cartoframes/io/carto.py
+++ b/cartoframes/io/carto.py
@@ -275,7 +275,7 @@ def rename_table(table_name, new_table_name, credentials=None, if_exists='fail',
         log.info('Success! Table "{0}" renamed to table "{1}" correctly'.format(table_name, new_table_name))
 
 
-def copy_table(table_name, new_table_name, credentials=None, if_exists='fail', log_enabled=True):
+def copy_table(table_name, new_table_name, credentials=None, if_exists='fail', log_enabled=True, cartodbfy=True):
     """Copy a table into a new table in the CARTO account.
 
     Args:
@@ -285,7 +285,8 @@ def copy_table(table_name, new_table_name, credentials=None, if_exists='fail', l
             instance of Credentials (username, api_key, etc).
         if_exists (str, optional): 'fail', 'replace', 'append'. Default is 'fail'.
         log_enabled (bool, optional): enable the logging mechanism. Default is True.
-
+        cartodbfy (bool, optional): convert the table to CARTO format. Default True. More info
+            `here <https://carto.com/developers/sql-api/guides/creating-tables/#create-tables>`.
     Raises:
         ValueError: if the table names provided are wrong or the if_exists param is not valid.
 
@@ -303,13 +304,19 @@ def copy_table(table_name, new_table_name, credentials=None, if_exists='fail', l
     query = 'SELECT * FROM {}'.format(table_name)
 
     context_manager = ContextManager(credentials)
-    new_table_name = context_manager.create_table_from_query(query, new_table_name, if_exists)
+    new_table_name = context_manager.create_table_from_query(query, new_table_name, if_exists, cartodbfy)
 
     if log_enabled:
         log.info('Success! Table "{0}" copied to table "{1}" correctly'.format(table_name, new_table_name))
 
 
-def create_table_from_query(query, new_table_name, credentials=None, if_exists='fail', log_enabled=True):
+def create_table_from_query(
+        query,
+        new_table_name,
+        credentials=None,
+        if_exists='fail',
+        log_enabled=True,
+        cartodbfy=True):
     """Create a new table from an SQL query in the CARTO account.
 
     Args:
@@ -319,7 +326,8 @@ def create_table_from_query(query, new_table_name, credentials=None, if_exists='
             instance of Credentials (username, api_key, etc).
         if_exists (str, optional): 'fail', 'replace', 'append'. Default is 'fail'.
         log_enabled (bool, optional): enable the logging mechanism. Default is True.
-
+        cartodbfy (bool, optional): convert the table to CARTO format. Default True. More info
+            `here <https://carto.com/developers/sql-api/guides/creating-tables/#create-tables>`.
     Raises:
         ValueError: if the query or table name provided is wrong or the if_exists param is not valid.
 
@@ -335,7 +343,7 @@ def create_table_from_query(query, new_table_name, credentials=None, if_exists='
             ', '.join(IF_EXISTS_OPTIONS)))
 
     context_manager = ContextManager(credentials)
-    new_table_name = context_manager.create_table_from_query(query, new_table_name, if_exists)
+    new_table_name = context_manager.create_table_from_query(query, new_table_name, if_exists, cartodbfy)
 
     if log_enabled:
         log.info('Success! Table "{0}" created correctly'.format(new_table_name))
diff --git a/cartoframes/io/managers/context_manager.py b/cartoframes/io/managers/context_manager.py
index b975beb96..360998bd7 100644
--- a/cartoframes/io/managers/context_manager.py
+++ b/cartoframes/io/managers/context_manager.py
@@ -122,7 +122,7 @@ def copy_from(self, gdf, table_name, if_exists='fail', cartodbfy=True,
 
         return table_name
 
-    def create_table_from_query(self, query, table_name, if_exists):
+    def create_table_from_query(self, query, table_name, if_exists, cartodbfy=True):
         schema = self.get_schema()
         table_name = self.normalize_table_name(table_name)
 
@@ -140,6 +140,10 @@ def create_table_from_query(self, query, table_name, if_exists):
         else:
             self._drop_create_table_from_query(table_name, schema, query)
 
+        if cartodbfy is True:
+            cartodbfy_query = _cartodbfy_query(table_name, schema)
+            self.execute_long_running_query(cartodbfy_query)
+
         return table_name
 
     def list_tables(self, schema=None):


Test patch:
diff --git a/tests/notebooks/requirements.txt b/tests/notebooks/requirements.txt
index 67d51022b..3e397b7b2 100644
--- a/tests/notebooks/requirements.txt
+++ b/tests/notebooks/requirements.txt
@@ -1,4 +1,4 @@
-notebook==6.1.5
+notebook==6.4.1
 pytest==6.1.1
 pytest-mock==3.3.1
 nbconvert==6.0.7
diff --git a/tests/unit/io/managers/test_context_manager.py b/tests/unit/io/managers/test_context_manager.py
index 54cf399bf..f40cd5cf8 100644
--- a/tests/unit/io/managers/test_context_manager.py
+++ b/tests/unit/io/managers/test_context_manager.py
@@ -278,3 +278,29 @@ def __init__(self):
 
         with pytest.raises(CartoRateLimitException):
             test_function(retry_times=0)
+
+    def test_create_table_from_query_cartodbfy(self, mocker):
+        # Given
+        mocker.patch.object(ContextManager, 'has_table', return_value=False)
+        mocker.patch.object(ContextManager, 'get_schema', return_value='schema')
+        mock = mocker.patch.object(ContextManager, 'execute_long_running_query')
+
+        # When
+        cm = ContextManager(self.credentials)
+        cm.create_table_from_query('SELECT * FROM table_name', '__new_table_name__', if_exists='fail', cartodbfy=True)
+
+        # Then
+        mock.assert_called_with("SELECT CDB_CartodbfyTable('schema', '__new_table_name__')")
+
+    def test_create_table_from_query_cartodbfy_default(self, mocker):
+        # Given
+        mocker.patch.object(ContextManager, 'has_table', return_value=False)
+        mocker.patch.object(ContextManager, 'get_schema', return_value='schema')
+        mock = mocker.patch.object(ContextManager, 'execute_long_running_query')
+
+        # When
+        cm = ContextManager(self.credentials)
+        cm.create_table_from_query('SELECT * FROM table_name', '__new_table_name__', if_exists='fail')
+
+        # Then
+        mock.assert_called_with("SELECT CDB_CartodbfyTable('schema', '__new_table_name__')")
diff --git a/tests/unit/io/test_carto.py b/tests/unit/io/test_carto.py
index 4acb40458..b3638f21f 100644
--- a/tests/unit/io/test_carto.py
+++ b/tests/unit/io/test_carto.py
@@ -500,6 +500,39 @@ def test_copy_table_wrong_if_exists(mocker):
     assert str(e.value) == 'Wrong option for the `if_exists` param. You should provide: fail, replace, append.'
 
 
+def test_copy_table_no_cartodbfy(mocker):
+    # Given
+    cm_mock = mocker.patch.object(ContextManager, 'create_table_from_query')
+
+    # When
+    copy_table('__table_name__', '__new_table_name__', CREDENTIALS, cartodbfy=False)
+
+    # Then
+    assert cm_mock.call_args[0][3] is False
+
+
+def test_copy_table_cartodbfy(mocker):
+    # Given
+    cm_mock = mocker.patch.object(ContextManager, 'create_table_from_query')
+
+    # When
+    copy_table('__table_name__', '__new_table_name__', CREDENTIALS, cartodbfy=True)
+
+    # Then
+    assert cm_mock.call_args[0][3] is True
+
+
+def test_copy_table_cartodbfy_default(mocker):
+    # Given
+    cm_mock = mocker.patch.object(ContextManager, 'create_table_from_query')
+
+    # When
+    copy_table('__table_name__', '__new_table_name__', CREDENTIALS)
+
+    # Then
+    assert cm_mock.call_args[0][3] is True
+
+
 def test_create_table_from_query_wrong_query(mocker):
     # When
     with pytest.raises(ValueError) as e:
@@ -535,3 +568,36 @@ def test_create_table_from_query_wrong_if_exists(mocker):
 
     # Then
     assert str(e.value) == 'Wrong option for the `if_exists` param. You should provide: fail, replace, append.'
+
+
+def test_create_table_from_query_no_cartodbfy(mocker):
+    # Given
+    cm_mock = mocker.patch.object(ContextManager, 'create_table_from_query')
+
+    # When
+    create_table_from_query('SELECT * FROM table', '__new_table_name__', CREDENTIALS, cartodbfy=False)
+
+    # Then
+    assert cm_mock.call_args[0][3] is False
+
+
+def test_create_table_from_query_cartodbfy(mocker):
+    # Given
+    cm_mock = mocker.patch.object(ContextManager, 'create_table_from_query')
+
+    # When
+    create_table_from_query('SELECT * FROM table', '__new_table_name__', CREDENTIALS, cartodbfy=True)
+
+    # Then
+    assert cm_mock.call_args[0][3] is True
+
+
+def test_create_table_from_query_cartodbfy_default(mocker):
+    # Given
+    cm_mock = mocker.patch.object(ContextManager, 'create_table_from_query')
+
+    # When
+    create_table_from_query('SELECT * FROM table', '__new_table_name__', CREDENTIALS)
+
+    # Then
+    assert cm_mock.call_args[0][3] is True