Enhancements to athena-cli by bryanck · Pull Request #36 · guardian/athena-cli

bryanck · 2018-09-02T04:16:08Z

Hi, we are using athena-cli across our company, and made a few enhancements based on feedback from users. We'd love to get your feedback on these changes and hopefully merge them in.

Some changes include:

Support different output formats in shell mode
Fixes to character set handling
Iterate over results where possible to reduce memory use
allow lower case commands to be multiline
added an option to truncate the width of the aligned format and default to line wrap
a couple of minor bug fixes

satterly · 2018-09-04T14:37:49Z

-    row_count = len(results['ResultSet']['Rows'])
-    if headers and len(results['ResultSet']['Rows']) and results['ResultSet']['Rows'][0]['Data'][0].get('VarCharValue', None) == headers[0]:
-        row_count -= 1  # don't count header
+    #tabulate() only supports ascii


Is this true for Python 3?

It seems to not be an issue in Python 3, I refactored this a little though and removed the comment

I was going to suggest athena CLI completely drop support for Python 2.7 because cmd2, it's major dependency, has dropped support as of the end of last month. https://github.com/python-cmd2/cmd2#python-27-support-is-eol

So the charset problem might be resolved by just reverting the earlier hack. See #34

We're using Python 3, so it's fine by us. I'll update the PR with some encoding cleanup and put in a python 3 shebang

I went ahead and made the changes, let me know what you think.

satterly · 2018-09-04T14:40:24Z


        if status == 'SUCCEEDED':
-            output_results(self.athena, self.format, execution_id, sys.stdout)
+            output_results(self.athena, self.format, execution_id, sys.stdout, False)


For boolean parameters I would include the paramater name eg ..., is_shell=False).

Yes, agreed. Done.

satterly · 2018-09-04T14:40:29Z

        if status == 'SUCCEEDED':
            process = subprocess.Popen(self.pager, stdin=subprocess.PIPE)
-            row_count = output_results(self.athena, self.format, self.execution_id,  process.stdin)
+            row_count = output_results(self.athena, self.format, self.execution_id, process.stdin, True)


satterly

Thanks for the PR. I don't work for the Guardian anymore but I'll see if I can get someone to merge this (with a few minor changes).

bryanck · 2018-09-04T20:31:56Z

Cool, thanks for the feedback and consideration

satterly · 2018-09-05T06:58:13Z

@@ -1,3 +1,4 @@
+#!/usr/bin/env python3


Instead of adding a shebang here you should add "python_requires" option to setup.py. See https://packaging.python.org/guides/distributing-packages-using-setuptools/#python-requires

Agreed. I'll check that in in a few

I set the python version requirement to 3.5 and also set a cmd2 version requirement

This line can be removed. It's ignored anyway as this is a python console script not a shell script.

OK I removed it

satterly · 2018-09-05T07:00:32Z

        self.athena = athena
        self.dbname = db
-        self.format = format
+        self.format = 'CSV' if format is None else format


A more idiomatic way of writing this is ...

self.format = format or 'CSV'

However, why did you remove the default format from the method signature?

The default wasn't being used anymore, it is always set, so it felt redundant having the default

I updated the code as you suggested...

satterly · 2018-09-05T07:01:35Z

 class AthenaShell(cmd.Cmd, object):

-    multilineCommands = ['WITH', 'SELECT', 'ALTER', 'CREATE', 'DESCRIBE', 'DROP', 'MSCK', 'SHOW', 'USE', 'VALUES']
+    multiline_commands = ['WITH', 'SELECT', 'ALTER', 'CREATE', 'DESCRIBE', 'DROP', 'MSCK', 'SHOW', 'USE', 'VALUES', 'with', 'select', 'alter', 'create', 'describe', 'drop', 'msck', 'show', 'use', 'values']


Why is it necessary to repeat the list of valid commands as both upper- and lowercase?

cmd2 seems to be case-sensitive with this feature, i.e. without the lower case commands in the list, then lower case statements aren't multiline

This could be rewritten as ...

multiline_commands = ['WITH', 'SELECT', 'ALTER', 'CREATE', 'DESCRIBE', 'DROP', 'MSCK', 'SHOW', 'USE', 'VALUES'] multiline_commands.extend([c.lower() for c in multiline_commands])

... to avoid the possibility of missing the fact you need to add the command twice if new commands are added.

Yes that's better. Checked in.

satterly · 2018-09-05T07:05:10Z

        help_output = """
 Supported commands:
 QUIT
+EXIT


Do EXIT and QUIT do exactly the same thing?

yes, "exit" is supported by other CLIs (like the presto-cli) so some users liked to have it

You're right. https://github.com/prestodb/presto/blob/master/presto-cli/src/main/java/com/facebook/presto/cli/Console.java#L229

satterly · 2018-09-05T18:41:35Z

+            if first_row == headers:
+                next(rows)
+
+            return (headers, rows)


This return has made the code after this try/exception block unreachable.

Whoops. This should be fixed now

satterly · 2018-09-06T07:32:12Z

        'Topic :: Utilities'
-    ]
+    ],
+    python_requires='>=3.5'


The tests are failing because they are being run against Python 2.7 and 3.4 but this python_requires restricts installation to Python 3.5+.

You need to update the .travis.yml file like so ...

python: - "3.5" - "3.6"

I checked in a fix

Bryan Keller and others added 7 commits August 17, 2018 20:37

support output formats in interactive mode

8b2ff9f

charset handling, format supports in shell, more

d49639e

renamed service back to athena

8272554

charset encoding fix, csv as default for batch

41f70bd

allow setting format interactively

af26147

fixed service name

b5e774f

version update

a808d4d

satterly reviewed Sep 4, 2018

View reviewed changes

satterly suggested changes Sep 4, 2018

View reviewed changes

specify arg name for boolean

0e8dbc3

Bryan Keller and others added 3 commits September 5, 2018 05:09

encoding cleanup, Python 3 only

279b22d

remove unused import

d0fbaad

use older api for python < 3.7

98fc251

satterly reviewed Sep 5, 2018

View reviewed changes

Bryan Keller added 3 commits September 5, 2018 15:12

set version requirements correctly

7f90919

Merge branch 'master' of https://github.com/braintreeps/athena-cli

a01ffc8

remove shebang

72f119d

satterly reviewed Sep 5, 2018

View reviewed changes

Bryan Keller added 5 commits September 5, 2018 19:11

lowercase commands for multiline

b69c280

fixed unreachable code

4fd817c

better fix for unreachable code

7ec5534

another fix

33a2f63

minor cleanup

81d8ce9

satterly reviewed Sep 6, 2018

View reviewed changes

braintreeps added 2 commits September 6, 2018 05:39

fixed travis config

865fe2f

removed 3.7 from travis

395fe97

Conversation

bryanck commented Sep 2, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

satterly left a comment

Choose a reason for hiding this comment

Uh oh!

bryanck commented Sep 4, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bryanck Sep 5, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

bryanck Sep 5, 2018 •

edited

Loading