FEAT: Added load_data to sqlalchemy's backend #1981

xmnlab · 2019-09-25T21:23:25Z

In this PR:

Added load_data
Allow database parameter for load/create/drop when database parameter is the same than the current database (fixes 'PostgreSQLClient' object has no attribute 'engine' #1979)
Added has_attachment to AlchemyClient, useful for some operations that need to use schema when database attachment is used, for example to_sql.

ibis/sql/alchemy.py

ibis/tests/all/test_client.py

xmnlab

thanks for the review @ian-r-rose

ibis/sql/alchemy.py

ibis/tests/all/test_client.py

xmnlab · 2020-03-24T19:44:21Z

this PR is ready for review. thanks!

xmnlab · 2020-03-25T18:47:33Z

CI is green now.

jreback · 2020-03-27T19:46:12Z

docs/source/release.rst

@@ -15,6 +15,7 @@ Release Notes
 * :feature:`2060` Add initial support for ibis.random function
 * :support:`2107` Added fragment_size to table creation for OmniSciDB
 * :feature:`2117` Add non-nullable info to schema output
+* :feature:`1981` Add load_data to sqlalchemy's backends


actually can you add an entry for the postrgres bug fix, ping on green.

xmnlab · 2020-03-30T02:33:46Z

@jreback CI is green. thanks!

xmnlab · 2020-04-01T00:33:16Z

@jreback a gentle reminder about this PR :)

jreback

pls
really really try to do one change per PR
missing things makes reviews 10x harder and will take much longer

jreback · 2020-04-01T00:37:39Z

ci/docker-compose.yml

@@ -10,7 +10,7 @@ services:
      POSTGRES_PASSWORD: ''

  mysql:
-    image: mariadb:10.2
+    image: mariadb:10.4.12


do we actually need this new? iow are we now not supporting an older version?

generally we should not touch anything in the CI except for a dedicated PR

this way we don’t regress

also we want to test oldest versions that are supported and not newest

Sorry, I forgot to mention about this change.

MariaDB had an issue that you can check it here: MariaDB/mariadb-docker#262 (comment)

so as I was running tests locally that hit some issue due MYSQL_INITDB_SKIP_TZINFO I needed to change that.

I can open a separated PR for that if you want.

jreback · 2020-04-01T00:38:43Z

ibis/impala/client.py

@@ -516,7 +516,7 @@ def insert(
        )
        return self._execute(statement)

-    def load_data(self, path, overwrite=False, partition=None):
+    def load_data(self, path, overwrite=False, partition=None, **kwargs):


why do we need kwargs? generally we want to explicitly list all parameters if possible

as I am adding a test for all backends .. it simplified the test. but I can add specific arguments for each backend.

jreback · 2020-04-01T00:39:27Z

ibis/sql/alchemy.py

@@ -1099,6 +1100,10 @@ def begin(self):

    @invalidates_reflection_cache
    def create_table(self, name, expr=None, schema=None, database=None):
+        # reset database if it is the same one used by the connection
+        if database == self.database_name:


why is this needed?

I hit #1979 testing load_data method locally. Just reseting the database variable when it is the same as sef.database_name fixed the issue.

another way to fix that maybe would be replace self.engine.url.database by self.database_name

jreback · 2020-04-01T00:39:47Z

ibis/sql/alchemy.py

+
+        Raises
+        ------
+        NotImplementedError


it is related to this:

raise NotImplementedError( 'Loading data to a table from a different database is not ' 'yet implemented' )

probably the original message is much better. I will use that for the documentation.

xmnlab

thanks @jreback for the review. I will apply your suggestion.

sorry if sometimes it looks a big PR .. but as sometimes it touches more than 1 issue for one specific testing I am doing locally .. so it is hard to keep it in different PRs

xmnlab · 2020-04-01T00:54:41Z

ci/docker-compose.yml

@@ -10,7 +10,7 @@ services:
      POSTGRES_PASSWORD: ''

  mysql:
-    image: mariadb:10.2
+    image: mariadb:10.4.12


Sorry, I forgot to mention about this change.

MariaDB had an issue that you can check it here: MariaDB/mariadb-docker#262 (comment)

so as I was running tests locally that hit some issue due MYSQL_INITDB_SKIP_TZINFO I needed to change that.

I can open a separated PR for that if you want.

xmnlab · 2020-04-01T01:06:58Z

ibis/sql/alchemy.py

@@ -1099,6 +1100,10 @@ def begin(self):

    @invalidates_reflection_cache
    def create_table(self, name, expr=None, schema=None, database=None):
+        # reset database if it is the same one used by the connection
+        if database == self.database_name:


I hit #1979 testing load_data method locally. Just reseting the database variable when it is the same as sef.database_name fixed the issue.

another way to fix that maybe would be replace self.engine.url.database by self.database_name

xmnlab · 2020-04-01T01:09:01Z

ibis/sql/alchemy.py

+
+        Raises
+        ------
+        NotImplementedError


