Mailing-List: contact reviews-help@impala.incubator.apache.org; run by ezmlm
Precedence: bulk
Message-Id: <201710121941.v9CJfCXc029036@ip-10-146-233-104.ec2.internal>
Date: Thu, 12 Oct 2017 19:41:12 +0000
From: "Csaba Ringhofer (Code Review)" <gerrit@cloudera.org>
To: impala-cr@cloudera.com, reviews@impala.incubator.apache.org
CC: Matthew Jacobs <mjacobs@apache.org>, Lars Volker <lv@cloudera.com>,
        Philip Zeyliger <philip@cloudera.com>
Subject: =?UTF-8?Q?=5BImpala-ASF-CR=5D_IMPALA-5736=3A_Add_impala-shell_argument_to_set_default_query_options=0A?=
In-Reply-To: <gerrit.1505231567000.I26a3b67230c80a99bd246b6af205d558fec9a986@gerrit.cloudera.org>
References: <gerrit.1505231567000.I26a3b67230c80a99bd246b6af205d558fec9a986@gerrit.cloudera.org>
Reply-To: csringhofer@cloudera.com, impala-cr@cloudera.com, lv@cloudera.com,
        marcelk@gmail.com, philip@cloudera.com,
        reviews@impala.incubator.apache.org, mjacobs@apache.org
MIME-Version: 1.0
Content-Transfer-Encoding: 8bit
Content-Disposition: inline
User-Agent: Gerrit/2.14.2
Content-Type: multipart/alternative; boundary="ALHVlz0w3N0="; charset=UTF-8
archived-at: Thu, 12 Oct 2017 19:41:20 -0000

--ALHVlz0w3N0=
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: quoted-printable

Csaba Ringhofer has posted comments on this change=2E ( http://gerrit=2Eclo=
udera=2Eorg:8080/8038 )

Change subject: IMPALA-5736: Add impala-shell argu=
ment to set default query options
=2E=2E=2E=2E=2E=2E=2E=2E=2E=2E=2E=2E=2E=
=2E=2E=2E=2E=2E=2E=2E=2E=2E=2E=2E=2E=2E=2E=2E=2E=2E=2E=2E=2E=2E=2E=2E=2E=2E=
=2E=2E=2E=2E=2E=2E=2E=2E=2E=2E=2E=2E=2E=2E=2E=2E=2E=2E=2E=2E=2E=2E=2E=2E=2E=
=2E=2E=2E=2E=2E=2E=2E


Patch Set 4:

(10 comments)

http://gerrit=2Ecloude=
ra=2Eorg:8080/#/c/8038/4/shell/impala_shell=2Epy
File shell/impala_shell=2E=
py:

http://gerrit=2Ecloudera=2Eorg:8080/#/c/8038/4/shell/impala_shell=2Epy=
@686
PS4, Line 686:     tmp_set =3D {}
> Add a comment:
Done


http://gerri=
t=2Ecloudera=2Eorg:8080/#/c/8038/4/shell/impala_shell=2Epy@1330
PS4, Line 1=
330: if __name__ =3D=3D "__main__":
> Perhaps we need a big comment:
Thanks=
 for the nice comment, I made some small changes=2E


http://gerrit=2Ecloud=
era=2Eorg:8080/#/c/8038/4/shell/impala_shell=2Epy@1350
PS4, Line 1350:   # =
default options loaded in from impala_shell_config_defaults=2Epy
> Let's ta=
ke the time to update this comment by disambiguating "shell options
Done


=
http://gerrit=2Ecloudera=2Eorg:8080/#/c/8038/4/shell/impala_shell=2Epy@1354=

PS4, Line 1354:     impala_shell_defaults=2Eupdate(loaded_shell_options)
>=
 I think "impala_query_options_default" is empty, but this assymetry
 > bet=
ween impala_shell_defaults and query_options is weird=2E 

I think it is le=
ss weird now with the long comment at the beginning=2E Query option default=
s come from the server, so the script does not know them yet=2E

 > It's we=
ird that you're updating impala_shell_defaults, rather than
 > updating "sh=
ell_options"=2E

I do not know why it was done this way originally=2E


htt=
p://gerrit=2Ecloudera=2Eorg:8080/#/c/8038/4/shell/impala_shell=2Epy@1437
PS=
4, Line 1437:   query_options=2Eupdate(
> Add comment:
Done


http://gerrit=
=2Ecloudera=2Eorg:8080/#/c/8038/4/shell/option_parser=2Epy
File shell/optio=
n_parser=2Epy:

http://gerrit=2Ecloudera=2Eorg:8080/#/c/8038/4/shell/option=
_parser=2Epy@36
PS4, Line 36: def convert_loaded_shell_option(option, value=
, default_options, config_filename):
> Please document config_filename=2E I=
'm unclear how it's used=2E
Done


http://gerrit=2Ecloudera=2Eorg:8080/#/c/=
8038/4/shell/option_parser=2Epy@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=2Ecloudera=2Eorg:8080/#/c/803=
8/4/shell/option_parser=2Epy@36
PS4, Line 36: def convert_loaded_shell_opti=
on(option, value, default_options, config_filename):
            :     """C=
onverts 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=2Elower() =3D=3D "true":
            :         return=
 True
            :       elif value=2Elower() =3D=3D 'false':
            =
:         return False
            :       else:
            :         # if=
 the option is not set to either true or false, use the default
           =
 :         return default_options[option]
            :     elif value=2Elo=
wer() =3D=3D "none":
            :       return None
            :     elif=
 option=2Elower() =3D=3D "config_file":
            :       return config_f=
ilename
> This function is mixing 2-space indent and 4-space indent=2E Plea=
se clean up=2E
Done


http://gerrit=2Ecloudera=2Eorg:8080/#/c/8038/4/shell/=
option_parser=2Epy@164
PS4, Line 164:                           "Only speci=
fy this as a option in the commandline=2E"
> s/as a option/as an option/
Do=
ne


http://gerrit=2Ecloudera=2Eorg:8080/#/c/8038/4/shell/option_parser=2Ep=
y@183
PS4, Line 183:                     help=3D"Sets default query options=
=2E"
> Add: "May be specified multiple times=2E" Unless it's clear from the=
 usage?
Done


-- 
To view, visit http://gerrit=2Ecloudera=2Eorg:8080/8038=

To unsubscribe, visit http://gerrit=2Ecloudera=2Eorg:8080/settings

Gerrit=
-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerr=
it-Change-Id: I26a3b67230c80a99bd246b6af205d558fec9a986
Gerrit-Change-Numbe=
r: 8038
Gerrit-PatchSet: 4
Gerrit-Owner: Csaba Ringhofer <csringhofer@cloud=
era=2Ecom>
Gerrit-Reviewer: Csaba Ringhofer <csringhofer@cloudera=2Ecom>
Ge=
rrit-Reviewer: Lars Volker <lv@cloudera=2Ecom>
Gerrit-Reviewer: Matthew Jac=
obs <mjacobs@apache=2Eorg>
Gerrit-Reviewer: Philip Zeyliger <philip@clouder=
a=2Ecom>
Gerrit-Comment-Date: Thu, 12 Oct 2017 19:41:12 +0000
Gerrit-HasCom=
ments: Yes

--ALHVlz0w3N0=--