You are viewing a plain text version of this content. The canonical link for it is here.

Posted to reviews@impala.apache.org by "Philip Zeyliger (Code Review)" <ge...@cloudera.org> on 2017/08/29 23:34:39 UTC

[Impala-ASF-CR] IMPALA-5589: change "set" in imapla-shell to show empty string for unset query options

Philip Zeyliger has uploaded a new change for review.

  http://gerrit.cloudera.org:8080/7886

Change subject: IMPALA-5589: change "set" in imapla-shell to show empty string for unset query options
......................................................................

IMPALA-5589: change "set" in imapla-shell to show empty string for unset query options

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

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_TIMEOUT: [0]
    +        RESERVATION_REQUEST_TIMEOUT: []
    -        SEQ_COMPRESSION_MODE: [0]
    +        SEQ_COMPRESSION_MODE: []
    -        V_CPU_CORES: [0]
    +        V_CPU_CORES: []

Obviously, the empty string is a valid value for a string-typed option, where
it will be impossible to tell the difference between "unset" and "set to empty
string." Today, there are two string-typed options: debug_string defaults to ""
and request_pool has no default. An alternative would have been to use
a special token like "_unset" or to introduce a new field in the beeswax
Thrift ConfigVariable struct.

The other users of this state, I think HiveServer2's OpenSession()
call and HiveServer2's response to a "SET" query are affected. It seems
like they'd benefit from the same fix, but I've not been able to
adequately run through that code path.

Change-Id: I86bc06a58d67b099da911293202dae9e844c439b
---
M be/src/service/query-options-test.cc
M be/src/service/query-options.cc
M be/src/service/query-options.h
3 files changed, 22 insertions(+), 3 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/86/7886/1
-- 
To view, visit http://gerrit.cloudera.org:8080/7886
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newchange
Gerrit-Change-Id: I86bc06a58d67b099da911293202dae9e844c439b
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Philip Zeyliger <ph...@cloudera.com>

[Impala-ASF-CR] IMPALA-5589: change "set" in imapla-shell to show empty string for unset query options

Posted by "Philip Zeyliger (Code Review)" <ge...@cloudera.org>.

Philip Zeyliger has posted comments on this change.

Change subject: IMPALA-5589: change "set" in imapla-shell to show empty string for unset query options
......................................................................


Patch Set 1:

(5 comments)

http://gerrit.cloudera.org:8080/#/c/7886/1//COMMIT_MSG
Commit Message:

PS1, Line 7: IMPALA-5589: change "set" in imapla-shell to show empty string for unset query options
> an impala shell test would be nice too
added a simple test here.


PS1, Line 7: imapla
> this would be the name of our imap server
Done


PS1, Line 40: 
            : The other users of this state, I think HiveServer2's OpenSession()
            : call and HiveServer2's response to a "SET" query are affected. It seems
            : like they'd benefit from the same fix, but I've not been able to
            : adequately run through that code path.
> you can add a hs2 test in tests/hs2/ , probably test_hs2.py
Done


http://gerrit.cloudera.org:8080/#/c/7886/1/be/src/service/query-options.h
File be/src/service/query-options.h:

PS1, Line 103: that 
> that aren't set and lack defaults
Done


PS1, Line 104: considered "unset", which is mapped
> this is just a bit wordy, I think this makes it more clear:
Done


-- 
To view, visit http://gerrit.cloudera.org:8080/7886
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I86bc06a58d67b099da911293202dae9e844c439b
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Philip Zeyliger <ph...@cloudera.com>
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-5589: change "set" in impala-shell to show empty string for unset query options

Posted by "Dan Hecht (Code Review)" <ge...@cloudera.org>.

Dan Hecht has posted comments on this change.

Change subject: IMPALA-5589: change "set" in impala-shell to show empty string for unset query options
......................................................................


Patch Set 2: Code-Review+1

Matt, please do the +2 when you don't have any more comments.