it is related to this:

raise NotImplementedError( 'Loading data to a table from a different database is not ' 'yet implemented' )

probably the original message is much better. I will use that for the documentation.

xmnlab · 2020-04-01T01:11:55Z

ibis/impala/client.py

@@ -516,7 +516,7 @@ def insert(
        )
        return self._execute(statement)

-    def load_data(self, path, overwrite=False, partition=None):
+    def load_data(self, path, overwrite=False, partition=None, **kwargs):


as I am adding a test for all backends .. it simplified the test. but I can add specific arguments for each backend.

… issuer.

xmnlab · 2020-05-26T21:14:47Z

this PR is ready again for a new review. thanks!

jreback · 2020-05-26T23:28:21Z

thanks @xmnlab

ian-r-rose reviewed Sep 27, 2019

View reviewed changes

ibis/sql/alchemy.py Outdated Show resolved Hide resolved

ibis/tests/all/test_client.py Outdated Show resolved Hide resolved

xmnlab changed the title ~~ENH: Added load_data to sqlalchemy's beckend~~ ENH: Added load_data to sqlalchemy's backend Oct 2, 2019

xmnlab commented Oct 4, 2019

View reviewed changes

ibis/sql/alchemy.py Outdated Show resolved Hide resolved

ibis/tests/all/test_client.py Outdated Show resolved Hide resolved

xmnlab changed the title ~~ENH: Added load_data to sqlalchemy's backend~~ FEAT: Added load_data to sqlalchemy's backend Nov 20, 2019

xmnlab force-pushed the fix-psql-create-table branch from efb199d to 71c7b32 Compare November 20, 2019 15:59

xmnlab added ddl Issues related to creating or altering data definitions feature Features or general enhancements postgres The PostgreSQL backend sqlalchemy SQLAlchemy-based backends labels Mar 6, 2020

xmnlab force-pushed the fix-psql-create-table branch from 83ce743 to 70c3769 Compare March 24, 2020 17:01

xmnlab marked this pull request as ready for review March 24, 2020 19:42

xmnlab mentioned this pull request Mar 24, 2020

DISCUSSION: DDL create_table from a dataframe #2151

Closed

xmnlab force-pushed the fix-psql-create-table branch from b64fd20 to f7a2e2a Compare March 25, 2020 16:05

jreback approved these changes Mar 27, 2020

View reviewed changes

jreback added this to the Next Feature Release milestone Mar 27, 2020

jreback reviewed Mar 27, 2020

View reviewed changes

xmnlab force-pushed the fix-psql-create-table branch from f7a2e2a to e116d4a Compare March 29, 2020 23:52

jreback suggested changes Apr 1, 2020

View reviewed changes

xmnlab commented Apr 1, 2020

View reviewed changes

xmnlab force-pushed the fix-psql-create-table branch 2 times, most recently from ba5f2a3 to 0f22a66 Compare April 8, 2020 15:12

Added load_data; fixed create_table and dropt_table database paramete…

66b1f77

… issuer.

xmnlab force-pushed the fix-psql-create-table branch from 2d4619d to 66b1f77 Compare May 26, 2020 19:26

jreback approved these changes May 26, 2020

View reviewed changes

jreback merged commit f99b642 into ibis-project:master May 26, 2020

xmnlab deleted the fix-psql-create-table branch June 3, 2020 21:02

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

FEAT: Added load_data to sqlalchemy's backend #1981

FEAT: Added load_data to sqlalchemy's backend #1981

xmnlab commented Sep 25, 2019 •

edited

Loading

xmnlab left a comment

xmnlab commented Mar 24, 2020

xmnlab commented Mar 25, 2020

jreback Mar 27, 2020

xmnlab commented Mar 30, 2020

xmnlab commented Apr 1, 2020

jreback left a comment

jreback Apr 1, 2020

xmnlab Apr 1, 2020

jreback Apr 1, 2020

xmnlab Apr 1, 2020

jreback Apr 1, 2020

xmnlab Apr 1, 2020

jreback Apr 1, 2020

xmnlab Apr 1, 2020

xmnlab left a comment

xmnlab Apr 1, 2020

xmnlab Apr 1, 2020

xmnlab Apr 1, 2020

xmnlab Apr 1, 2020

xmnlab commented May 26, 2020

jreback commented May 26, 2020

FEAT: Added load_data to sqlalchemy's backend #1981

FEAT: Added load_data to sqlalchemy's backend #1981

Conversation

xmnlab commented Sep 25, 2019 • edited Loading

xmnlab left a comment

Choose a reason for hiding this comment

xmnlab commented Mar 24, 2020

xmnlab commented Mar 25, 2020

Choose a reason for hiding this comment

xmnlab commented Mar 30, 2020

xmnlab commented Apr 1, 2020

jreback left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xmnlab left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xmnlab commented May 26, 2020

jreback commented May 26, 2020

xmnlab commented Sep 25, 2019 •

edited

Loading