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.

Sign up for GitHub

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

Jump to bottom

FEAT: OmniSciDB - Refactor/Fix DDL and Client; Add temporary parameter to create_table and "force" parameter to drop_view #2086

Merged
merged 3 commits into from
Mar 23, 2020
Merged

FEAT: OmniSciDB - Refactor/Fix DDL and Client; Add temporary parameter to create_table and "force" parameter to drop_view #2086

merged 3 commits into from
Mar 23, 2020

Conversation

xmnlab
Copy link
Contributor

@xmnlab xmnlab commented Feb 21, 2020 •
edited
Loading

In this PR:

depends on #2117 and #2104

@xmnlab xmnlab mentioned this pull request Feb 27, 2020
Merged
@xmnlab xmnlab force-pushed the omniscidb-add-more-ddl-operations branch from a7f092a to f11fa72 Compare February 28, 2020 01:18
@pep8speaks
Copy link

pep8speaks commented Feb 28, 2020 •
edited
Loading

Hello @xmnlab! 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-21 00:49:37 UTC

@xmnlab
Copy link
Contributor Author

xmnlab commented Feb 28, 2020

pep8speaks seems wrong here.

@xmnlab xmnlab force-pushed the omniscidb-add-more-ddl-operations branch from f11fa72 to cd66e62 Compare March 3, 2020 23:26
@xmnlab xmnlab mentioned this pull request Mar 5, 2020
Closed
@xmnlab xmnlab changed the title FEAT: OmniSciDB - Add more DDL operations FEAT: OmniSciDB - Refactor/Fix DDL module and add rename_columns and rename_user methods. Mar 6, 2020
@xmnlab xmnlab added ddl Issues related to creating or altering data definitions feature Features or general enhancements omnisci labels Mar 6, 2020
@xmnlab xmnlab marked this pull request as ready for review March 7, 2020 00:39
This was referenced Mar 7, 2020
Closed
Closed
@xmnlab xmnlab force-pushed the omniscidb-add-more-ddl-operations branch from 4322243 to db99b17 Compare March 10, 2020 14:18
@xmnlab xmnlab requested a review from jreback March 10, 2020 15:37
@xmnlab
Copy link
Contributor Author

xmnlab commented Mar 10, 2020 •
edited
Loading

this PR is ready for review. thanks!

jreback
jreback suggested changes Mar 12, 2020
View reviewed changes
Copy link
Contributor

@jreback jreback left a comment

Choose a reason for hiding this comment

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

adding User ddl seems out of scope. Try to lmit PRs to one thing. and do refactors in a separate PR, otherwise its very hard to tell what is going on .

@xmnlab
Copy link
Contributor Author

xmnlab commented Mar 12, 2020

@jreback as currently user ddl methods are already in the code .. what is your suggestion? open a PR to remove that from the code?

@xmnlab
Copy link
Contributor Author

xmnlab commented Mar 16, 2020

@jreback any feedback about my previous question? #2086 (comment)

@xmnlab xmnlab force-pushed the omniscidb-add-more-ddl-operations branch from db99b17 to e7f6de9 Compare March 16, 2020 19:38
jreback
jreback suggested changes Mar 17, 2020
View reviewed changes
ibis/expr/datatypes.py Outdated Show resolved Hide resolved
ibis/omniscidb/client.py Outdated Show resolved Hide resolved
ibis/omniscidb/tests/test_ddl.py Outdated Show resolved Hide resolved
ibis/omniscidb/tests/test_ddl.py Outdated Show resolved Hide resolved
ibis/tests/all/test_client.py Outdated Show resolved Hide resolved
ibis/tests/all/test_client.py Outdated
]
)

table_name = 'test_nullable_output_{}'.format(backend.name)
Copy link
Contributor

Choose a reason for hiding this comment

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

what is the purpose of this test?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it checks if the table created with non-nullable field will return correctly this field as non-nullable

@jreback
Copy link
Contributor

jreback commented Mar 17, 2020

