[impala] 03/03: IMPALA-8330: Impala shell config file should support flag names

[jo...@apache.org]

This is an automated email from the ASF dual-hosted git repository.

joemcdonnell pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/impala.git

commit 7adb411bd99812d30e2a8cf795557746591df15e
Author: Fredy Wijaya <fw...@cloudera.com>
AuthorDate: Thu Mar 21 10:05:27 2019 -0700

    IMPALA-8330: Impala shell config file should support flag names
    
    This patch updates the file format in Impala shell config file to accept
    both short and long flag names in addition to optparse's dest names
    (variable names to store flag values) for better user experience because
    dest names are internal to Impala shell.
    
    Format:
    [impala]
    flag_name=flag_value
    
    Example:
    [impala]
    ; This is long flag.
    query=select 1
    ; This is short flag.
    Q=DEFAULT_FILE_FORMAT=parquet
    ; Flags can be repeated with ,
    var=msg1=hello,var=msg2=world
    ; The old format using internal variable name is still supported for
    ; backward compatibility.
    keyval=msg3=foo,keyval=msg4=bar
    
    Testing:
    - Ran all E2E shell tests on Python 2.6 and 2.7.
    
    Change-Id: Ic43603c1b538af08fddcab1b2c1f6ad1af1a6cb9
    Reviewed-on: http://gerrit.cloudera.org:8080/12823
    Reviewed-by: Impala Public Jenkins <im...@cloudera.com>
    Tested-by: Impala Public Jenkins <im...@cloudera.com>
---
 shell/impala_shell.py                 |  1 -
 shell/option_parser.py                | 40 ++++++++++++++++++++++++++++++++---
 tests/shell/good_impalarc             |  6 ++++--
 tests/shell/test_shell_commandline.py |  5 ++++-
 4 files changed, 45 insertions(+), 7 deletions(-)

diff --git a/shell/impala_shell.py b/shell/impala_shell.py
index d1a1163..d382c8b 100755
--- a/shell/impala_shell.py
+++ b/shell/impala_shell.py
@@ -1638,7 +1638,6 @@ if __name__ == "__main__":
     print_to_stderr(e)
     sys.exit(1)
 
-  # TODO: IMPALA-8330: Use flag names instead of dest names.
   parser = get_option_parser(impala_shell_defaults)
   options, args = parser.parse_args()
 
diff --git a/shell/option_parser.py b/shell/option_parser.py
index c14e9c0..ae28121 100755
--- a/shell/option_parser.py
+++ b/shell/option_parser.py
@@ -63,8 +63,18 @@ def parse_shell_options(options, defaults, option_list):
 
      Returns a dictionary with option names as keys and option values as values.
   """
+  # Build a dictionary that maps short and long option name to option for a quick lookup.
+  option_dests = dict()
+  for option in option_list:
+    if len(option._short_opts) > 0:
+      option_dests[option._short_opts[0][1:]] = option
+    if len(option._long_opts) > 0:
+      option_dests[option._long_opts[0][2:]] = option
+    if option.dest not in option_dests:
+      # Allowing dest name for backward compatibility.
+      option_dests[option.dest] = option
+
   result = {}
-  option_dests = dict((opt.dest, opt) for opt in option_list)
   for option, value in options:
     opt = option_dests.get(option)
     if opt is None:
@@ -106,6 +116,8 @@ def get_config_from_file(config_filename, option_list):
   as keys and option values as values.
   """
   config = ConfigParser.ConfigParser()
+  # Preserve case-sensitivity since flag names are case sensitive.
+  config.optionxform = str
   try:
     config.read(config_filename)
   except Exception, e:
@@ -120,6 +132,7 @@ def get_config_from_file(config_filename, option_list):
       print >> sys.stderr, "WARNING: Option 'config_file' can be only set from shell."
       shell_options["config_file"] = config_filename
 
+  config = ConfigParser.ConfigParser()
   try:
     config.read(config_filename)
   except Exception, e:
@@ -142,8 +155,6 @@ def get_option_parser(defaults):
   """
 
   parser = OptionParser()
-  parser.set_defaults(**defaults)
-
   parser.add_option("-i", "--impalad", dest="impalad",
                     help="<host:port> of impalad to connect to \t\t")
   parser.add_option("-b", "--kerberos_host_fqdn", dest="kerberos_host_fqdn",
@@ -246,8 +257,29 @@ def get_option_parser(defaults):
                     help="Timeout in milliseconds after which impala-shell will time out"
                     " if it fails to connect to Impala server. Set to 0 to disable any"
                     " timeout.")
+
   # add default values to the help text
   for option in parser.option_list:
+    if option.dest is not None:
+      # option._short_opts returns a list of short options, e.g. ["-Q"].
+      # option._long_opts returns a list of long options, e.g. ["--query_option"].
+      # The code below removes the - from the short option and -- from the long option.
+      short_opt = option._short_opts[0][1:] if len(option._short_opts) > 0 else None
+      long_opt = option._long_opts[0][2:] if len(option._long_opts) > 0 else None
+      # In order to set the default flag values, optparse requires the keys to be the
+      # dest names. The default flag values are set in impala_shell_config_defaults.py and
+      # the default flag values may contain default values that are not for flags.
+      if short_opt in defaults:
+        if option.dest not in defaults:
+          defaults[option.dest] = defaults[short_opt]
+        elif type(defaults[option.dest]) == list:
+          defaults[option.dest].extend(defaults[short_opt])
+      elif long_opt in defaults:
+        if option.dest not in defaults:
+          defaults[option.dest] = defaults[long_opt]
+        elif type(defaults[option.dest]) == list:
+          defaults[option.dest].extend(defaults[long_opt])
+
     # since the quiet flag is the same as the verbose flag
     # we need to make sure to print the opposite value for it
     # (print quiet is false since verbose is true)
@@ -257,4 +289,6 @@ def get_option_parser(defaults):
       # don't want to print default value for help
       option.help += " [default: %default]"
 
+  parser.set_defaults(**defaults)
+
   return parser
diff --git a/tests/shell/good_impalarc b/tests/shell/good_impalarc
index 8ebc6dd..a1cb541 100644
--- a/tests/shell/good_impalarc
+++ b/tests/shell/good_impalarc
@@ -1,6 +1,8 @@
 [impala]
 query=select 1
-creds_ok_in_clear=true
+auth_creds_ok_in_clear=true
+# An old format using optparse dest name for backward compatibility.
 keyval=msg1=hello,keyval=msg2=world
+var=msg3=foo,var=msg4=bar
 verbose=true
-query_options=DEFAULT_FILE_FORMAT=parquet
\ No newline at end of file
+Q=DEFAULT_FILE_FORMAT=parquet
\ No newline at end of file
diff --git a/tests/shell/test_shell_commandline.py b/tests/shell/test_shell_commandline.py
index 1782925..666c202 100644
--- a/tests/shell/test_shell_commandline.py
+++ b/tests/shell/test_shell_commandline.py
@@ -481,11 +481,14 @@ class TestImpalaShell(ImpalaTestSuite):
 
     # IMPALA-8317: Add support for list-type, i.e. action=append in config file.
     args = '--config_file=%s/good_impalarc ' \
-           '--query="select \'${VAR:msg1}\'; select \'${VAR:msg2}\'; set"' % \
+           '--query="select \'${VAR:msg1}\'; select \'${VAR:msg2}\'; ' \
+           'select \'${VAR:msg3}\'; select \'${VAR:msg4}\'; set"' % \
            QUERY_FILE_PATH
     result = run_impala_shell_cmd(args)
     assert 'Query: select \'hello\'' in result.stderr
     assert 'Query: select \'world\'' in result.stderr
+    assert 'Query: select \'foo\'' in result.stderr
+    assert 'Query: select \'bar\'' in result.stderr
     assert 'DEFAULT_FILE_FORMAT: parquet' in result.stdout
 
     # Override the variables in the config file with the ones passed via --var.