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

Append common actions to the PandasClient #1464

Closed
wants to merge 11 commits into
base: master
from

Conversation

Projects
3 participants
@tonyfast
Contributor

tonyfast commented Jun 1, 2018

This pull request adds list_tables, load_data, and create_table attributes for the PandasClient.

@cpcloud cpcloud self-assigned this Jun 1, 2018

@cpcloud cpcloud added this to To do in Pandas via automation Jun 1, 2018

@cpcloud cpcloud added this to the 0.14 milestone Jun 1, 2018

@cpcloud

Thanks for doing this! A few comments.

return list(filter(lambda t: pattern.findall(t), tables))
return tables

This comment has been minimized.

@cpcloud

cpcloud Jun 1, 2018

Member

Kill this newline.

dtypes = dict(ibis_schema_to_pandas(schema))
df = pd.DataFrame(columns=list(dtypes.keys())).astype(dtypes)
self.dictionary[table_name] =df

This comment has been minimized.

@cpcloud

cpcloud Jun 1, 2018

Member

Spacing after the = in =df

-------
if_exists : boolean
"""
return bool(self.list_tables(like=name))

This comment has been minimized.

@cpcloud

cpcloud Jun 1, 2018

Member

Could also implement this as name in self.dictionary which avoids going through every table.

@@ -35,12 +35,19 @@ def test_client_table(table):
def test_client_table_repr(table):
assert 'PandasTable' in repr(table)
def test_load_data(client):

This comment has been minimized.

@cpcloud

cpcloud Jun 1, 2018

Member

use two newlines after function definitions.

df = pd.DataFrame(obj)
else:
dtypes = dict(ibis_schema_to_pandas(schema))
df = pd.DataFrame(columns=list(dtypes.keys())).astype(dtypes)

This comment has been minimized.

@cpcloud

cpcloud Jun 1, 2018

Member

you should be able to write this as: schema.apply_to(pd.DataFrame()).

def test_literal(client):
lit = ibis.literal(1)
result = client.execute(lit)
assert result == 1
def test_list_tables(client):

This comment has been minimized.

@cpcloud

cpcloud Jun 1, 2018

Member

two newlines

def test_literal(client):
lit = ibis.literal(1)
result = client.execute(lit)
assert result == 1
def test_list_tables(client):
assert len(client.list_tables()) > 0

This comment has been minimized.

@cpcloud

cpcloud Jun 1, 2018

Member

you can implement this as assert client.list_tables()

tables = list(self.dictionary.keys())
if like is not None:
pattern = re.compile(like)
return list(filter(lambda t: pattern.findall(t), tables))

This comment has been minimized.

@cpcloud

cpcloud Jun 1, 2018

Member

Is there any particular reason you chose to use pattern.findall here instead of using re.search and checking if the match result is None? We only need to find the first matching occurrence and then terminate. Using findall will return every matching non-overlapping substring, which seems a bit wasteful.

@tonyfast

This comment has been minimized.

Contributor

tonyfast commented Jun 4, 2018

Thanks for the comments. Are these failing circle tests something that this PR caused?

@cpcloud

This comment has been minimized.

Member

cpcloud commented Jun 4, 2018

@tonyfast nope, I'm working on fixing them in #1458

@cpcloud

LGTM, except for the exists_table implementation. After that is fixed this is ready to merge.

-------
if_exists : boolean
"""
return name in self.list_tables()

This comment has been minimized.

@cpcloud

cpcloud Jun 4, 2018

Member

I still think this should be name in self.dictionary. No reason to make this unnecessarily slow.

@xmnlab

This comment has been minimized.

Contributor

xmnlab commented Jun 4, 2018

@cpcloud

This comment has been minimized.

Member

cpcloud commented Jun 4, 2018

@tonyfast rebase on master and you'll be able to get past any failures that aren't related this PR.

@cpcloud

This comment has been minimized.

Member

cpcloud commented Jun 4, 2018

@tonyfast Looks like there's another issue, putting a PR up now to fix it.

@cpcloud

This comment has been minimized.

Member

cpcloud commented Jun 4, 2018

#1468 should fix these errors.

# kwargs is a catch all for any options required by other backends.
self.dictionary[table_name] = pd.DataFrame(obj)
def create_table(self, table_name, obj=None, schema=None):

This comment has been minimized.

@cpcloud

cpcloud Jun 4, 2018

Member

Can you add a test for this?

@cpcloud

This comment has been minimized.

Member

cpcloud commented Jun 4, 2018

@tonyfast it looks like you're out of date with master, can you rebase again?

@cpcloud

cpcloud approved these changes Jun 4, 2018

LGTM

@tonyfast

This comment has been minimized.

Contributor

tonyfast commented Jun 4, 2018

👌Thanks @cpcloud!

Pandas automation moved this from To do to Done Jun 4, 2018

@cpcloud cpcloud closed this in f7392fa Jun 4, 2018

@cpcloud

This comment has been minimized.

Member

cpcloud commented Jun 4, 2018

thanks @tonyfast!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment