Csaba Ringhofer 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 8: (11 comments) http://gerrit.cloudera.org:8080/#/c/8038/6/shell/impala_shell.py File shell/impala_shell.py: http://gerrit.cloudera.org:8080/#/c/8038/6/shell/impala_shell.py@1332 PS6, Line 1332: if __name__ == "__main__": > typo. "be set be set" Done http://gerrit.cloudera.org:8080/#/c/8038/7/shell/impala_shell.py File shell/impala_shell.py: http://gerrit.cloudera.org:8080/#/c/8038/7/shell/impala_shell.py@686 PS7, Line 686: > "_set" implies this is a set, but it's actually a dict. I think "new_query_ Done http://gerrit.cloudera.org:8080/#/c/8038/7/shell/impala_shell.py@1332 PS7, Line 1332: if __name__ == "__main__": > nit: missing article Done 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@28 PS7, Line 28: # EXPLAIN_LEVEL=2 > are these case sensitive? Otherwise I'd opt for consistency with the previo The 'set' command is case insensitive, but it prints query option in upper case. This is why I thought that people expect these to be upper case. http://gerrit.cloudera.org:8080/#/c/8038/7/shell/option_parser.py@42 PS7, Line 42: if isinstance(default_options[option], bool) > The recommended[0] way is to use isinstance() [1] Done http://gerrit.cloudera.org:8080/#/c/8038/7/shell/option_parser.py@58 PS7, Line 58: return config_filename > What if none of the cases match? Thanks, that is a bug actually. http://gerrit.cloudera.org:8080/#/c/8038/7/shell/option_parser.py@66 PS7, Line 66: "[impala]": > Can you explain what exactly gets converted into what? Strings into Python Done http://gerrit.cloudera.org:8080/#/c/8038/7/shell/option_parser.py@72 PS7, Line 72: Overrides the defaults of the query options. Not validated here, > Can you add a comment explaining what the keys and values of those dictiona Done http://gerrit.cloudera.org:8080/#/c/8038/7/shell/option_parser.py@91 PS7, Line 91: > Dictionary comprehensions were only added in Python 2.7 [0]. Impala has use Is their a required python version for Impala? I thought that python 2.7 is ubiquitous these days. http://gerrit.cloudera.org:8080/#/c/8038/7/shell/option_parser.py@167 PS7, Line 167: " May either be a copy of Impala's certificate (for self-signed " > I think we should mention that the section titles are case sensitive Done 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: assert "INVALID_QUERY_OPTION is not supported for the impalad being " > What about testing for: Invalid value: currently their is no type validation, the 'set' command will accept anything, and the next query will fail if the value is invalid: ERROR: Errors parsing query options esfsdfs is not valid for mt_dop. Valid values are in [0, 64]. All query options are integers, with the exception of SUPPORT_START_OVER: [false]. -- 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: 8 Gerrit-Owner: Csaba Ringhofer <[email protected]> Gerrit-Reviewer: Csaba Ringhofer <[email protected]> Gerrit-Reviewer: Lars Volker <[email protected]> Gerrit-Reviewer: Matthew Jacobs <[email protected]> Gerrit-Reviewer: Michael Brown <[email protected]> Gerrit-Reviewer: Philip Zeyliger <[email protected]> Gerrit-Reviewer: anujphadke <[email protected]> Gerrit-Comment-Date: Thu, 19 Oct 2017 15:07:00 +0000 Gerrit-HasComments: Yes