pls try to limit scope in PRs.

@xmnlab xmnlab force-pushed the omniscidb-add-more-ddl-operations branch from 53d96d6 to 786d6cc Compare March 17, 2020 15:48
@xmnlab xmnlab changed the title FEAT: OmniSciDB - Refactor/Fix DDL module and add rename_columns and rename_user methods. FEAT: OmniSciDB - Refactor/Fix DDL and Client module Mar 17, 2020
@xmnlab xmnlab mentioned this pull request Mar 17, 2020
Closed
@xmnlab xmnlab force-pushed the omniscidb-add-more-ddl-operations branch from 15c2d3c to a74f9ef Compare March 17, 2020 21:36
xmnlab
xmnlab commented Mar 17, 2020
View reviewed changes
Makefile Outdated
@@ -8,7 +8,7 @@ MAKEFILE_DIR = $(patsubst %/,%,$(dir $(abspath $(lastword $(MAKEFILE_LIST)))))
# and `./ci/docker-compose.yml`)
# you can use `3.6` or `3.7` for now
PYTHON_VERSION := 3.6
PYTHONHASHSEED := "random"
PYTHONHASHSEED := random
Copy link
Contributor Author

Choose a reason for hiding this comment

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

#2140

@xmnlab xmnlab changed the title FEAT: OmniSciDB - Refactor/Fix DDL and Client module FEAT: OmniSciDB - Refactor/Fix DDL and Client; Add temporary parameter to create_table and "force" parameter to drop_view Mar 18, 2020
jreback
jreback suggested changes Mar 20, 2020
View reviewed changes
Copy link
Contributor

@jreback jreback left a comment

Choose a reason for hiding this comment

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

look ok, a few comments, pls rebase

ibis/omniscidb/client.py
@@ -649,7 +586,8 @@ def __init__(

def __del__(self):
"""Close the connection when instance is deleted."""
self.close()
if hasattr(self, 'con') and self.con:
Copy link
Contributor

Choose a reason for hiding this comment

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

why woudn't this always be true?

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 didn't investigate too much the reason. but I had this issue many times playing with the client.

ibis/tests/all/conftest.py
@@ -366,3 +371,24 @@ def get_common_spark_testing_client(data_directory, connect):
df_udf_random.createOrReplaceTempView('udf_random')

return _spark_testing_client


@pytest.fixture
Copy link
Contributor

Choose a reason for hiding this comment

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

suprised we don't already have something like this, can you check (and if not, ok, follows to use this fixture would be good), can you create an issue

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thanks @jreback . done: #2148

@xmnlab xmnlab force-pushed the omniscidb-add-more-ddl-operations branch from 913427d to 766ab2f Compare March 21, 2020 00:00
jreback
jreback reviewed Mar 22, 2020
View reviewed changes
Copy link
Contributor

@jreback jreback left a comment

Choose a reason for hiding this comment

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

question

ibis/omniscidb/client.py
@@ -989,16 +929,18 @@ def create_view(self, name, expr, database=None):
statement = ddl.CreateView(name, select, database=database)
self._execute(statement)

def drop_view(self, name, database=None):
def drop_view(self, name, database=None, force: bool = False):
Copy link
Contributor

Choose a reason for hiding this comment

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

is force a common name across other back ends for this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, it is a common name for operations like drop, drop_table, drop_view and drop_database across other backends

@jreback jreback added this to the Next Feature Release milestone Mar 23, 2020
@jreback jreback merged commit e2381fc into ibis-project:master Mar 23, 2020
@xmnlab xmnlab deleted the omniscidb-add-more-ddl-operations branch December 21, 2020 18:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jreback jreback jreback approved these changes

Assignees
No one assigned
Labels
ddl Issues related to creating or altering data definitions feature Features or general enhancements
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

TST: Add temp_table and temp_view fixture BUG: OmniSciDB backend doesn't honor non-nullable fields
3 participants
@xmnlab @pep8speaks @jreback