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.

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

Add the util function as_dict #390

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

rafaelreuber
Copy link
Contributor

Add the util function as_dict. This is very useful to create APIs.

Copy link
Contributor

@timarmstrong timarmstrong left a comment

Choose a reason for hiding this comment

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

There's already a way to do something similar to this by setting dictify=true in the cursor constructor. I can see that this is still useful if you have an existing cursor and want to convert it.

That feature is missing from the README, so I think we should document it at the very least. I'm open to having this function too, but it'd be good to have a basic test so that nobody breaks it in the future.

@@ -64,6 +64,18 @@ def as_pandas(cursor, coerce_float=False):
coerce_float=coerce_float)


def as_dict(cursor):
Copy link
Contributor

Choose a reason for hiding this comment

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

I think as_dicts() would be a clearer name (it sounds like it returns a single dictionary).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You are right! Another option is dictfetchall. This name refers to the fetchall operation that occurs on the function.

Copy link
Contributor

Choose a reason for hiding this comment

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

Either is fine by me. It looks like the dictfetchall is used by django so there's some precedent.

@@ -64,6 +64,18 @@ def as_pandas(cursor, coerce_float=False):
coerce_float=coerce_float)


def as_dict(cursor):
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you add a test for this method. E.g. see impala/tests/test_hive_dict_cursor.py

@rafaelreuber
Copy link
Contributor Author

Hi timarmstrong,

What you think about both as_dicts and as_pandas as methods from HiveServer2Cursor.

class HiveServer2Cursor(Cursor):

    def as_dicts(self):
        pass
    def as_pandas(self):
        pass

I think this provides a concise interface.

with conn.cursor() as cursor:
    cursor.execute('SELECT * FROM mytable LIMIT 100')
    data = cursor.as_dict()

with conn.cursor() as cursor:
    cursor.execute('SELECT * FROM mytable LIMIT 100')
    df = cursor.as_pandas()	

And of course the class HiveServer2DictCursor could be removed.

@timarmstrong
Copy link
Contributor

I think those are both useful methods. My ask is to not remove any existing functionality that people may depend on, but adding new convenience methods seems like a good idea.

@dknupp
Copy link
Contributor

dknupp commented Mar 24, 2020

@rafaelreuber @timarmstrong -- Does adding an as_pandas() method imply that pandas is being added as a library dependency?

@timarmstrong
Copy link
Contributor

We already have an as_pandas method, but it only imports pandas inside the method. So I guess it's only a dependency if you want to use that method.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants