You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@impala.apache.org by "Csaba Ringhofer (Code Review)" <ge...@cloudera.org> on 2017/09/12 15:52:47 UTC
[Impala-ASF-CR] IMPALA-5736: Add impala-shell argument to set default query options
Csaba Ringhofer has uploaded a new change for review.
http://gerrit.cloudera.org:8080/8038
Change subject: IMPALA-5736: Add impala-shell argument to set default query options
......................................................................
IMPALA-5736: Add impala-shell argument to set default query options
Query options can be set from command line and impala rc as comma
separated key=value pairs.
Examples:
command line:
impala-shell.sh --query_options=EXPLAIN_LEVEL=2,MT_DOP=2
.impalarc:
[impala]
query_options=EXPLAIN_LEVEL=2,MT_DOP=2
Note that the option set in command line will overwrite the one
in impalarc instead of updating it as a set. For example, if
impalarc contains "query_options=EXPLAIN_LEVEL=2", and the shell
command contains --query_options="MT_DOP=2", only MT_DOP will be set,
and EXPLAIN_LEVEL will use its default value.
Change-Id: I26a3b67230c80a99bd246b6af205d558fec9a986
---
M shell/impala_shell.py
M shell/impala_shell_config_defaults.py
M shell/option_parser.py
3 files changed, 13 insertions(+), 1 deletion(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/38/8038/1
--
To view, visit http://gerrit.cloudera.org:8080/8038
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: I26a3b67230c80a99bd246b6af205d558fec9a986
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Csaba Ringhofer <cs...@cloudera.com>
[Impala-ASF-CR] IMPALA-5736: Add impala-shell argument to set default query options
Posted by "Michael Brown (Code Review)" <ge...@cloudera.org>.
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 15:
(6 comments)
I'm lukewarm on the exception hierarchy introduced. I appreciate the desire for clearer errors for the user. However, in the case of exception hierarchies, PEP-008 [0] has this to say:
> Design exception hierarchies based on the distinctions that code catching the exceptions is likely to need, rather than the locations where the exceptions are raised.
In the case of the exception hierarchy introduced in patch set 15, there isn't any catching introduced: both exceptions are raised directly to the shell user.
Is it that important, then, to introduce this exception hierarchy at all?
Again however, I appreciate the desire to have clearer errors. At a high level, there are three options I see:
1. Completely remove the exception hierarchy introduced.
2. Leave exception hierarchy in place, but trim declarations to be more conventional.
3. Leave the exception hierarchy in place, and leave its declarations in this more "verbose", unconventional state.
I'm going to leave review comments guiding toward #2. Happy to hear arguments for 1 or 3.
[0] https://www.python.org/dev/peps/pep-0008/
http://gerrit.cloudera.org:8080/#/c/8038/15/shell/option_parser.py
File shell/option_parser.py:
http://gerrit.cloudera.org:8080/#/c/8038/15/shell/option_parser.py@36
PS15, Line 36: class OptionParserError(Exception):
: """Base class for option parser exceptions."""
: def __init__(self, msg):
: self.msg = msg
I think this should be removed. The only reason to have a hierarchy like this is if calling code needs to except the whole group, i.e,
except OptionParserError:
The calling code isn't catching any of these new exceptions.
http://gerrit.cloudera.org:8080/#/c/8038/15/shell/option_parser.py@41
PS15, Line 41: FileFormatError
ConfigFileFormatError might be a clearer name.
http://gerrit.cloudera.org:8080/#/c/8038/15/shell/option_parser.py@43
PS15, Line 43: def __init__(self, msg):
: self.msg = msg
I don't you think you need to override __init__, because the Exception hierarchy already has a builtin message attribute:
>>> Exception.message
<attribute 'message' of 'exceptions.BaseException' objects>
>>>
That is, if you use the attribute "message" that already exists instead of rolling your own "msg" attribute, you can make the declaration of this exception cleaner. However, I'm not sure you need to do anything inside this class with "message" or "msg" if you follow the next comment's advice.
http://gerrit.cloudera.org:8080/#/c/8038/15/shell/option_parser.py@46
PS15, Line 46: def __str__(self):
: return "Unable to read configuration file correctly. Check formatting: %s" % self.msg
Instead of overriding __str__, it is more conventional to let the message argument when raising the exception be the full message of the exception. Let the message and clear exception class name tell the full story. Taking this comment and the previous one together, this is the more conventional way to create one's own exception:
class MyError(ParentException):
"""docstring"""
pass
# ...
raise MyError("full message of exception")
http://gerrit.cloudera.org:8080/#/c/8038/15/shell/option_parser.py@49
PS15, Line 49: InvalidValueError
InvalidOptionValueError?
http://gerrit.cloudera.org:8080/#/c/8038/15/shell/option_parser.py@51
PS15, Line 51: def __init__(self, msg):
: self.msg = msg
:
: def __str__(self):
: return "Unexpected value in configuration file. %s" % self.msg
Same comments with __init__ and __str__ apply here.
--
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: 15
Gerrit-Owner: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj...@apache.org>
Gerrit-Reviewer: Michael Brown <mi...@cloudera.com>
Gerrit-Reviewer: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Reviewer: anujphadke <ap...@cloudera.com>
Gerrit-Comment-Date: Wed, 25 Oct 2017 18:22:58 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5736: Add impala-shell argument to set default query options
Posted by "Csaba Ringhofer (Code Review)" <ge...@cloudera.org>.
Hello Lars Volker, Michael Brown, Matthew Jacobs, Philip Zeyliger, anujphadke, Impala Public Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/8038
to look at the new patch set (#18).
Change subject: IMPALA-5736: Add impala-shell argument to set default query options
......................................................................
IMPALA-5736: Add impala-shell argument to set default query options
Query options can be set from command line and impala rc as
key=value pairs, where key is case insensitive.
Examples:
command line:
impala-shell.sh -Q MT_DOP=1 --query_option=MAX_ERRORS=200
.impalarc:
[impala.query_options]
EXPLAIN_LEVEL=2
MT_DOP=2
The options set in command line will update the ones
in impalarc one by one, so the result of the example
above will be:
EXPLAIN_LEVEL=2
MT_DOP=1
MAX_ERRORS=200
Additional changes:
- 0 and 1 are accepted as bools in section [impala] to
make it more consistent with [impala.query_options]
- options that are expected to be bool but are not
0/1/true/false lead to error instead of warning
Change-Id: I26a3b67230c80a99bd246b6af205d558fec9a986
---
M bin/rat_exclude_files.txt
M shell/impala_shell.py
M shell/option_parser.py
A tests/shell/impalarc_with_error
A tests/shell/impalarc_with_query_options
A tests/shell/impalarc_with_warnings
M tests/shell/test_shell_commandline.py
M tests/shell/test_shell_interactive.py
8 files changed, 159 insertions(+), 46 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/38/8038/18
--
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: newpatchset
Gerrit-Change-Id: I26a3b67230c80a99bd246b6af205d558fec9a986
Gerrit-Change-Number: 8038
Gerrit-PatchSet: 18
Gerrit-Owner: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj...@apache.org>
Gerrit-Reviewer: Michael Brown <mi...@cloudera.com>
Gerrit-Reviewer: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Reviewer: anujphadke <ap...@cloudera.com>
[Impala-ASF-CR] IMPALA-5736: Add impala-shell argument to set default query options
Posted by "Csaba Ringhofer (Code Review)" <ge...@cloudera.org>.
Hello Lars Volker, Matthew Jacobs, Philip Zeyliger,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/8038
to look at the new patch set (#3).
Change subject: IMPALA-5736: Add impala-shell argument to set default query options
......................................................................
IMPALA-5736: Add impala-shell argument to set default query options
Query options can be set from command line and impala rc as
key=value pairs.
Examples:
command line:
impala-shell.sh --query_option=EXPLAIN_LEVEL=2 -Q MT_DOP=2
.impalarc:
[impala.query_options]
EXPLAIN_LEVEL=2
MT_DOP=2
Note that the options set in command line will update the ones
in impalarc one by one. For example, if impalarc contains
"EXPLAIN_LEVEL=2 and MT_DOP=2", and the shell command contains
--query_option="MT_DOP=1", the result will be
EXPLAIN_LEVEL=2 and MT_DOP=1.:
Change-Id: I26a3b67230c80a99bd246b6af205d558fec9a986
---
M shell/impala_shell.py
M shell/option_parser.py
2 files changed, 46 insertions(+), 27 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/38/8038/3
--
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: newpatchset
Gerrit-Change-Id: I26a3b67230c80a99bd246b6af205d558fec9a986
Gerrit-Change-Number: 8038
Gerrit-PatchSet: 3
Gerrit-Owner: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj...@apache.org>
Gerrit-Reviewer: Philip Zeyliger <ph...@cloudera.com>
[Impala-ASF-CR] IMPALA-5736: Add impala-shell argument to set default query options
Posted by "Lars Volker (Code Review)" <ge...@cloudera.org>.
Lars Volker 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:
(1 comment)
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@58
PS7, Line 58: return config_filename
> Thanks, that is a bug actually.
As discussed in person, can you please add a test for this case?
--
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 <cs...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj...@apache.org>
Gerrit-Reviewer: Michael Brown <mi...@cloudera.com>
Gerrit-Reviewer: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Reviewer: anujphadke <ap...@cloudera.com>
Gerrit-Comment-Date: Thu, 19 Oct 2017 22:40:52 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5736: Add impala-shell argument to set default query options
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins 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 17: Verified-1
Build failed: https://jenkins.impala.io/job/gerrit-verify-dryrun/1425/
--
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: 17
Gerrit-Owner: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj...@apache.org>
Gerrit-Reviewer: Michael Brown <mi...@cloudera.com>
Gerrit-Reviewer: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Reviewer: anujphadke <ap...@cloudera.com>
Gerrit-Comment-Date: Thu, 02 Nov 2017 01:28:06 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-5736: Add impala-shell argument to set default query options
Posted by "Csaba Ringhofer (Code Review)" <ge...@cloudera.org>.
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 14:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/8038/14//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/8038/14//COMMIT_MSG@30
PS14, Line 30:
> This seems to lack a _
Done
http://gerrit.cloudera.org:8080/#/c/8038/14/tests/shell/test_shell_commandline.py
File tests/shell/test_shell_commandline.py:
http://gerrit.cloudera.org:8080/#/c/8038/14/tests/shell/test_shell_commandline.py@389
PS14, Line 389: formatting
> This does not report a formatting error, so I'm inclined to remove the "Che
This part was printed by impala_shell.py, which did not differentiate between different exceptions from config_parser. I wanted to keep it this way, so I have added some exceptions that format the full error message.
--
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: 14
Gerrit-Owner: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj...@apache.org>
Gerrit-Reviewer: Michael Brown <mi...@cloudera.com>
Gerrit-Reviewer: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Reviewer: anujphadke <ap...@cloudera.com>
Gerrit-Comment-Date: Wed, 25 Oct 2017 15:48:04 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5736: Add impala-shell argument to set default query options
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins 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 18:
Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/1427/
--
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: 18
Gerrit-Owner: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj...@apache.org>
Gerrit-Reviewer: Michael Brown <mi...@cloudera.com>
Gerrit-Reviewer: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Reviewer: anujphadke <ap...@cloudera.com>
Gerrit-Comment-Date: Thu, 02 Nov 2017 20:20:26 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-5736: Add impala-shell argument to set default query options
Posted by "Csaba Ringhofer (Code Review)" <ge...@cloudera.org>.
Hello Lars Volker, Matthew Jacobs, Philip Zeyliger,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/8038
to look at the new patch set (#5).
Change subject: IMPALA-5736: Add impala-shell argument to set default query options
......................................................................
IMPALA-5736: Add impala-shell argument to set default query options
Query options can be set from command line and impala rc as
key=value pairs.
Examples:
command line:
impala-shell.sh -Q MT_DOP=1 --query_option=MAX_ERRORS=200
.impalarc:
[impala.query_options]
EXPLAIN_LEVEL=2
MT_DOP=2
The options set in command line will update the ones
in impalarc one by one, so the result of the example
above will be:
EXPLAIN_LEVEL=2
MT_DOP=1
MAX_ERRORS=200
Change-Id: I26a3b67230c80a99bd246b6af205d558fec9a986
---
M shell/impala_shell.py
M shell/option_parser.py
A tests/shell/impalarc_with_query_options
M tests/shell/test_shell_interactive.py
4 files changed, 98 insertions(+), 38 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/38/8038/5
--
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: newpatchset
Gerrit-Change-Id: I26a3b67230c80a99bd246b6af205d558fec9a986
Gerrit-Change-Number: 8038
Gerrit-PatchSet: 5
Gerrit-Owner: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj...@apache.org>
Gerrit-Reviewer: Philip Zeyliger <ph...@cloudera.com>
[Impala-ASF-CR] IMPALA-5736: Add impala-shell argument to set default query options
Posted by "Csaba Ringhofer (Code Review)" <ge...@cloudera.org>.
Hello Lars Volker, Matthew Jacobs, Philip Zeyliger, anujphadke,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/8038
to look at the new patch set (#7).
Change subject: IMPALA-5736: Add impala-shell argument to set default query options
......................................................................
IMPALA-5736: Add impala-shell argument to set default query options
Query options can be set from command line and impala rc as
key=value pairs.
Examples:
command line:
impala-shell.sh -Q MT_DOP=1 --query_option=MAX_ERRORS=200
.impalarc:
[impala.query_options]
EXPLAIN_LEVEL=2
MT_DOP=2
The options set in command line will update the ones
in impalarc one by one, so the result of the example
above will be:
EXPLAIN_LEVEL=2
MT_DOP=1
MAX_ERRORS=200
Change-Id: I26a3b67230c80a99bd246b6af205d558fec9a986
---
M shell/impala_shell.py
M shell/option_parser.py
A tests/shell/impalarc_with_query_options
M tests/shell/test_shell_interactive.py
4 files changed, 101 insertions(+), 39 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/38/8038/7
--
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: newpatchset
Gerrit-Change-Id: I26a3b67230c80a99bd246b6af205d558fec9a986
Gerrit-Change-Number: 8038
Gerrit-PatchSet: 7
Gerrit-Owner: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj...@apache.org>
Gerrit-Reviewer: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Reviewer: anujphadke <ap...@cloudera.com>
[Impala-ASF-CR] IMPALA-5736: Add impala-shell argument to set default query options
Posted by "Philip Zeyliger (Code Review)" <ge...@cloudera.org>.
Philip Zeyliger has posted comments on this change.
Change subject: IMPALA-5736: Add impala-shell argument to set default query options
......................................................................
Patch Set 1:
(3 comments)
http://gerrit.cloudera.org:8080/#/c/8038/1//COMMIT_MSG
Commit Message:
Line 12: Examples:
I think you can add a test in tests/shell/test_shell_interactive.py. test_var_substitution() has an example where options are provided. I didn't see an ~/.impalarc test.
@pytest.mark.execute_serially
def test_var_substitution(self):
cmds = open(os.path.join(QUERY_FILE_PATH, 'test_var_substitution.sql')).read()
args = '''--var=foo=123 --var=BAR=456 --delimited "--output_delimiter= " '''
result = run_impala_shell_interactive(cmds, shell_args=args)
assert_var_substitution(result)
PS1, Line 16: .impalarc:
: [impala]
: query_options=EXPLAIN_LEVEL=2,MT_DOP=2
This might be hard to do, but I think
[impala]
[[query_options]]
EXPLAIN_LEVEL=2
MT_DOP=2
may be better.
http://gerrit.cloudera.org:8080/#/c/8038/1/shell/option_parser.py
File shell/option_parser.py:
PS1, Line 160: parser.add_option("--var", dest="keyval", action="append",
: help="Define variable(s) to be used within the Impala session."
: " It must follow the pattern \"KEY=VALUE\","
: " KEY starts with an alphabetic character and"
: " contains alphanumeric characters or underscores.")
: parser.add_option("--query_options", dest="query_options",
: help="Sets default query options. The format is comma separated"
:
A little bit of bike-shedding:
Impala-shell seems to support --var a=b --var c=d, but for query options it's "--query_options=a=b,c=d". I think the former approach is preferable, because it means we don't have to worry about commas in settings. It's also consistent with --var.
--
To view, visit http://gerrit.cloudera.org:8080/8038
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I26a3b67230c80a99bd246b6af205d558fec9a986
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Philip Zeyliger <ph...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5736: Add impala-shell argument to set default query options
Posted by "Lars Volker (Code Review)" <ge...@cloudera.org>.
Lars Volker 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 17: Code-Review+2
Carrying Michael's +2
--
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: 17
Gerrit-Owner: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj...@apache.org>
Gerrit-Reviewer: Michael Brown <mi...@cloudera.com>
Gerrit-Reviewer: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Reviewer: anujphadke <ap...@cloudera.com>
Gerrit-Comment-Date: Wed, 01 Nov 2017 21:32:07 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-5736: Add impala-shell argument to set default query options
Posted by "Michael Brown (Code Review)" <ge...@cloudera.org>.
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 16: Code-Review+2
You've received several +1s along the way. I chatted with Lars when he asked me to look at this again and it's good to go for +2. However, as you might have seen on the dev mailing list [0] we are holding off submissions until builds stabilize. In the mean time, I noticed your patch contains conflicts. Want to rebase and fix the conflicts?
[0] http://mail-archives.apache.org/mod_mbox/incubator-impala-dev/201710.mbox/%3CCAM_aD9cic7XjCs8Rs_3QtcmnK-PmcrADavqNq5JzTU8f0%3DtXXA%40mail.gmail.com%3E
--
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: 16
Gerrit-Owner: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj...@apache.org>
Gerrit-Reviewer: Michael Brown <mi...@cloudera.com>
Gerrit-Reviewer: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Reviewer: anujphadke <ap...@cloudera.com>
Gerrit-Comment-Date: Mon, 30 Oct 2017 22:04:17 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-5736: Add impala-shell argument to set default query options
Posted by "Lars Volker (Code Review)" <ge...@cloudera.org>.
Lars Volker 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:
(10 comments)
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: tmp_set
"_set" implies this is a set, but it's actually a dict. I think "new_query_options" might be a better name.
http://gerrit.cloudera.org:8080/#/c/8038/7/shell/impala_shell.py@1332
PS7, Line 1332: There are two types of options: shell options and query_options. Both can be set in
nit: missing article
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 previous section.
http://gerrit.cloudera.org:8080/#/c/8038/7/shell/option_parser.py@36
PS7, Line 36: def convert_loaded_shell_option(option, value, default_options, config_filename):
: """Converts some values based on their type in default_options
:
: Setting 'config_filename' in the config file would have no effect,
: so its original value is kept.
From looking at the signature and the comment I have no idea what this method does. Can you clarify it so it makes sense without further context? The comment also should explain what the return value is.
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:" ?
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?
http://gerrit.cloudera.org:8080/#/c/8038/7/shell/option_parser.py@66
PS7, Line 66: filtered and some values are converted (False, True, None).
Can you explain what exactly gets converted into what? Strings into Python values?
http://gerrit.cloudera.org:8080/#/c/8038/7/shell/option_parser.py@72
PS7, Line 72: Returns a pair of dictionaries (shell_options, query_options)
Can you add a comment explaining what the keys and values of those dictionaries are?
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?
http://gerrit.cloudera.org:8080/#/c/8038/7/shell/option_parser.py@167
PS7, Line 167: "The following sections are used: [impala], "
I think we should mention that the section titles are case sensitive
--
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 <cs...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj...@apache.org>
Gerrit-Reviewer: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Reviewer: anujphadke <ap...@cloudera.com>
Gerrit-Comment-Date: Tue, 17 Oct 2017 23:08:54 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5736: Add impala-shell argument to set default query options
Posted by "Michael Brown (Code Review)" <ge...@cloudera.org>.
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 9:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/8038/9/shell/impala_shell.py
File shell/impala_shell.py:
http://gerrit.cloudera.org:8080/#/c/8038/9/shell/impala_shell.py@688
PS9, Line 688:
Nit: trailing white space. You can use 'git diff --check' to catch these.
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@91
PS7, Line 91: conve
> Is their a required python version for Impala? I thought that python 2.7 is
Unfortunately Python 2.7 is not ubiquitous in Linux distributions like CentOS 5 and CentOS 6.
--
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: 9
Gerrit-Owner: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj...@apache.org>
Gerrit-Reviewer: Michael Brown <mi...@cloudera.com>
Gerrit-Reviewer: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Reviewer: anujphadke <ap...@cloudera.com>
Gerrit-Comment-Date: Thu, 19 Oct 2017 16:18:57 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5736: Add impala-shell argument to set default query options
Posted by "Lars Volker (Code Review)" <ge...@cloudera.org>.
Lars Volker 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 11:
(3 comments)
http://gerrit.cloudera.org:8080/#/c/8038/11/shell/option_parser.py
File shell/option_parser.py:
http://gerrit.cloudera.org:8080/#/c/8038/11/shell/option_parser.py@36
PS11, Line 36: convert_loaded_shell_option
I still struggle to understand the function name. How about parse_shell_option(option, default_value).
http://gerrit.cloudera.org:8080/#/c/8038/11/shell/option_parser.py@37
PS11, Line 37: """Converts values 'True', 'False' and 'None' to their corresponding python values.
Mention that this parses string values into corresponding python types. The comment should also explain what happens if the parsing fails.
http://gerrit.cloudera.org:8080/#/c/8038/11/shell/option_parser.py@64
PS11, Line 64: values
types
--
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: 11
Gerrit-Owner: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj...@apache.org>
Gerrit-Reviewer: Michael Brown <mi...@cloudera.com>
Gerrit-Reviewer: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Reviewer: anujphadke <ap...@cloudera.com>
Gerrit-Comment-Date: Fri, 20 Oct 2017 22:50:35 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5736: Add impala-shell argument to set default query options
Posted by "Michael Brown (Code Review)" <ge...@cloudera.org>.
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 17:
Csaba, it's hard to see, but the build failed the Apache RAT check. If you search 'FAILED JOB' at https://jenkins.impala.io/job/gerrit-verify-dryrun/1425/console you see a pointer to https://jenkins.impala.io/job/rat-check-ub1604/111/ . In there is:
22:05:14 + bin/check-rat-report.py bin/rat_exclude_files.txt rat.xml
22:05:15 UNAPPROVED: tests/shell/impalarc_with_error
22:05:15 UNAPPROVED: tests/shell/impalarc_with_query_options
22:05:15 UNAPPROVED: tests/shell/impalarc_with_warnings
I think these exclusions can be added to bin/rat_exclude_files.txt in this section, alongside the existing shell test data exclusions:
# http://www.apache.org/legal/src-headers.html: "Test data for which the addition of a
# source header would cause the tests to fail."
--
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: 17
Gerrit-Owner: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj...@apache.org>
Gerrit-Reviewer: Michael Brown <mi...@cloudera.com>
Gerrit-Reviewer: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Reviewer: anujphadke <ap...@cloudera.com>
Gerrit-Comment-Date: Thu, 02 Nov 2017 19:57:40 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-5736: Add impala-shell argument to set default query options
Posted by "Csaba Ringhofer (Code Review)" <ge...@cloudera.org>.
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 2:
I will add unit tests if the current logic is ok for everyone.
--
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: 2
Gerrit-Owner: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj...@apache.org>
Gerrit-Reviewer: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Comment-Date: Thu, 28 Sep 2017 15:25:54 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-5736: Add impala-shell argument to set default query options
Posted by "Lars Volker (Code Review)" <ge...@cloudera.org>.
Lars Volker has posted comments on this change.
Change subject: IMPALA-5736: Add impala-shell argument to set default query options
......................................................................
Patch Set 1:
(1 comment)
Looks good to me, only a minor comment.
http://gerrit.cloudera.org:8080/#/c/8038/1/shell/impala_shell.py
File shell/impala_shell.py:
PS1, Line 1308: keyvals
Can you inline this?
--
To view, visit http://gerrit.cloudera.org:8080/8038
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I26a3b67230c80a99bd246b6af205d558fec9a986
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5736: Add impala-shell argument to set default query options
Posted by "Csaba Ringhofer (Code Review)" <ge...@cloudera.org>.
Csaba Ringhofer has posted comments on this change.
Change subject: IMPALA-5736: Add impala-shell argument to set default query options
......................................................................
Patch Set 1:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/8038/1//COMMIT_MSG
Commit Message:
PS1, Line 16: .impalarc:
: [impala]
: query_options=EXPLAIN_LEVEL=2,MT_DOP=2
> This might be hard to do, but I think
I think that it would be easier to do it like this:
[impala.query_options]
EXPLAIN_LEVEL=2
MT_DOP=2
A similar config group could be created for variables too.
If we decide to do it this way, then we should also change the overwrite behavior, because config groups are typically not overwritten as whole, but as separate configs.
http://gerrit.cloudera.org:8080/#/c/8038/1/shell/option_parser.py
File shell/option_parser.py:
PS1, Line 160: parser.add_option("--var", dest="keyval", action="append",
: help="Define variable(s) to be used within the Impala session."
: " It must follow the pattern \"KEY=VALUE\","
: " KEY starts with an alphabetic character and"
: " contains alphanumeric characters or underscores.")
: parser.add_option("--query_options", dest="query_options",
: help="Sets default query options. The format is comma separated"
:
> A little bit of bike-shedding:
We have talked about this with Lars, and decided to stick with the comma separated strings.
It is also an option to support both formats.
--
To view, visit http://gerrit.cloudera.org:8080/8038
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I26a3b67230c80a99bd246b6af205d558fec9a986
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Philip Zeyliger <ph...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5736: Add impala-shell argument to set default query options
Posted by "Csaba Ringhofer (Code Review)" <ge...@cloudera.org>.
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 4:
(10 comments)
http://gerrit.cloudera.org:8080/#/c/8038/4/shell/impala_shell.py
File shell/impala_shell.py:
http://gerrit.cloudera.org:8080/#/c/8038/4/shell/impala_shell.py@686
PS4, Line 686: tmp_set = {}
> Add a comment:
Done
http://gerrit.cloudera.org:8080/#/c/8038/4/shell/impala_shell.py@1330
PS4, Line 1330: if __name__ == "__main__":
> Perhaps we need a big comment:
Thanks for the nice comment, I made some small changes.
http://gerrit.cloudera.org:8080/#/c/8038/4/shell/impala_shell.py@1350
PS4, Line 1350: # default options loaded in from impala_shell_config_defaults.py
> Let's take the time to update this comment by disambiguating "shell options
Done
http://gerrit.cloudera.org:8080/#/c/8038/4/shell/impala_shell.py@1354
PS4, Line 1354: impala_shell_defaults.update(loaded_shell_options)
> I think "impala_query_options_default" is empty, but this assymetry
> between impala_shell_defaults and query_options is weird.
I think it is less weird now with the long comment at the beginning. Query option defaults come from the server, so the script does not know them yet.
> It's weird that you're updating impala_shell_defaults, rather than
> updating "shell_options".
I do not know why it was done this way originally.
http://gerrit.cloudera.org:8080/#/c/8038/4/shell/impala_shell.py@1437
PS4, Line 1437: query_options.update(
> Add comment:
Done
http://gerrit.cloudera.org:8080/#/c/8038/4/shell/option_parser.py
File shell/option_parser.py:
http://gerrit.cloudera.org:8080/#/c/8038/4/shell/option_parser.py@36
PS4, Line 36: def convert_loaded_shell_option(option, value, default_options, config_filename):
> Please document config_filename. I'm unclear how it's used.
Done
http://gerrit.cloudera.org:8080/#/c/8038/4/shell/option_parser.py@47
PS4, Line 47: # if the option is not set to either true or false, use the default
> Do we need to log about the ignored value here?
Done
http://gerrit.cloudera.org:8080/#/c/8038/4/shell/option_parser.py@36
PS4, Line 36: def convert_loaded_shell_option(option, value, default_options, config_filename):
: """Converts some values based on their type in default_options
: """
: if default_options[option] in [True, False]:
: # validate the option if it can only be a boolean value
: # the only choice for these options is true or false
: if value.lower() == "true":
: return True
: elif value.lower() == 'false':
: return False
: else:
: # if the option is not set to either true or false, use the default
: return default_options[option]
: elif value.lower() == "none":
: return None
: elif option.lower() == "config_file":
: return config_filename
> This function is mixing 2-space indent and 4-space indent. Please clean up.
Done
http://gerrit.cloudera.org:8080/#/c/8038/4/shell/option_parser.py@164
PS4, Line 164: "Only specify this as a option in the commandline."
> s/as a option/as an option/
Done
http://gerrit.cloudera.org:8080/#/c/8038/4/shell/option_parser.py@183
PS4, Line 183: help="Sets default query options."
> Add: "May be specified multiple times." Unless it's clear from the usage?
Done
--
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: 4
Gerrit-Owner: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj...@apache.org>
Gerrit-Reviewer: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Comment-Date: Thu, 12 Oct 2017 19:41:12 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5736: Add impala-shell argument to set default query options
Posted by "Csaba Ringhofer (Code Review)" <ge...@cloudera.org>.
Hello Lars Volker, Michael Brown, Matthew Jacobs, Philip Zeyliger, anujphadke,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/8038
to look at the new patch set (#8).
Change subject: IMPALA-5736: Add impala-shell argument to set default query options
......................................................................
IMPALA-5736: Add impala-shell argument to set default query options
Query options can be set from command line and impala rc as
key=value pairs, where key is case insensitive.
Examples:
command line:
impala-shell.sh -Q MT_DOP=1 --query_option=MAX_ERRORS=200
.impalarc:
[impala.query_options]
EXPLAIN_LEVEL=2
MT_DOP=2
The options set in command line will update the ones
in impalarc one by one, so the result of the example
above will be:
EXPLAIN_LEVEL=2
MT_DOP=1
MAX_ERRORS=200
Change-Id: I26a3b67230c80a99bd246b6af205d558fec9a986
---
M shell/impala_shell.py
M shell/option_parser.py
A tests/shell/impalarc_with_query_options
M tests/shell/test_shell_interactive.py
4 files changed, 112 insertions(+), 38 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/38/8038/8
--
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: newpatchset
Gerrit-Change-Id: I26a3b67230c80a99bd246b6af205d558fec9a986
Gerrit-Change-Number: 8038
Gerrit-PatchSet: 8
Gerrit-Owner: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj...@apache.org>
Gerrit-Reviewer: Michael Brown <mi...@cloudera.com>
Gerrit-Reviewer: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Reviewer: anujphadke <ap...@cloudera.com>
[Impala-ASF-CR] IMPALA-5736: Add impala-shell argument to set default query options
Posted by "Csaba Ringhofer (Code Review)" <ge...@cloudera.org>.
Hello Lars Volker, Matthew Jacobs, Philip Zeyliger,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/8038
to look at the new patch set (#6).
Change subject: IMPALA-5736: Add impala-shell argument to set default query options
......................................................................
IMPALA-5736: Add impala-shell argument to set default query options
Query options can be set from command line and impala rc as
key=value pairs.
Examples:
command line:
impala-shell.sh -Q MT_DOP=1 --query_option=MAX_ERRORS=200
.impalarc:
[impala.query_options]
EXPLAIN_LEVEL=2
MT_DOP=2
The options set in command line will update the ones
in impalarc one by one, so the result of the example
above will be:
EXPLAIN_LEVEL=2
MT_DOP=1
MAX_ERRORS=200
Change-Id: I26a3b67230c80a99bd246b6af205d558fec9a986
---
M shell/impala_shell.py
M shell/option_parser.py
A tests/shell/impalarc_with_query_options
M tests/shell/test_shell_interactive.py
4 files changed, 101 insertions(+), 39 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/38/8038/6
--
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: newpatchset
Gerrit-Change-Id: I26a3b67230c80a99bd246b6af205d558fec9a986
Gerrit-Change-Number: 8038
Gerrit-PatchSet: 6
Gerrit-Owner: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj...@apache.org>
Gerrit-Reviewer: Philip Zeyliger <ph...@cloudera.com>
[Impala-ASF-CR] IMPALA-5736: Add impala-shell argument to set default query options
Posted by "Philip Zeyliger (Code Review)" <ge...@cloudera.org>.
Philip Zeyliger 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 3:
(10 comments)
The UI feels good to me.
As a UI improvement, I think "impala-shell --help" should mention that it's possible to write a .impalarc file, and perhaps have a sample file.
There are three types of key-value pairs we keep track of: query options, impala shell options, and variables. I find the code already very confusing about how these three states are managed across defaults, config files, and command line options. I think making that clearer is worthwhile.
I was able to trigger a few bugs:
$impala-shell.sh
Starting Impala Shell without Kerberos authentication
Error connecting: TTransportException, Could not connect to philip-dev.gce.cloudera.com:21000
MT_DOP is not supported for the impalad being connected to, ignoring.
Traceback (most recent call last):
File "/home/philip/src/impala/shell/impala_shell.py", line 1448, in <module>
shell = ImpalaShell(options)
File "/home/philip/src/impala/shell/impala_shell.py", line 195, in __init__
self.do_connect(options.impalad)
File "/home/philip/src/impala/shell/impala_shell.py", line 686, in do_connect
for set_option in self.set_query_options:
RuntimeError: dictionary changed size during iteration
$impala-shell.sh
WARNING: Unable to read configuration file correctly. Check formatting: 'live_progress'
Starting Impala Shell without Kerberos authentication
Traceback (most recent call last):
File "/home/philip/src/impala/shell/impala_shell.py", line 1432, in <module>
options.set_query_options.update(
AttributeError: Values instance has no attribute 'set_query_options'
http://gerrit.cloudera.org:8080/#/c/8038/3//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/8038/3//COMMIT_MSG@25
PS3, Line 25: EXPLAIN_LEVEL=2 and MT_DOP=1.:
Maybe just make this the example in line 14 and 19?
http://gerrit.cloudera.org:8080/#/c/8038/3/shell/impala_shell.py
File shell/impala_shell.py:
http://gerrit.cloudera.org:8080/#/c/8038/3/shell/impala_shell.py@1430
PS3, Line 1430: options.set_query_options.update(
Just to confirm: this is command line overriding what's in the config file, yes? I think it's right; we should make sure we test it.
http://gerrit.cloudera.org:8080/#/c/8038/3/shell/impala_shell.py@1431
PS3, Line 1431: [(k.upper(),v) for k,v in parse_variables(options.query_options).items()])
nit: I believe pep8 style has a space after comma
http://gerrit.cloudera.org:8080/#/c/8038/3/shell/option_parser.py
File shell/option_parser.py:
http://gerrit.cloudera.org:8080/#/c/8038/3/shell/option_parser.py@33
PS3, Line 33: def validate_and_convert_options(loaded_options, default_options):
Let's be clear that these are impala_shell options and not impala_query options. I think we should rename the method as well as the argument names to make that more obvious.
http://gerrit.cloudera.org:8080/#/c/8038/3/shell/option_parser.py@39
PS3, Line 39: print >> sys.stderr, "WARNING: Unable to read configuration file correctly. " \
Perhaps: "Ignoring unrecognized config option '%s'"?
http://gerrit.cloudera.org:8080/#/c/8038/3/shell/option_parser.py@47
PS3, Line 47: loaded_options[i] = (option, True)
It's weird that we're modifying loaded_options here.
Are we intentionally passing the unrecognized options through?
I think it's poor form to both modify an argument in place and to return it. I would prefer you accumulating a "ret = []" kind of variable and returning that, to make this clearer.
Also, I tested it and it broke:
$impala-shell.sh
WARNING: Unable to read configuration file correctly. Check formatting: 'live_progress'
Starting Impala Shell without Kerberos authentication
Error connecting: TTransportException, Could not connect to philip-dev.gce.cloudera.com:21000
MT_DOP is not supported for the impalad being connected to, ignoring.
Traceback (most recent call last):
File "/home/philip/src/impala/shell/impala_shell.py", line 1448, in <module>
shell = ImpalaShell(options)
File "/home/philip/src/impala/shell/impala_shell.py", line 195, in __init__
self.do_connect(options.impalad)
File "/home/philip/src/impala/shell/impala_shell.py", line 686, in do_connect
for set_option in self.set_query_options:
RuntimeError: dictionary changed size during iteration
$cat ~/.impalarc
[impala]
live_progress=true
[impala.query_options]
EXPLAIN_LEVEL=2
MT_DOP=2
http://gerrit.cloudera.org:8080/#/c/8038/3/shell/option_parser.py@65
PS3, Line 65: Validates some values (False, True, None)
This only happens for the shell options part of it, yes?
http://gerrit.cloudera.org:8080/#/c/8038/3/shell/option_parser.py@73
PS3, Line 73: loaded_options = []
This is both shell options and impala query options, yeah? Should we make that clearer?
http://gerrit.cloudera.org:8080/#/c/8038/3/shell/option_parser.py@79
PS3, Line 79: loaded_options.append( ("set_query_options",
It took me longer than I care to admit to understand what's going on here. I get it now: Impala query options get written into options.set_query_options eventually. I think it's worthy of more explanation in the pydoc.
http://gerrit.cloudera.org:8080/#/c/8038/3/shell/option_parser.py@179
PS3, Line 179: " KEY starts with an alphabetic character and"
This is really about "key" being a valid query option, right? I think you can remove lines 179-180, and simply mention that you can see valid query options with "SET".
--
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: 3
Gerrit-Owner: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj...@apache.org>
Gerrit-Reviewer: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Comment-Date: Thu, 28 Sep 2017 18:46:39 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5736: Add impala-shell argument to set default query options
Posted by "anujphadke (Code Review)" <ge...@cloudera.org>.
anujphadke 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 6:
(1 comment)
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: There are two types of options: shell options and query_options. Both can be set be set
typo. "be set be set"
--
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: 6
Gerrit-Owner: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj...@apache.org>
Gerrit-Reviewer: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Reviewer: anujphadke <ap...@cloudera.com>
Gerrit-Comment-Date: Thu, 12 Oct 2017 23:29:08 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5736: Add impala-shell argument to set default query options
Posted by "Lars Volker (Code Review)" <ge...@cloudera.org>.
Lars Volker 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 14:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/8038/14//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/8038/14//COMMIT_MSG@30
PS14, Line 30:
This seems to lack a _
http://gerrit.cloudera.org:8080/#/c/8038/14/tests/shell/test_shell_commandline.py
File tests/shell/test_shell_commandline.py:
http://gerrit.cloudera.org:8080/#/c/8038/14/tests/shell/test_shell_commandline.py@389
PS14, Line 389: formatting
This does not report a formatting error, so I'm inclined to remove the "Check formatting:" part. What do you think?
--
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: 14
Gerrit-Owner: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj...@apache.org>
Gerrit-Reviewer: Michael Brown <mi...@cloudera.com>
Gerrit-Reviewer: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Reviewer: anujphadke <ap...@cloudera.com>
Gerrit-Comment-Date: Tue, 24 Oct 2017 17:35:19 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5736: Add impala-shell argument to set default query options
Posted by "Csaba Ringhofer (Code Review)" <ge...@cloudera.org>.
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 18:
> Csaba, it's hard to see, but the build failed the Apache RAT check.
> If you search 'FAILED JOB' at https://jenkins.impala.io/job/gerrit-verify-dryrun/1425/console
> you see a pointer to https://jenkins.impala.io/job/rat-check-ub1604/111/
> . In there is:
>
> 22:05:14 + bin/check-rat-report.py bin/rat_exclude_files.txt
> rat.xml
> 22:05:15 UNAPPROVED: tests/shell/impalarc_with_error
> 22:05:15 UNAPPROVED: tests/shell/impalarc_with_query_options
> 22:05:15 UNAPPROVED: tests/shell/impalarc_with_warnings
>
> I think these exclusions can be added to bin/rat_exclude_files.txt
> in this section, alongside the existing shell test data exclusions:
>
> # http://www.apache.org/legal/src-headers.html: "Test data for
> which the addition of a
> # source header would cause the tests to fail."
Thanks for the hint! I have added a test impalarc files to bin/rat_exclude_files.txt.
--
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: 18
Gerrit-Owner: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj...@apache.org>
Gerrit-Reviewer: Michael Brown <mi...@cloudera.com>
Gerrit-Reviewer: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Reviewer: anujphadke <ap...@cloudera.com>
Gerrit-Comment-Date: Thu, 02 Nov 2017 20:13:10 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-5736: Add impala-shell argument to set default query options
Posted by "Csaba Ringhofer (Code Review)" <ge...@cloudera.org>.
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 <cs...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj...@apache.org>
Gerrit-Reviewer: Michael Brown <mi...@cloudera.com>
Gerrit-Reviewer: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Reviewer: anujphadke <ap...@cloudera.com>
Gerrit-Comment-Date: Thu, 19 Oct 2017 15:07:00 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5736: Add impala-shell argument to set default query options
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/8038 )
Change subject: IMPALA-5736: Add impala-shell argument to set default query options
......................................................................
IMPALA-5736: Add impala-shell argument to set default query options
Query options can be set from command line and impala rc as
key=value pairs, where key is case insensitive.
Examples:
command line:
impala-shell.sh -Q MT_DOP=1 --query_option=MAX_ERRORS=200
.impalarc:
[impala.query_options]
EXPLAIN_LEVEL=2
MT_DOP=2
The options set in command line will update the ones
in impalarc one by one, so the result of the example
above will be:
EXPLAIN_LEVEL=2
MT_DOP=1
MAX_ERRORS=200
Additional changes:
- 0 and 1 are accepted as bools in section [impala] to
make it more consistent with [impala.query_options]
- options that are expected to be bool but are not
0/1/true/false lead to error instead of warning
Change-Id: I26a3b67230c80a99bd246b6af205d558fec9a986
Reviewed-on: http://gerrit.cloudera.org:8080/8038
Reviewed-by: Michael Brown <mi...@cloudera.com>
Tested-by: Impala Public Jenkins
---
M bin/rat_exclude_files.txt
M shell/impala_shell.py
M shell/option_parser.py
A tests/shell/impalarc_with_error
A tests/shell/impalarc_with_query_options
A tests/shell/impalarc_with_warnings
M tests/shell/test_shell_commandline.py
M tests/shell/test_shell_interactive.py
8 files changed, 159 insertions(+), 46 deletions(-)
Approvals:
Michael Brown: Looks good to me, approved
Impala Public Jenkins: Verified
--
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: merged
Gerrit-Change-Id: I26a3b67230c80a99bd246b6af205d558fec9a986
Gerrit-Change-Number: 8038
Gerrit-PatchSet: 19
Gerrit-Owner: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj...@apache.org>
Gerrit-Reviewer: Michael Brown <mi...@cloudera.com>
Gerrit-Reviewer: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Reviewer: anujphadke <ap...@cloudera.com>
[Impala-ASF-CR] IMPALA-5736: Add impala-shell argument to set default query options
Posted by "Philip Zeyliger (Code Review)" <ge...@cloudera.org>.
Philip Zeyliger 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: Code-Review+1
--
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 <cs...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj...@apache.org>
Gerrit-Reviewer: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Reviewer: anujphadke <ap...@cloudera.com>
Gerrit-Comment-Date: Fri, 13 Oct 2017 16:09:48 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-5736: Add impala-shell argument to set default query options
Posted by "Lars Volker (Code Review)" <ge...@cloudera.org>.
Lars Volker has posted comments on this change.
Change subject: IMPALA-5736: Add impala-shell argument to set default query options
......................................................................
Patch Set 1:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/8038/1/shell/option_parser.py
File shell/option_parser.py:
PS1, Line 160: parser.add_option("--var", dest="keyval", action="append",
: help="Define variable(s) to be used within the Impala session."
: " It must follow the pattern \"KEY=VALUE\","
: " KEY starts with an alphabetic character and"
: " contains alphanumeric characters or underscores.")
: parser.add_option("--query_options", dest="query_options",
: help="Sets default query options. The format is comma separated"
:
> We have talked about this with Lars, and decided to stick with the comma se
My thinking was that we already follow the k=v,k=v syntax for tools like start-impala-cluster.py. However I see how consistency with --var is a nice thing to have and I'm ok with picking that route.
--
To view, visit http://gerrit.cloudera.org:8080/8038
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I26a3b67230c80a99bd246b6af205d558fec9a986
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Philip Zeyliger <ph...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5736: Add impala-shell argument to set default query options
Posted by "Csaba Ringhofer (Code Review)" <ge...@cloudera.org>.
Hello Lars Volker, Michael Brown, Matthew Jacobs, Philip Zeyliger, anujphadke,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/8038
to look at the new patch set (#15).
Change subject: IMPALA-5736: Add impala-shell argument to set default query options
......................................................................
IMPALA-5736: Add impala-shell argument to set default query options
Query options can be set from command line and impala rc as
key=value pairs, where key is case insensitive.
Examples:
command line:
impala-shell.sh -Q MT_DOP=1 --query_option=MAX_ERRORS=200
.impalarc:
[impala.query_options]
EXPLAIN_LEVEL=2
MT_DOP=2
The options set in command line will update the ones
in impalarc one by one, so the result of the example
above will be:
EXPLAIN_LEVEL=2
MT_DOP=1
MAX_ERRORS=200
Additional changes:
- 0 and 1 are accepted as bools in section [impala] to
make it more consistent with [impala.query_options]
- options that are expected to be bool but are not
0/1/true/false lead to error instead of warning
Change-Id: I26a3b67230c80a99bd246b6af205d558fec9a986
---
M shell/impala_shell.py
M shell/option_parser.py
A tests/shell/impalarc_with_error
A tests/shell/impalarc_with_query_options
A tests/shell/impalarc_with_warnings
M tests/shell/test_shell_commandline.py
M tests/shell/test_shell_interactive.py
7 files changed, 166 insertions(+), 46 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/38/8038/15
--
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: newpatchset
Gerrit-Change-Id: I26a3b67230c80a99bd246b6af205d558fec9a986
Gerrit-Change-Number: 8038
Gerrit-PatchSet: 15
Gerrit-Owner: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj...@apache.org>
Gerrit-Reviewer: Michael Brown <mi...@cloudera.com>
Gerrit-Reviewer: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Reviewer: anujphadke <ap...@cloudera.com>
[Impala-ASF-CR] IMPALA-5736: Add impala-shell argument to set default query options
Posted by "Michael Brown (Code Review)" <ge...@cloudera.org>.
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 <cs...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj...@apache.org>
Gerrit-Reviewer: Michael Brown <mi...@cloudera.com>
Gerrit-Reviewer: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Reviewer: anujphadke <ap...@cloudera.com>
Gerrit-Comment-Date: Wed, 18 Oct 2017 20:34:20 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5736: Add impala-shell argument to set default query options
Posted by "Lars Volker (Code Review)" <ge...@cloudera.org>.
Lars Volker 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 15: Code-Review+1
Thank you for the continued effort. I like the exceptions. Let's give Mike a bit of time to look at the latest PS and then I think this is ready to be submitted.
--
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: 15
Gerrit-Owner: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj...@apache.org>
Gerrit-Reviewer: Michael Brown <mi...@cloudera.com>
Gerrit-Reviewer: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Reviewer: anujphadke <ap...@cloudera.com>
Gerrit-Comment-Date: Wed, 25 Oct 2017 17:14:13 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-5736: Add impala-shell argument to set default query options
Posted by "Csaba Ringhofer (Code Review)" <ge...@cloudera.org>.
Hello Lars Volker, Michael Brown, Matthew Jacobs, Philip Zeyliger, anujphadke,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/8038
to look at the new patch set (#12).
Change subject: IMPALA-5736: Add impala-shell argument to set default query options
......................................................................
IMPALA-5736: Add impala-shell argument to set default query options
Query options can be set from command line and impala rc as
key=value pairs, where key is case insensitive.
Examples:
command line:
impala-shell.sh -Q MT_DOP=1 --query_option=MAX_ERRORS=200
.impalarc:
[impala.query_options]
EXPLAIN_LEVEL=2
MT_DOP=2
The options set in command line will update the ones
in impalarc one by one, so the result of the example
above will be:
EXPLAIN_LEVEL=2
MT_DOP=1
MAX_ERRORS=200
Additional changes:
- 0 and 1 are accepted as bools in section [impala] to
make it more consistent with [impala.query options]
- options that are expected to be bool but are not
0/1/true/false lead to error instead of warning
Change-Id: I26a3b67230c80a99bd246b6af205d558fec9a986
---
M shell/impala_shell.py
M shell/option_parser.py
A tests/shell/impalarc_with_error
A tests/shell/impalarc_with_query_options
A tests/shell/impalarc_with_warnings
M tests/shell/test_shell_commandline.py
M tests/shell/test_shell_interactive.py
7 files changed, 134 insertions(+), 40 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/38/8038/12
--
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: newpatchset
Gerrit-Change-Id: I26a3b67230c80a99bd246b6af205d558fec9a986
Gerrit-Change-Number: 8038
Gerrit-PatchSet: 12
Gerrit-Owner: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj...@apache.org>
Gerrit-Reviewer: Michael Brown <mi...@cloudera.com>
Gerrit-Reviewer: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Reviewer: anujphadke <ap...@cloudera.com>
[Impala-ASF-CR] IMPALA-5736: Add impala-shell argument to set default query options
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins 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 17:
Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/1425/
--
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: 17
Gerrit-Owner: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj...@apache.org>
Gerrit-Reviewer: Michael Brown <mi...@cloudera.com>
Gerrit-Reviewer: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Reviewer: anujphadke <ap...@cloudera.com>
Gerrit-Comment-Date: Wed, 01 Nov 2017 21:32:18 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-5736: Add impala-shell argument to set default query options
Posted by "Csaba Ringhofer (Code Review)" <ge...@cloudera.org>.
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 3:
(10 comments)
http://gerrit.cloudera.org:8080/#/c/8038/3//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/8038/3//COMMIT_MSG@25
PS3, Line 25: EXPLAIN_LEVEL=2 and MT_DOP=1.:
> Maybe just make this the example in line 14 and 19?
Done
http://gerrit.cloudera.org:8080/#/c/8038/3/shell/impala_shell.py
File shell/impala_shell.py:
http://gerrit.cloudera.org:8080/#/c/8038/3/shell/impala_shell.py@1430
PS3, Line 1430: options.set_query_options.update(
> Just to confirm: this is command line overriding what's in the config file,
Done
http://gerrit.cloudera.org:8080/#/c/8038/3/shell/impala_shell.py@1431
PS3, Line 1431: [(k.upper(),v) for k,v in parse_variables(options.query_options).items()])
> nit: I believe pep8 style has a space after comma
Done
http://gerrit.cloudera.org:8080/#/c/8038/3/shell/option_parser.py
File shell/option_parser.py:
http://gerrit.cloudera.org:8080/#/c/8038/3/shell/option_parser.py@33
PS3, Line 33: def validate_and_convert_options(loaded_options, default_options):
> Let's be clear that these are impala_shell options and not impala_query opt
I did a bit of refactoring, I think that it is more clear now. Please comment if I should work on it more.
http://gerrit.cloudera.org:8080/#/c/8038/3/shell/option_parser.py@39
PS3, Line 39: print >> sys.stderr, "WARNING: Unable to read configuration file correctly. " \
> Perhaps: "Ignoring unrecognized config option '%s'"?
Done
http://gerrit.cloudera.org:8080/#/c/8038/3/shell/option_parser.py@47
PS3, Line 47: loaded_options[i] = (option, True)
> It's weird that we're modifying loaded_options here.
The cause of the runtime error is fixed now, see impala_shell.py line 686-693.
Note that query options only work when the shell is connected to impalad and this was the reason why MT_DOP was not accepted here.
http://gerrit.cloudera.org:8080/#/c/8038/3/shell/option_parser.py@65
PS3, Line 65: Validates some values (False, True, None)
> This only happens for the shell options part of it, yes?
Yes.
http://gerrit.cloudera.org:8080/#/c/8038/3/shell/option_parser.py@73
PS3, Line 73: loaded_options = []
> This is both shell options and impala query options, yeah? Should we make t
Done
http://gerrit.cloudera.org:8080/#/c/8038/3/shell/option_parser.py@79
PS3, Line 79: loaded_options.append( ("set_query_options",
> It took me longer than I care to admit to understand what's going on here.
I have changed some of this, I hope that it is less messy now. Some of the option handling code in impala_shell.py is still a bit "strange" in my opinion, but I am not sure that rewriting it should be in the scope of this commit.
http://gerrit.cloudera.org:8080/#/c/8038/3/shell/option_parser.py@179
PS3, Line 179: " KEY starts with an alphabetic character and"
> This is really about "key" being a valid query option, right? I think you c
Done
--
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: 3
Gerrit-Owner: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj...@apache.org>
Gerrit-Reviewer: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Comment-Date: Tue, 10 Oct 2017 20:08:35 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5736: Add impala-shell argument to set default query options
Posted by "Lars Volker (Code Review)" <ge...@cloudera.org>.
Lars Volker has posted comments on this change.
Change subject: IMPALA-5736: Add impala-shell argument to set default query options
......................................................................
Patch Set 1:
MJ, do you prefer one option with a comma separated list of key=value pairs, or using several options similar to --var ?
--
To view, visit http://gerrit.cloudera.org:8080/8038
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I26a3b67230c80a99bd246b6af205d558fec9a986
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj...@apache.org>
Gerrit-Reviewer: Philip Zeyliger <ph...@cloudera.com>
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-5736: Add impala-shell argument to set default query options
Posted by "Philip Zeyliger (Code Review)" <ge...@cloudera.org>.
Philip Zeyliger 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 10:
(1 comment)
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@91
PS7, Line 91: conve
> Unfortunately Python 2.7 is not ubiquitous in Linux distributions like Cent
To be specific: CentOS/Redhat 6 uses py2.6. CentOS/Redhat 5: py2.4.
--
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: 10
Gerrit-Owner: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj...@apache.org>
Gerrit-Reviewer: Michael Brown <mi...@cloudera.com>
Gerrit-Reviewer: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Reviewer: anujphadke <ap...@cloudera.com>
Gerrit-Comment-Date: Thu, 19 Oct 2017 16:47:19 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5736: Add impala-shell argument to set default query options
Posted by "Michael Brown (Code Review)" <ge...@cloudera.org>.
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 10: Code-Review+1
--
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: 10
Gerrit-Owner: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj...@apache.org>
Gerrit-Reviewer: Michael Brown <mi...@cloudera.com>
Gerrit-Reviewer: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Reviewer: anujphadke <ap...@cloudera.com>
Gerrit-Comment-Date: Thu, 19 Oct 2017 17:25:03 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-5736: Add impala-shell argument to set default query options
Posted by "Csaba Ringhofer (Code Review)" <ge...@cloudera.org>.
Hello Lars Volker, Matthew Jacobs, Philip Zeyliger,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/8038
to look at the new patch set (#2).
Change subject: IMPALA-5736: Add impala-shell argument to set default query options
......................................................................
IMPALA-5736: Add impala-shell argument to set default query options
Query options can be set from command line and impala rc as
key=value pairs.
Examples:
command line:
impala-shell.sh --query_option=EXPLAIN_LEVEL=2 -Q MT_DOP=2
.impalarc:
[impala.query_options]
EXPLAIN_LEVEL=2
MT_DOP=2
Note that the options set in command line will update the ones
in impalarc one by one. For example, if impalarc contains
"EXPLAIN_LEVEL=2 and MT_DOP=2", and the shell command contains
--query_option="MT_DOP=1", the result will be
EXPLAIN_LEVEL=2 and MT_DOP=1.:
Change-Id: I26a3b67230c80a99bd246b6af205d558fec9a986
---
M shell/impala_shell.py
M shell/impala_shell_config_defaults.py
M shell/option_parser.py
3 files changed, 13 insertions(+), 1 deletion(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/38/8038/2
--
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: newpatchset
Gerrit-Change-Id: I26a3b67230c80a99bd246b6af205d558fec9a986
Gerrit-Change-Number: 8038
Gerrit-PatchSet: 2
Gerrit-Owner: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj...@apache.org>
Gerrit-Reviewer: Philip Zeyliger <ph...@cloudera.com>
[Impala-ASF-CR] IMPALA-5736: Add impala-shell argument to set default query options
Posted by "Philip Zeyliger (Code Review)" <ge...@cloudera.org>.
Philip Zeyliger 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 4:
(10 comments)
Thanks! I think this is much clearer than the previous iteration. All of my comments are either nits or requests to add a few more comments.
http://gerrit.cloudera.org:8080/#/c/8038/4/shell/impala_shell.py
File shell/impala_shell.py:
http://gerrit.cloudera.org:8080/#/c/8038/4/shell/impala_shell.py@686
PS4, Line 686: tmp_set = {}
Add a comment:
# Use a temporary to avoid changing set_query_options during iteration.
(An alternative is to use del, but use .items() instead of .iteritems())
http://gerrit.cloudera.org:8080/#/c/8038/4/shell/impala_shell.py@1330
PS4, Line 1330: if __name__ == "__main__":
Perhaps we need a big comment:
"""
There are two types of options: shell options and query_options. You can set these in the shell itself, which overrides settings on the command line, which override the defaults in impala_shell_config_defaults.py. Query options have no defaults within the impala-shell, but they do have defaults on the server.
"""
http://gerrit.cloudera.org:8080/#/c/8038/4/shell/impala_shell.py@1350
PS4, Line 1350: # default options loaded in from impala_shell_config_defaults.py
Let's take the time to update this comment by disambiguating "shell options" vs "query options" here?
http://gerrit.cloudera.org:8080/#/c/8038/4/shell/impala_shell.py@1354
PS4, Line 1354: impala_shell_defaults.update(loaded_shell_options)
I think "impala_query_options_default" is empty, but this assymetry between impala_shell_defaults and query_options is weird. It's weird that you're updating impala_shell_defaults, rather than updating "shell_options".
http://gerrit.cloudera.org:8080/#/c/8038/4/shell/impala_shell.py@1437
PS4, Line 1437: query_options.update(
Add comment:
Override query_options with those specified on the command line.
http://gerrit.cloudera.org:8080/#/c/8038/4/shell/option_parser.py
File shell/option_parser.py:
http://gerrit.cloudera.org:8080/#/c/8038/4/shell/option_parser.py@36
PS4, Line 36: def convert_loaded_shell_option(option, value, default_options, config_filename):
Please document config_filename. I'm unclear how it's used.
http://gerrit.cloudera.org:8080/#/c/8038/4/shell/option_parser.py@47
PS4, Line 47: # if the option is not set to either true or false, use the default
Do we need to log about the ignored value here?
http://gerrit.cloudera.org:8080/#/c/8038/4/shell/option_parser.py@36
PS4, Line 36: def convert_loaded_shell_option(option, value, default_options, config_filename):
: """Converts some values based on their type in default_options
: """
: if default_options[option] in [True, False]:
: # validate the option if it can only be a boolean value
: # the only choice for these options is true or false
: if value.lower() == "true":
: return True
: elif value.lower() == 'false':
: return False
: else:
: # if the option is not set to either true or false, use the default
: return default_options[option]
: elif value.lower() == "none":
: return None
: elif option.lower() == "config_file":
: return config_filename
This function is mixing 2-space indent and 4-space indent. Please clean up.
http://gerrit.cloudera.org:8080/#/c/8038/4/shell/option_parser.py@164
PS4, Line 164: "Only specify this as a option in the commandline."
s/as a option/as an option/
http://gerrit.cloudera.org:8080/#/c/8038/4/shell/option_parser.py@183
PS4, Line 183: help="Sets default query options."
Add: "May be specified multiple times." Unless it's clear from the usage?
--
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: 4
Gerrit-Owner: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj...@apache.org>
Gerrit-Reviewer: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Comment-Date: Wed, 11 Oct 2017 18:42:39 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5736: Add impala-shell argument to set default query options
Posted by "Csaba Ringhofer (Code Review)" <ge...@cloudera.org>.
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 17:
> You've received several +1s along the way. I chatted with Lars when
> he asked me to look at this again and it's good to go for +2.
> However, as you might have seen on the dev mailing list [0] we are
> holding off submissions until builds stabilize. In the mean time, I
> noticed your patch contains conflicts. Want to rebase and fix the
> conflicts?
>
> [0] http://mail-archives.apache.org/mod_mbox/incubator-impala-dev/201710.mbox/%3CCAM_aD9cic7XjCs8Rs_3QtcmnK-PmcrADavqNq5JzTU8f0%3DtXXA%40mail.gmail.com%3E
I have done the rebase in patch set 17.
--
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: 17
Gerrit-Owner: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj...@apache.org>
Gerrit-Reviewer: Michael Brown <mi...@cloudera.com>
Gerrit-Reviewer: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Reviewer: anujphadke <ap...@cloudera.com>
Gerrit-Comment-Date: Mon, 30 Oct 2017 22:31:48 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-5736: Add impala-shell argument to set default query options
Posted by "Csaba Ringhofer (Code Review)" <ge...@cloudera.org>.
Hello Lars Volker, Michael Brown, Matthew Jacobs, Philip Zeyliger, anujphadke,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/8038
to look at the new patch set (#14).
Change subject: IMPALA-5736: Add impala-shell argument to set default query options
......................................................................
IMPALA-5736: Add impala-shell argument to set default query options
Query options can be set from command line and impala rc as
key=value pairs, where key is case insensitive.
Examples:
command line:
impala-shell.sh -Q MT_DOP=1 --query_option=MAX_ERRORS=200
.impalarc:
[impala.query_options]
EXPLAIN_LEVEL=2
MT_DOP=2
The options set in command line will update the ones
in impalarc one by one, so the result of the example
above will be:
EXPLAIN_LEVEL=2
MT_DOP=1
MAX_ERRORS=200
Additional changes:
- 0 and 1 are accepted as bools in section [impala] to
make it more consistent with [impala.query options]
- options that are expected to be bool but are not
0/1/true/false lead to error instead of warning
Change-Id: I26a3b67230c80a99bd246b6af205d558fec9a986
---
M shell/impala_shell.py
M shell/option_parser.py
A tests/shell/impalarc_with_error
A tests/shell/impalarc_with_query_options
A tests/shell/impalarc_with_warnings
M tests/shell/test_shell_commandline.py
M tests/shell/test_shell_interactive.py
7 files changed, 140 insertions(+), 43 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/38/8038/14
--
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: newpatchset
Gerrit-Change-Id: I26a3b67230c80a99bd246b6af205d558fec9a986
Gerrit-Change-Number: 8038
Gerrit-PatchSet: 14
Gerrit-Owner: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj...@apache.org>
Gerrit-Reviewer: Michael Brown <mi...@cloudera.com>
Gerrit-Reviewer: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Reviewer: anujphadke <ap...@cloudera.com>
[Impala-ASF-CR] IMPALA-5736: Add impala-shell argument to set default query options
Posted by "Csaba Ringhofer (Code Review)" <ge...@cloudera.org>.
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 14:
(3 comments)
Sorry for the many patches, 12, 13, 14 should be seen as one.
There are also some functional changes (see commit message), and automatic tests were added for some impalarc related warnings/errors.
http://gerrit.cloudera.org:8080/#/c/8038/11/shell/option_parser.py
File shell/option_parser.py:
http://gerrit.cloudera.org:8080/#/c/8038/11/shell/option_parser.py@36
PS11, Line 36: parse_bool_option(value):
> I still struggle to understand the function name. How about parse_shell_opt
I have rewritten these functions again, I hope that the naming is more intuitive in the new version.
http://gerrit.cloudera.org:8080/#/c/8038/11/shell/option_parser.py@37
PS11, Line 37: """Returns True for '1' and 'True', and False for '0' and 'False'.
> Mention that this parses string values into corresponding python types. The
Done
http://gerrit.cloudera.org:8080/#/c/8038/11/shell/option_parser.py@64
PS11, Line 64:
> types
Done
--
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: 14
Gerrit-Owner: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj...@apache.org>
Gerrit-Reviewer: Michael Brown <mi...@cloudera.com>
Gerrit-Reviewer: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Reviewer: anujphadke <ap...@cloudera.com>
Gerrit-Comment-Date: Tue, 24 Oct 2017 15:43:13 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5736: Add impala-shell argument to set default query options
Posted by "Csaba Ringhofer (Code Review)" <ge...@cloudera.org>.
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 7:
(1 comment)
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@36
PS7, Line 36: def convert_loaded_shell_option(option, value, default_options, config_filename):
: """Converts some values based on their type in default_options
:
: Setting 'config_filename' in the config file would have no effect,
: so its original value is kept.
> From looking at the signature and the comment I have no idea what this meth
I have changed the signature in patch set 9. Is it clear enough this way?
--
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 <cs...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj...@apache.org>
Gerrit-Reviewer: Michael Brown <mi...@cloudera.com>
Gerrit-Reviewer: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Reviewer: anujphadke <ap...@cloudera.com>
Gerrit-Comment-Date: Thu, 19 Oct 2017 15:57:26 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5736: Add impala-shell argument to set default query options
Posted by "Csaba Ringhofer (Code Review)" <ge...@cloudera.org>.
Hello Lars Volker, Michael Brown, Matthew Jacobs, Philip Zeyliger, anujphadke,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/8038
to look at the new patch set (#9).
Change subject: IMPALA-5736: Add impala-shell argument to set default query options
......................................................................
IMPALA-5736: Add impala-shell argument to set default query options
Query options can be set from command line and impala rc as
key=value pairs, where key is case insensitive.
Examples:
command line:
impala-shell.sh -Q MT_DOP=1 --query_option=MAX_ERRORS=200
.impalarc:
[impala.query_options]
EXPLAIN_LEVEL=2
MT_DOP=2
The options set in command line will update the ones
in impalarc one by one, so the result of the example
above will be:
EXPLAIN_LEVEL=2
MT_DOP=1
MAX_ERRORS=200
Change-Id: I26a3b67230c80a99bd246b6af205d558fec9a986
---
M shell/impala_shell.py
M shell/option_parser.py
A tests/shell/impalarc_with_query_options
M tests/shell/test_shell_interactive.py
4 files changed, 112 insertions(+), 37 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/38/8038/9
--
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: newpatchset
Gerrit-Change-Id: I26a3b67230c80a99bd246b6af205d558fec9a986
Gerrit-Change-Number: 8038
Gerrit-PatchSet: 9
Gerrit-Owner: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj...@apache.org>
Gerrit-Reviewer: Michael Brown <mi...@cloudera.com>
Gerrit-Reviewer: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Reviewer: anujphadke <ap...@cloudera.com>
[Impala-ASF-CR] IMPALA-5736: Add impala-shell argument to set default query options
Posted by "Csaba Ringhofer (Code Review)" <ge...@cloudera.org>.
Hello Lars Volker, Matthew Jacobs, Philip Zeyliger,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/8038
to look at the new patch set (#4).
Change subject: IMPALA-5736: Add impala-shell argument to set default query options
......................................................................
IMPALA-5736: Add impala-shell argument to set default query options
Query options can be set from command line and impala rc as
key=value pairs.
Examples:
command line:
impala-shell.sh -Q MT_DOP=1 --query_option=MAX_ERRORS=200
.impalarc:
[impala.query_options]
EXPLAIN_LEVEL=2
MT_DOP=2
The options set in command line will update the ones
in impalarc one by one, so the result of the example
above will be:
EXPLAIN_LEVEL=2
MT_DOP=1
MAX_ERRORS=200
Change-Id: I26a3b67230c80a99bd246b6af205d558fec9a986
---
M shell/impala_shell.py
M shell/option_parser.py
A tests/shell/impalarc_with_query_options
M tests/shell/test_shell_interactive.py
4 files changed, 83 insertions(+), 37 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/38/8038/4
--
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: newpatchset
Gerrit-Change-Id: I26a3b67230c80a99bd246b6af205d558fec9a986
Gerrit-Change-Number: 8038
Gerrit-PatchSet: 4
Gerrit-Owner: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj...@apache.org>
Gerrit-Reviewer: Philip Zeyliger <ph...@cloudera.com>
[Impala-ASF-CR] IMPALA-5736: Add impala-shell argument to set default query options
Posted by "Csaba Ringhofer (Code Review)" <ge...@cloudera.org>.
Hello Lars Volker, Michael Brown, Matthew Jacobs, Philip Zeyliger, anujphadke,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/8038
to look at the new patch set (#13).
Change subject: IMPALA-5736: Add impala-shell argument to set default query options
......................................................................
IMPALA-5736: Add impala-shell argument to set default query options
Query options can be set from command line and impala rc as
key=value pairs, where key is case insensitive.
Examples:
command line:
impala-shell.sh -Q MT_DOP=1 --query_option=MAX_ERRORS=200
.impalarc:
[impala.query_options]
EXPLAIN_LEVEL=2
MT_DOP=2
The options set in command line will update the ones
in impalarc one by one, so the result of the example
above will be:
EXPLAIN_LEVEL=2
MT_DOP=1
MAX_ERRORS=200
Additional changes:
- 0 and 1 are accepted as bools in section [impala] to
make it more consistent with [impala.query options]
- options that are expected to be bool but are not
0/1/true/false lead to error instead of warning
Change-Id: I26a3b67230c80a99bd246b6af205d558fec9a986
---
M shell/impala_shell.py
M shell/option_parser.py
A tests/shell/impalarc_with_error
A tests/shell/impalarc_with_query_options
A tests/shell/impalarc_with_warnings
M tests/shell/test_shell_commandline.py
M tests/shell/test_shell_interactive.py
7 files changed, 139 insertions(+), 43 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/38/8038/13
--
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: newpatchset
Gerrit-Change-Id: I26a3b67230c80a99bd246b6af205d558fec9a986
Gerrit-Change-Number: 8038
Gerrit-PatchSet: 13
Gerrit-Owner: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj...@apache.org>
Gerrit-Reviewer: Michael Brown <mi...@cloudera.com>
Gerrit-Reviewer: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Reviewer: anujphadke <ap...@cloudera.com>
[Impala-ASF-CR] IMPALA-5736: Add impala-shell argument to set default query options
Posted by "Csaba Ringhofer (Code Review)" <ge...@cloudera.org>.
Hello Lars Volker, Michael Brown, Matthew Jacobs, Philip Zeyliger, anujphadke,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/8038
to look at the new patch set (#10).
Change subject: IMPALA-5736: Add impala-shell argument to set default query options
......................................................................
IMPALA-5736: Add impala-shell argument to set default query options
Query options can be set from command line and impala rc as
key=value pairs, where key is case insensitive.
Examples:
command line:
impala-shell.sh -Q MT_DOP=1 --query_option=MAX_ERRORS=200
.impalarc:
[impala.query_options]
EXPLAIN_LEVEL=2
MT_DOP=2
The options set in command line will update the ones
in impalarc one by one, so the result of the example
above will be:
EXPLAIN_LEVEL=2
MT_DOP=1
MAX_ERRORS=200
Change-Id: I26a3b67230c80a99bd246b6af205d558fec9a986
---
M shell/impala_shell.py
M shell/option_parser.py
A tests/shell/impalarc_with_query_options
M tests/shell/test_shell_interactive.py
4 files changed, 112 insertions(+), 37 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/38/8038/10
--
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: newpatchset
Gerrit-Change-Id: I26a3b67230c80a99bd246b6af205d558fec9a986
Gerrit-Change-Number: 8038
Gerrit-PatchSet: 10
Gerrit-Owner: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj...@apache.org>
Gerrit-Reviewer: Michael Brown <mi...@cloudera.com>
Gerrit-Reviewer: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Reviewer: anujphadke <ap...@cloudera.com>
[Impala-ASF-CR] IMPALA-5736: Add impala-shell argument to set default query options
Posted by "Csaba Ringhofer (Code Review)" <ge...@cloudera.org>.
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 7:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/8038/9/shell/impala_shell.py
File shell/impala_shell.py:
http://gerrit.cloudera.org:8080/#/c/8038/9/shell/impala_shell.py@688
PS9, Line 688:
> Nit: trailing white space. You can use 'git diff --check' to catch these.
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@58
PS7, Line 58: return config_filename
> As discussed in person, can you please add a test for this case?
It turned out, that there is already a test for this branch (test_shell_commandline.py / test_config_file), but I did not run it...
Running test_shell_commandline.py revealed that there was another issue caused by this change: breaking execute_queries_non_interactive_mode(), which handles the case when -q option is set). This issue was fixed in the new patch.
--
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 <cs...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj...@apache.org>
Gerrit-Reviewer: Michael Brown <mi...@cloudera.com>
Gerrit-Reviewer: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Reviewer: anujphadke <ap...@cloudera.com>
Gerrit-Comment-Date: Fri, 20 Oct 2017 19:28:28 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5736: Add impala-shell argument to set default query options
Posted by "Csaba Ringhofer (Code Review)" <ge...@cloudera.org>.
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 16:
(6 comments)
> (6 comments)
>
> I'm lukewarm on the exception hierarchy introduced. I appreciate
> the desire for clearer errors for the user. However, in the case of
> exception hierarchies, PEP-008 [0] has this to say:
>
> > Design exception hierarchies based on the distinctions that code
> catching the exceptions is likely to need, rather than the
> locations where the exceptions are raised.
>
> In the case of the exception hierarchy introduced in patch set 15,
> there isn't any catching introduced: both exceptions are raised
> directly to the shell user.
>
> Is it that important, then, to introduce this exception hierarchy
> at all?
>
> Again however, I appreciate the desire to have clearer errors. At a
> high level, there are three options I see:
>
> 1. Completely remove the exception hierarchy introduced.
> 2. Leave exception hierarchy in place, but trim declarations to be
> more conventional.
> 3. Leave the exception hierarchy in place, and leave its
> declarations in this more "verbose", unconventional state.
>
> I'm going to leave review comments guiding toward #2. Happy to hear
> arguments for 1 or 3.
>
> [0] https://www.python.org/dev/peps/pep-0008/
I am unsure about 1 vs 2. Probably no one will ever differentiate between these exceptions, so they could be simply thrown as Exception. I have made the patch according to your comments towards 2.
http://gerrit.cloudera.org:8080/#/c/8038/15/shell/option_parser.py
File shell/option_parser.py:
http://gerrit.cloudera.org:8080/#/c/8038/15/shell/option_parser.py@36
PS15, Line 36: class ConfigFileFormatError(Exception):
: """Raised when the config file cannot be read by ConfigParser."""
: pass
:
> I think this should be removed. The only reason to have a hierarchy like th
Done
http://gerrit.cloudera.org:8080/#/c/8038/15/shell/option_parser.py@41
PS15, Line 41: aised when an o
> ConfigFileFormatError might be a clearer name.
Done
http://gerrit.cloudera.org:8080/#/c/8038/15/shell/option_parser.py@43
PS15, Line 43:
: def parse_bool_opt
> I don't you think you need to override __init__, because the Exception hier
Done
http://gerrit.cloudera.org:8080/#/c/8038/15/shell/option_parser.py@46
PS15, Line 46: Throws ValueError for other values.
: """
> Instead of overriding __str__, it is more conventional to let the message a
Done
http://gerrit.cloudera.org:8080/#/c/8038/15/shell/option_parser.py@49
PS15, Line 49: turn True
> InvalidOptionValueError?
Done
http://gerrit.cloudera.org:8080/#/c/8038/15/shell/option_parser.py@51
PS15, Line 51: return False
: else:
: raise InvalidOptionValueError("Unexpected value in configuration file. '" + value \
: + "' is not a valid value for a boolean option.")
:
> Same comments with __init__ and __str__ apply here.
Done
--
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: 16
Gerrit-Owner: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj...@apache.org>
Gerrit-Reviewer: Michael Brown <mi...@cloudera.com>
Gerrit-Reviewer: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Reviewer: anujphadke <ap...@cloudera.com>
Gerrit-Comment-Date: Mon, 30 Oct 2017 21:51:14 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5736: Add impala-shell argument to set default query options
Posted by "Csaba Ringhofer (Code Review)" <ge...@cloudera.org>.
Hello Lars Volker, Michael Brown, Matthew Jacobs, Philip Zeyliger, anujphadke,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/8038
to look at the new patch set (#11).
Change subject: IMPALA-5736: Add impala-shell argument to set default query options
......................................................................
IMPALA-5736: Add impala-shell argument to set default query options
Query options can be set from command line and impala rc as
key=value pairs, where key is case insensitive.
Examples:
command line:
impala-shell.sh -Q MT_DOP=1 --query_option=MAX_ERRORS=200
.impalarc:
[impala.query_options]
EXPLAIN_LEVEL=2
MT_DOP=2
The options set in command line will update the ones
in impalarc one by one, so the result of the example
above will be:
EXPLAIN_LEVEL=2
MT_DOP=1
MAX_ERRORS=200
Change-Id: I26a3b67230c80a99bd246b6af205d558fec9a986
---
M shell/impala_shell.py
M shell/option_parser.py
A tests/shell/impalarc_with_query_options
M tests/shell/test_shell_interactive.py
4 files changed, 115 insertions(+), 40 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/38/8038/11
--
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: newpatchset
Gerrit-Change-Id: I26a3b67230c80a99bd246b6af205d558fec9a986
Gerrit-Change-Number: 8038
Gerrit-PatchSet: 11
Gerrit-Owner: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj...@apache.org>
Gerrit-Reviewer: Michael Brown <mi...@cloudera.com>
Gerrit-Reviewer: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Reviewer: anujphadke <ap...@cloudera.com>
[Impala-ASF-CR] IMPALA-5736: Add impala-shell argument to set default query options
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins 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 18: Verified+1
--
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: 18
Gerrit-Owner: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj...@apache.org>
Gerrit-Reviewer: Michael Brown <mi...@cloudera.com>
Gerrit-Reviewer: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Reviewer: anujphadke <ap...@cloudera.com>
Gerrit-Comment-Date: Fri, 03 Nov 2017 00:11:29 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-5736: Add impala-shell argument to set default query options
Posted by "Michael Brown (Code Review)" <ge...@cloudera.org>.
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 18: Code-Review+2
--
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: 18
Gerrit-Owner: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj...@apache.org>
Gerrit-Reviewer: Michael Brown <mi...@cloudera.com>
Gerrit-Reviewer: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Reviewer: anujphadke <ap...@cloudera.com>
Gerrit-Comment-Date: Thu, 02 Nov 2017 20:19:55 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-5736: Add impala-shell argument to set default query options
Posted by "Csaba Ringhofer (Code Review)" <ge...@cloudera.org>.
Hello Lars Volker, Michael Brown, Matthew Jacobs, Philip Zeyliger, anujphadke,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/8038
to look at the new patch set (#17).
Change subject: IMPALA-5736: Add impala-shell argument to set default query options
......................................................................
IMPALA-5736: Add impala-shell argument to set default query options
Query options can be set from command line and impala rc as
key=value pairs, where key is case insensitive.
Examples:
command line:
impala-shell.sh -Q MT_DOP=1 --query_option=MAX_ERRORS=200
.impalarc:
[impala.query_options]
EXPLAIN_LEVEL=2
MT_DOP=2
The options set in command line will update the ones
in impalarc one by one, so the result of the example
above will be:
EXPLAIN_LEVEL=2
MT_DOP=1
MAX_ERRORS=200
Additional changes:
- 0 and 1 are accepted as bools in section [impala] to
make it more consistent with [impala.query_options]
- options that are expected to be bool but are not
0/1/true/false lead to error instead of warning
Change-Id: I26a3b67230c80a99bd246b6af205d558fec9a986
---
M shell/impala_shell.py
M shell/option_parser.py
A tests/shell/impalarc_with_error
A tests/shell/impalarc_with_query_options
A tests/shell/impalarc_with_warnings
M tests/shell/test_shell_commandline.py
M tests/shell/test_shell_interactive.py
7 files changed, 156 insertions(+), 46 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/38/8038/17
--
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: newpatchset
Gerrit-Change-Id: I26a3b67230c80a99bd246b6af205d558fec9a986
Gerrit-Change-Number: 8038
Gerrit-PatchSet: 17
Gerrit-Owner: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj...@apache.org>
Gerrit-Reviewer: Michael Brown <mi...@cloudera.com>
Gerrit-Reviewer: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Reviewer: anujphadke <ap...@cloudera.com>
[Impala-ASF-CR] IMPALA-5736: Add impala-shell argument to set default query options
Posted by "Matthew Jacobs (Code Review)" <ge...@cloudera.org>.
Matthew Jacobs 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 1:
> MJ, do you prefer one option with a comma separated list of
> key=value pairs, or using several options similar to --var ?
I don't have a preference. When I commented on the JIRA I hadn't considered the alternative proposed by Phil. I think that sounds reasonable.
--
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: 1
Gerrit-Owner: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj...@apache.org>
Gerrit-Reviewer: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Comment-Date: Thu, 21 Sep 2017 17:45:38 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-5736: Add impala-shell argument to set default query options
Posted by "Csaba Ringhofer (Code Review)" <ge...@cloudera.org>.
Hello Lars Volker, Michael Brown, Matthew Jacobs, Philip Zeyliger, anujphadke,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/8038
to look at the new patch set (#16).
Change subject: IMPALA-5736: Add impala-shell argument to set default query options
......................................................................
IMPALA-5736: Add impala-shell argument to set default query options
Query options can be set from command line and impala rc as
key=value pairs, where key is case insensitive.
Examples:
command line:
impala-shell.sh -Q MT_DOP=1 --query_option=MAX_ERRORS=200
.impalarc:
[impala.query_options]
EXPLAIN_LEVEL=2
MT_DOP=2
The options set in command line will update the ones
in impalarc one by one, so the result of the example
above will be:
EXPLAIN_LEVEL=2
MT_DOP=1
MAX_ERRORS=200
Additional changes:
- 0 and 1 are accepted as bools in section [impala] to
make it more consistent with [impala.query_options]
- options that are expected to be bool but are not
0/1/true/false lead to error instead of warning
Change-Id: I26a3b67230c80a99bd246b6af205d558fec9a986
---
M shell/impala_shell.py
M shell/option_parser.py
A tests/shell/impalarc_with_error
A tests/shell/impalarc_with_query_options
A tests/shell/impalarc_with_warnings
M tests/shell/test_shell_commandline.py
M tests/shell/test_shell_interactive.py
7 files changed, 155 insertions(+), 46 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/38/8038/16
--
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: newpatchset
Gerrit-Change-Id: I26a3b67230c80a99bd246b6af205d558fec9a986
Gerrit-Change-Number: 8038
Gerrit-PatchSet: 16
Gerrit-Owner: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj...@apache.org>
Gerrit-Reviewer: Michael Brown <mi...@cloudera.com>
Gerrit-Reviewer: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Reviewer: anujphadke <ap...@cloudera.com>