Michael Brown has posted comments on this change. ( http://gerrit.cloudera.org:8080/8038 )
Change subject: IMPALA-5736: Add impala-shell argument to set default query options ...................................................................... Patch Set 7: (3 comments) http://gerrit.cloudera.org:8080/#/c/8038/7/shell/option_parser.py File shell/option_parser.py: http://gerrit.cloudera.org:8080/#/c/8038/7/shell/option_parser.py@42 PS7, Line 42: if default_options[option] in [True, False]: > How about "if type(...) is bool:" ? The recommended[0] way is to use isinstance() [1] [0] https://www.python.org/dev/peps/pep-0008/ [1] https://docs.python.org/2/library/functions.html#isinstance http://gerrit.cloudera.org:8080/#/c/8038/7/shell/option_parser.py@91 PS7, Line 91: upper > Is this needed? If so, can you add a comment explaining why? Dictionary comprehensions were only added in Python 2.7 [0]. Impala has users using older OSs that won't have Python 2.7. If this is needed, please re-write it not to use the comprehension. [0] https://docs.python.org/2.7/whatsnew/2.7.html#python-3-1-features http://gerrit.cloudera.org:8080/#/c/8038/7/tests/shell/test_shell_interactive.py File tests/shell/test_shell_interactive.py: http://gerrit.cloudera.org:8080/#/c/8038/7/tests/shell/test_shell_interactive.py@300 PS7, Line 300: What about testing for: 1. query options that don't exist or don't parse? 2. query option with an invalid value -- To view, visit http://gerrit.cloudera.org:8080/8038 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I26a3b67230c80a99bd246b6af205d558fec9a986 Gerrit-Change-Number: 8038 Gerrit-PatchSet: 7 Gerrit-Owner: Csaba Ringhofer <csringho...@cloudera.com> Gerrit-Reviewer: Csaba Ringhofer <csringho...@cloudera.com> Gerrit-Reviewer: Lars Volker <l...@cloudera.com> Gerrit-Reviewer: Matthew Jacobs <mjac...@apache.org> Gerrit-Reviewer: Michael Brown <mi...@cloudera.com> Gerrit-Reviewer: Philip Zeyliger <phi...@cloudera.com> Gerrit-Reviewer: anujphadke <apha...@cloudera.com> Gerrit-Comment-Date: Wed, 18 Oct 2017 20:34:20 +0000 Gerrit-HasComments: Yes
