impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Csaba Ringhofer (Code Review)" <ger...@cloudera.org>
Subject [Impala-ASF-CR] IMPALA-5736: Add impala-shell argument to set default query options
Date Thu, 19 Oct 2017 15:07:00 GMT
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 <csringhofer@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <csringhofer@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mjacobs@apache.org>
Gerrit-Reviewer: Michael Brown <mikeb@cloudera.com>
Gerrit-Reviewer: Philip Zeyliger <philip@cloudera.com>
Gerrit-Reviewer: anujphadke <aphadke@cloudera.com>
Gerrit-Comment-Date: Thu, 19 Oct 2017 15:07:00 +0000
Gerrit-HasComments: Yes

Mime
  • Unnamed multipart/alternative (inline, 8-Bit, 0 bytes)
View raw message