Mailing-List: contact reviews-help@impala.incubator.apache.org; run by ezmlm
Precedence: bulk
Message-Id: <201710030111.v931BoZ6024651@ip-10-146-233-104.ec2.internal>
Date: Tue, 3 Oct 2017 01:11:50 +0000
From: "Impala Public Jenkins (Code Review)" <gerrit@cloudera.org>
To: Philip Zeyliger <philip@cloudera.com>, impala-cr@cloudera.com,
        reviews@impala.incubator.apache.org
Subject: =?UTF-8?Q?=5BImpala-ASF-CR=5D_Re-apply=3A_IMPALA-5589=3A_change_=22set=22_in_impala-shell_to_show_empty_string_for_unset_query_options=0A?=
In-Reply-To: <gerrit.1505769977000.I29f5d8ab874cb1338077f16019a9537766cac0c4@gerrit.cloudera.org>
References: <gerrit.1505769977000.I29f5d8ab874cb1338077f16019a9537766cac0c4@gerrit.cloudera.org>
Reply-To: impala-cr@cloudera.com, philip@cloudera.com,
        reviews@impala.incubator.apache.org
MIME-Version: 1.0
Content-Transfer-Encoding: 8bit
Content-Disposition: inline
User-Agent: Gerrit/2.14.2
Content-Type: multipart/alternative; boundary="3nfydzKMGo4="; charset=UTF-8
archived-at: Tue, 03 Oct 2017 01:11:57 -0000

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

Impala Public Jenkins has submitted this change and it was merged=2E ( http=
://gerrit=2Ecloudera=2Eorg:8080/8096 )

Change subject: Re-apply: IMPALA-55=
89: change "set" in impala-shell to show empty string for unset query optio=
ns
=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

Re-apply: =
IMPALA-5589: change "set" in impala-shell to show empty string for unset qu=
ery options

(Re-applies reverted commit 387bde0639ffd8ef580ccbf727152954e6=
2bacbe=2E
The commit broke ASAN tests due to a race in how test infrastruct=
ure
re-uses connections=2E The fix for that is in an adjacent commit=2E)

W=
hen converting TQueryOptions to a map<string,string>, we now convert
unset =
options to the empty string=2E Within TQueryOptions we have optional
option=
s (like mt_dop or compression_codec) with no default specified=2E In
this c=
ase, the user was seeing 0 for numeric types and the first enum
option for =
enumeration types (e=2Eg=2E, "NONE" in the compression case)=2E
This was co=
nfusing as the implementation handles this "null" case
differently (e=2Eg=
=2E, using SNAPPY as the default codec in the case
reported in the JIRA)=2E=


When running "set" in impala-shell, the difference is as
follows:

    - =
       BUFFER_POOL_LIMIT: [0]
    +        BUFFER_POOL_LIMIT: []
    -     =
   COMPRESSION_CODEC: [NONE]
    +        COMPRESSION_CODEC: []
    -      =
  MT_DOP: [0]
    +        MT_DOP: []
    -        RESERVATION_REQUEST_TIME=
OUT: [0]
    +        RESERVATION_REQUEST_TIMEOUT: []
    -        SEQ_COMP=
RESSION_MODE: [0]
    +        SEQ_COMPRESSION_MODE: []
    -        V_CPU_=
CORES: [0]
    +        V_CPU_CORES: []

Obviously, the empty string is a v=
alid value for a string-typed option, where
it will be impossible to tell t=
he difference between "unset" and "set to empty
string=2E" Today, there are=
 two string-typed options: debug_string defaults to ""
and request_pool has=
 no default=2E An alternative would have been to use
a special token like "=
_unset" or to introduce a new field in the beeswax
Thrift ConfigVariable st=
ruct=2E I think the empty string approach
is clearest=2E

The other users o=
f this state, which I believe are HiveServer2's OpenSession()
call and Hive=
Server2's response to a "SET" query are affected=2E They
benefit from the s=
ame fix, and a new test has been added to test_hs2=2Epy=2E

I did a mild re=
factoring in the HS2 tests to write a helper method
for the very common pat=
tern of excecuting a query=2E

Testing:
* Manual testing with impala-shell
=
* Modified impala-shell tests to check this explicitly for one case=2E
* Mo=
dified HS2 test to check this as well as the SET k=3Dv statement,
  which l=
ooked otherwise untested=2E

Change-Id: I29f5d8ab874cb1338077f16019a9537766=
cac0c4
Reviewed-on: http://gerrit=2Ecloudera=2Eorg:8080/8096
Reviewed-by: D=
an Hecht <dhecht@cloudera=2Ecom>
Tested-by: Impala Public Jenkins
---
M be/=
src/service/query-options-test=2Ecc
M be/src/service/query-options=2Ecc
M b=
e/src/service/query-options=2Eh
M testdata/workloads/functional-query/queri=
es/QueryTest/set=2Etest
M tests/hs2/hs2_test_suite=2Epy
M tests/hs2/test_hs=
2=2Epy
M tests/shell/test_shell_commandline=2Epy
7 files changed, 106 inser=
tions(+), 60 deletions(-)

Approvals:
  Dan Hecht: Looks good to me, approv=
ed
  Impala Public Jenkins: Verified

-- 
To view, visit http://gerrit=2Ecl=
oudera=2Eorg:8080/8096
To unsubscribe, visit http://gerrit=2Ecloudera=2Eorg=
:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Mes=
sageType: merged
Gerrit-Change-Id: I29f5d8ab874cb1338077f16019a9537766cac0c=
4
Gerrit-Change-Number: 8096
Gerrit-PatchSet: 5
Gerrit-Owner: Philip Zeylig=
er <philip@cloudera=2Ecom>
Gerrit-Reviewer: Dan Hecht <dhecht@cloudera=2Eco=
m>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Philip Zeyliger =
<philip@cloudera=2Ecom>

--3nfydzKMGo4=--