impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Csaba Ringhofer (Code Review)" <>
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. (

Change subject: IMPALA-5736: Add impala-shell argument to set default query options

Patch Set 8:

File shell/
PS6, Line 1332: if __name__ == "__main__":
> typo. "be set be set"
File shell/
PS7, Line 686: 
> "_set" implies this is a set, but it's actually a dict. I think "new_query_
PS7, Line 1332: if __name__ == "__main__":
> nit: missing article
File shell/
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.
PS7, Line 42: if isinstance(default_options[option], bool)
> The recommended[0] way is to use isinstance() [1]
PS7, Line 58:     return config_filename
> What if none of the cases match?
Thanks, that is a bug actually.
PS7, Line 66:   "[impala]":
> Can you explain what exactly gets converted into what? Strings into Python 
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
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
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
File tests/shell/
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
To unsubscribe, visit

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 <>
Gerrit-Reviewer: Csaba Ringhofer <>
Gerrit-Reviewer: Lars Volker <>
Gerrit-Reviewer: Matthew Jacobs <>
Gerrit-Reviewer: Michael Brown <>
Gerrit-Reviewer: Philip Zeyliger <>
Gerrit-Reviewer: anujphadke <>
Gerrit-Comment-Date: Thu, 19 Oct 2017 15:07:00 +0000
Gerrit-HasComments: Yes

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