-- 
To view, visit http://gerrit.cloudera.org:8080/7886
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I86bc06a58d67b099da911293202dae9e844c439b
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Philip Zeyliger <ph...@cloudera.com>
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-5589: change "set" in impala-shell to show empty string for unset query options

Posted by "Matthew Jacobs (Code Review)" <ge...@cloudera.org>.

Matthew Jacobs has posted comments on this change.

Change subject: IMPALA-5589: change "set" in impala-shell to show empty string for unset query options
......................................................................


Patch Set 4: Code-Review+2

-- 
To view, visit http://gerrit.cloudera.org:8080/7886
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I86bc06a58d67b099da911293202dae9e844c439b
Gerrit-PatchSet: 4
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Philip Zeyliger <ph...@cloudera.com>
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-5589: change "set" in impala-shell to show empty string for unset query options

Posted by "Matthew Jacobs (Code Review)" <ge...@cloudera.org>.

Matthew Jacobs has posted comments on this change.

Change subject: IMPALA-5589: change "set" in impala-shell to show empty string for unset query options
......................................................................


Patch Set 2:

Please fix the typos before submitting

-- 
To view, visit http://gerrit.cloudera.org:8080/7886
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I86bc06a58d67b099da911293202dae9e844c439b
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Philip Zeyliger <ph...@cloudera.com>
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-5589: change "set" in impala-shell to show empty string for unset query options

Posted by "Philip Zeyliger (Code Review)" <ge...@cloudera.org>.

Hello Impala Public Jenkins, Matthew Jacobs, Dan Hecht,

I'd like you to reexamine a change.  Please visit

    http://gerrit.cloudera.org:8080/7886

to look at the new patch set (#4).

Change subject: IMPALA-5589: change "set" in impala-shell to show empty string for unset query options
......................................................................

IMPALA-5589: change "set" in impala-shell to show empty string for unset query options

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

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_TIMEOUT: [0]
    +        RESERVATION_REQUEST_TIMEOUT: []
    -        SEQ_COMPRESSION_MODE: [0]
    +        SEQ_COMPRESSION_MODE: []
    -        V_CPU_CORES: [0]
    +        V_CPU_CORES: []

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

The other users of this state, which I believe are HiveServer2's OpenSession()
call and HiveServer2's response to a "SET" query are affected. They
benefit from the same fix, and a new test has been added to test_hs2.py.

I did a mild refactoring in the HS2 tests to write a helper method
for the very common pattern of excecuting a query.

Testing:
* Manual testing with impala-shell
* Modified impala-shell tests to check this explicitly for one case.
* Modified HS2 test to check this as well as the SET k=v statement,
  which looked otherwise untested.

Change-Id: I86bc06a58d67b099da911293202dae9e844c439b
---
M be/src/service/query-options-test.cc
M be/src/service/query-options.cc
M be/src/service/query-options.h
M testdata/workloads/functional-query/queries/QueryTest/set.test
M tests/hs2/hs2_test_suite.py
M tests/hs2/test_hs2.py
M tests/shell/test_shell_commandline.py
7 files changed, 106 insertions(+), 60 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/86/7886/4
-- 
To view, visit http://gerrit.cloudera.org:8080/7886
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I86bc06a58d67b099da911293202dae9e844c439b
Gerrit-PatchSet: 4
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Philip Zeyliger <ph...@cloudera.com>

[Impala-ASF-CR] IMPALA-5589: change "set" in impala-shell to show empty string for unset query options

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.

Impala Public Jenkins has posted comments on this change.

Change subject: IMPALA-5589: change "set" in impala-shell to show empty string for unset query options
......................................................................


Patch Set 3:

Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/1196/

-- 
To view, visit http://gerrit.cloudera.org:8080/7886
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I86bc06a58d67b099da911293202dae9e844c439b
Gerrit-PatchSet: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Philip Zeyliger <ph...@cloudera.com>
Gerrit-HasComments: No