FEAT: [OmnisciDB] Support add_columns and drop_columns for OmnisciDB table #2094

YarShev · 2020-02-25T12:36:46Z

No description provided.

pep8speaks · 2020-02-25T12:36:50Z

Hello @YarShev! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻

Comment last updated at 2020-03-24 18:30:42 UTC

anmyachev · 2020-02-25T23:35:59Z

@YarShev please fix docstrings

YarShev · 2020-02-26T06:07:16Z

@YarShev please fix docstrings

thanks, fixed

semelianova · 2020-02-26T06:32:48Z

ibis/omniscidb/ddl.py

+        -------
+        string
+        """
+        unsupported_types = ['POINT',


Why these types are unsupported? Omnisci supports geo functions and types, there are tests for them.

I read it at the Omnisci documentation

I found it in examples from doc page
ALTER TABLE t ADD COLUMN pt_dropoff POINT DEFAULT 'point(0 0)';

Clause 'Note' say that "Currently, OmniSci does not support adding a geo column type (POINT, LINESTRING, POLYGON, or MULTIPOLYGON) to a table"

Sorry, didn't notice

I checked it, it is working fine on omnisci master branch.

Then may delete it, thanks

semelianova · 2020-02-26T06:46:37Z

ibis/omniscidb/client.py

@@ -905,6 +905,44 @@ def drop_table(self, table_name, database=None, force=False):
        self._execute(statement, False)
        self.set_database(_database)

+    def add_column(self, table_name, column_name, data_type):


I think it is possible to add several columns at a time through giving a list of column names and a list of data types. How do you like it? Can you implement it?

It can be implemented, but I think it's enough.

It will allow getting more performance because we will send and execute one query while adding several columns. It's not so difficult to do so I suggest doing it. It's up to you.

I will implement method for some columns.

anmyachev · 2020-02-26T11:06:28Z

@YarShev do make develop to avoiding code style issues

xmnlab · 2020-02-27T01:00:59Z

ibis/omniscidb/client.py

+        Examples
+        --------
+        >>> table_name = 'my_table'
+        >>> columns_with_types = my_column_1 = 'INTEGER',


it seems this line is raising an error on doctest tests.
maybe you could use a dictionary here.

problem was relating to writing multiple lines.

ibis/omniscidb/ddl.py

xmnlab

@YarShev thanks for working on that!

I just added some comments here. thanks!

ibis/omniscidb/ddl.py

xmnlab · 2020-02-27T01:24:09Z

ibis/omniscidb/tests/test_client.py

@@ -100,6 +100,53 @@ def test_union_op(alltypes):
        expr.compile()


+def test_add_column(con):
+    table_name = 'my_table'


could you add test for:

0 field

1 field (done)

2 or more fields (with different types)

thanks!

@YarShev thanks for adding more parameters for the tests here!

I think you could to refactor a little bit your code here. you can try something like this:

@pytest.mark.parametrize( 'cols_with_types', [ {}, {'c': 'double'}, {'c': 'double', 'd': 'string', 'e': 'point', 'f': 'polygon', 'g': 'int16'}, ], ) def test_add_column(con, new_cols): table_name = 'my_table' con.drop_table(table_name, force=True) schema = ibis.schema([('a', 'float'), ('b', 'int8')]) con.create_table(table_name, schema=schema) if len(new_cols) == 0: with pytest.raises(IbisInputError): con.add_column(table_name, cols_with_types) return con.drop_table(table_name) con.add_column(table_name, new_cols) schema_new_cols = ibis.schema(new_cols.items()) schema_new = schema.append(schema_new_cols) try: t = con.table(table_name) assert schema == schema_new finally: con.drop_table(table_name)

note: probably for ddl we should use the same type names used by ibis expressions .. not the ones directly used by omniscidb. it could be a little bit confusing . I have a WIP PR that I was adding the add_columns op too. As you are working on that I will just remove that from my code ... but for now you can take a look how I was treating the datatypes there: #2086

semelianova · 2020-02-27T10:00:46Z

ibis/omniscidb/tests/test_client.py

+
+    con.create_table(table_name, schema=schema)
+
+    dict_cols_with_types = {'c': 'DOUBLE', 'd': 'TEXT'}


I think you need to test adding columns with more types, at least one with geo type.

xmnlab

@YarShev thanks! it seems it is almost ready :) I added some more comments. thanks again!

xmnlab · 2020-02-27T20:18:29Z

ibis/omniscidb/tests/test_client.py

@@ -100,6 +100,53 @@ def test_union_op(alltypes):
        expr.compile()


+def test_add_column(con):
+    table_name = 'my_table'


@YarShev thanks for adding more parameters for the tests here!

I think you could to refactor a little bit your code here. you can try something like this:

@pytest.mark.parametrize( 'cols_with_types', [ {}, {'c': 'double'}, {'c': 'double', 'd': 'string', 'e': 'point', 'f': 'polygon', 'g': 'int16'}, ], ) def test_add_column(con, new_cols): table_name = 'my_table' con.drop_table(table_name, force=True) schema = ibis.schema([('a', 'float'), ('b', 'int8')]) con.create_table(table_name, schema=schema) if len(new_cols) == 0: with pytest.raises(IbisInputError): con.add_column(table_name, cols_with_types) return con.drop_table(table_name) con.add_column(table_name, new_cols) schema_new_cols = ibis.schema(new_cols.items()) schema_new = schema.append(schema_new_cols) try: t = con.table(table_name) assert schema == schema_new finally: con.drop_table(table_name)

note: probably for ddl we should use the same type names used by ibis expressions .. not the ones directly used by omniscidb. it could be a little bit confusing . I have a WIP PR that I was adding the add_columns op too. As you are working on that I will just remove that from my code ... but for now you can take a look how I was treating the datatypes there: #2086

ibis/omniscidb/tests/test_client.py

YarShev · 2020-03-16T15:53:44Z

@jreback , just friendly reminder.

YarShev · 2020-03-20T09:00:10Z

@jreback , just friendly reminder to review this PR.

jreback · 2020-03-20T10:50:35Z

make sure the PR is all green before a ping

YarShev · 2020-03-20T13:22:06Z

@jreback , sorry, I didn't notice the problem relates with doc. Now, CI is green.

jreback · 2020-03-20T18:58:50Z

this should come after #2086 which moves around the dtype mappings.

YarShev · 2020-03-20T19:46:36Z

Ok, I will be waiting for this PR.

jreback · 2020-03-23T12:46:15Z

can you merge master and fix conflicts

YarShev · 2020-03-23T15:32:52Z

@jreback , thanks for reminder, merged master and fixed conflicts

YarShev · 2020-03-24T19:44:55Z

@jreback , just a friendly reminder.

jreback

thanks @YarShev nice patch!

…nisciDB table (ibis-project#2094)" This reverts commit b2df7cc.

YarShev requested review from jreback and xmnlab February 25, 2020 12:39

semelianova reviewed Feb 26, 2020

View reviewed changes

xmnlab reviewed Feb 27, 2020

View reviewed changes

ibis/omniscidb/ddl.py Outdated Show resolved Hide resolved

xmnlab reviewed Feb 27, 2020

View reviewed changes

semelianova reviewed Feb 27, 2020

View reviewed changes

xmnlab reviewed Feb 27, 2020

View reviewed changes

YarShev added 17 commits February 29, 2020 14:25

support add and drop column

0633fa9

fix coding style

82f0a3a

delete indent

8b1f6a2

add blank line

15f2395

fix docstrings

abafc3b

delete indents

3641f75

delete ,

19119cd

support possibility add/drop some columns

c00f518

fix

66dd5da

fix

e61f35d

apply formatters

311b875

apply comments

d85ef23

add parametrization

d50a279

fix

a8fc7c1

fix

629a917

fix

a6ca8d2

fix warning

1d26904

YarShev added 2 commits March 15, 2020 22:53

add release notes

06c64d3

combine fixtures

63f03bd

YarShev changed the title ~~FEAT: [OmnisciDB] Support add_column and drop_column for OmnisciDB table~~ FEAT: [OmnisciDB] Support add_columns and drop_columns for OmnisciDB table Mar 16, 2020

add s to methods

d74cd3f

YarShev added 2 commits March 17, 2020 09:10

Merge branch 'master' into dev/yigoshev-add-drop-column

b4c7173

Merge branch 'master' into dev/yigoshev-add-drop-column

8ed58d1

YarShev requested a review from jreback March 19, 2020 06:40

fix isort

34d9502

YarShev force-pushed the dev/yigoshev-add-drop-column branch from 92fdbdc to 34d9502 Compare March 20, 2020 12:31

fix doc

c5d9ef4

YarShev closed this Mar 20, 2020

YarShev reopened this Mar 20, 2020

YarShev added 3 commits March 23, 2020 16:01

Merge branch 'master' into dev/yigoshev-add-drop-column

a1bafef

fix

7c31d5c

fix

0daa256

Merge branch 'master' into dev/yigoshev-add-drop-column

0b3a81d

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

jreback approved these changes Mar 24, 2020

View reviewed changes

jreback merged commit b2df7cc into ibis-project:master Mar 24, 2020

YarShev deleted the dev/yigoshev-add-drop-column branch March 24, 2020 22:29

datapythonista added a commit to datapythonista/ibis that referenced this pull request Apr 22, 2020

Revert "FEAT: [OmnisciDB] Support add_columns and drop_columns for Om…

5a15339

…nisciDB table (ibis-project#2094)" This reverts commit b2df7cc.


		con.create_table(table_name, schema=schema)

		dict_cols_with_types = {'c': 'DOUBLE', 'd': 'TEXT'}

FEAT: [OmnisciDB] Support add_columns and drop_columns for OmnisciDB table #2094

FEAT: [OmnisciDB] Support add_columns and drop_columns for OmnisciDB table #2094

Conversation

YarShev commented Feb 25, 2020

pep8speaks commented Feb 25, 2020 • edited

Comment last updated at 2020-03-24 18:30:42 UTC

anmyachev commented Feb 25, 2020

YarShev commented Feb 26, 2020

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

Choose a reason for hiding this comment

YarShev Feb 26, 2020 • edited

Choose a reason for hiding this comment

anmyachev commented Feb 26, 2020

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

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

YarShev commented Mar 16, 2020

YarShev commented Mar 20, 2020

jreback commented Mar 20, 2020

YarShev commented Mar 20, 2020

jreback commented Mar 20, 2020

YarShev commented Mar 20, 2020

jreback commented Mar 23, 2020

YarShev commented Mar 23, 2020 • edited

YarShev commented Mar 24, 2020

jreback left a comment

Choose a reason for hiding this comment

pep8speaks commented Feb 25, 2020 •

edited

YarShev Feb 26, 2020 •

edited

YarShev commented Mar 23, 2020 •

edited