subject:"\[Impala\-ASF\-CR\] IMPALA\-2181\: Add query option levels for display"

[Impala-ASF-CR] IMPALA-2181: Add query option levels for display

2017-11-15 Thread Gabor Kaszab (Code Review)

Gabor Kaszab has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8447 )

Change subject: IMPALA-2181: Add query option levels for display
..


Patch Set 14:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/8447/11/shell/impala_shell.py
File shell/impala_shell.py:

http://gerrit.cloudera.org:8080/#/c/8447/11/shell/impala_shell.py@232
PS11, Line 232:  just the 'Regular'
> This comment s not consistent with the new commit message and code, as adva
Done



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I75720d0d454527e1a0ed19bb43cf9e4f018ce1d1
Gerrit-Change-Number: 8447
Gerrit-PatchSet: 14
Gerrit-Owner: Gabor Kaszab 
Gerrit-Reviewer: Attila Jeges 
Gerrit-Reviewer: Csaba Ringhofer 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Gabor Kaszab 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Laszlo Gaal 
Gerrit-Reviewer: Philip Zeyliger 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Reviewer: Zoltan Borok-Nagy 
Gerrit-Comment-Date: Wed, 15 Nov 2017 21:20:16 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-2181: Add query option levels for display

2017-11-15 Thread Gabor Kaszab (Code Review)

Hello Lars Volker, Laszlo Gaal, Zoltan Borok-Nagy, Philip Zeyliger, Attila 
Jeges, Tim Armstrong, Csaba Ringhofer, Dan Hecht,

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

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

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

Change subject: IMPALA-2181: Add query option levels for display
..

IMPALA-2181: Add query option levels for display

Four display levels are introduced for each query option: REGULAR, ADVANCED,
DEVELOPMENT and DEPRECATED. When the query options are diplayed in Impala shell
using SET then only the REGULAR and ADVANCED options are shown. A new command
called SET ALL shows all the options grouped by their option levels.

Change-Id: I75720d0d454527e1a0ed19bb43cf9e4f018ce1d1
---
M be/src/service/child-query.cc
M be/src/service/client-request-state.cc
M be/src/service/client-request-state.h
M be/src/service/impala-server.cc
M be/src/service/impala-server.h
M be/src/service/query-options.cc
M be/src/service/query-options.h
M common/thrift/Frontend.thrift
M common/thrift/ImpalaService.thrift
M common/thrift/beeswax.thrift
M fe/src/main/cup/sql-parser.cup
M fe/src/main/java/org/apache/impala/analysis/SetStmt.java
M fe/src/main/java/org/apache/impala/service/Frontend.java
M shell/impala_client.py
M shell/impala_shell.py
M testdata/workloads/functional-query/queries/QueryTest/set.test
M tests/custom_cluster/test_set_and_unset.py
M tests/hs2/test_hs2.py
M tests/shell/test_shell_interactive.py
19 files changed, 451 insertions(+), 225 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/47/8447/14
--
To view, visit http://gerrit.cloudera.org:8080/8447
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I75720d0d454527e1a0ed19bb43cf9e4f018ce1d1
Gerrit-Change-Number: 8447
Gerrit-PatchSet: 14
Gerrit-Owner: Gabor Kaszab 
Gerrit-Reviewer: Attila Jeges 
Gerrit-Reviewer: Csaba Ringhofer 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Gabor Kaszab 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Laszlo Gaal 
Gerrit-Reviewer: Philip Zeyliger 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Reviewer: Zoltan Borok-Nagy

[Impala-ASF-CR] IMPALA-2181: Add query option levels for display

2017-11-14 Thread Csaba Ringhofer (Code Review)

Csaba Ringhofer has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8447 )

Change subject: IMPALA-2181: Add query option levels for display
..


Patch Set 13:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/8447/11/shell/impala_shell.py
File shell/impala_shell.py:

http://gerrit.cloudera.org:8080/#/c/8447/11/shell/impala_shell.py@232
PS11, Line 232:  just the 'Regular'
This comment s not consistent with the new commit message and code, as advanced 
options are shown by default too.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I75720d0d454527e1a0ed19bb43cf9e4f018ce1d1
Gerrit-Change-Number: 8447
Gerrit-PatchSet: 13
Gerrit-Owner: Gabor Kaszab 
Gerrit-Reviewer: Attila Jeges 
Gerrit-Reviewer: Csaba Ringhofer 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Gabor Kaszab 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Laszlo Gaal 
Gerrit-Reviewer: Philip Zeyliger 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Reviewer: Zoltan Borok-Nagy 
Gerrit-Comment-Date: Wed, 15 Nov 2017 01:24:08 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-2181: Add query option levels for display

2017-11-14 Thread Gabor Kaszab (Code Review)

Hello Lars Volker, Laszlo Gaal, Zoltan Borok-Nagy, Philip Zeyliger, Attila 
Jeges, Tim Armstrong, Csaba Ringhofer, Dan Hecht,

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

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

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

Change subject: IMPALA-2181: Add query option levels for display
..

IMPALA-2181: Add query option levels for display

Four display levels are introduced for each query option: REGULAR, ADVANCED,
DEVELOPMENT and DEPRECATED. When the query options are diplayed in Impala shell
using SET then only the REGULAR and ADVANCED options are shown. A new command
called SET ALL shows all the options grouped by their option levels.

Change-Id: I75720d0d454527e1a0ed19bb43cf9e4f018ce1d1
---
M be/src/service/child-query.cc
M be/src/service/client-request-state.cc
M be/src/service/client-request-state.h
M be/src/service/impala-server.cc
M be/src/service/impala-server.h
M be/src/service/query-options.cc
M be/src/service/query-options.h
M common/thrift/Frontend.thrift
M common/thrift/ImpalaService.thrift
M common/thrift/beeswax.thrift
M fe/src/main/cup/sql-parser.cup
M fe/src/main/java/org/apache/impala/analysis/SetStmt.java
M fe/src/main/java/org/apache/impala/service/Frontend.java
M shell/impala_client.py
M shell/impala_shell.py
M tests/hs2/test_hs2.py
M tests/shell/test_shell_interactive.py
17 files changed, 360 insertions(+), 136 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/47/8447/13
--
To view, visit http://gerrit.cloudera.org:8080/8447
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I75720d0d454527e1a0ed19bb43cf9e4f018ce1d1
Gerrit-Change-Number: 8447
Gerrit-PatchSet: 13
Gerrit-Owner: Gabor Kaszab 
Gerrit-Reviewer: Attila Jeges 
Gerrit-Reviewer: Csaba Ringhofer 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Gabor Kaszab 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Laszlo Gaal 
Gerrit-Reviewer: Philip Zeyliger 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Reviewer: Zoltan Borok-Nagy

[Impala-ASF-CR] IMPALA-2181: Add query option levels for display

2017-11-14 Thread Gabor Kaszab (Code Review)

Hello Lars Volker, Laszlo Gaal, Zoltan Borok-Nagy, Philip Zeyliger, Attila 
Jeges, Tim Armstrong, Csaba Ringhofer, Dan Hecht,

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

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

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

Change subject: IMPALA-2181: Add query option levels for display
..

IMPALA-2181: Add query option levels for display

Four display levels are introduced for each query option: REGULAR, ADVANCED,
DEVELOPMENT and DEPRECATED. When the query options are diplayed in Impala shell
using SET then only the REGULAR and ADVANCED options are shown. A new command
called SET ALL shows all the options grouped by their option levels.

Change-Id: I75720d0d454527e1a0ed19bb43cf9e4f018ce1d1
---
M be/src/service/child-query.cc
M be/src/service/client-request-state.cc
M be/src/service/client-request-state.h
M be/src/service/impala-server.cc
M be/src/service/impala-server.h
M be/src/service/query-options.cc
M be/src/service/query-options.h
M common/thrift/Frontend.thrift
M common/thrift/ImpalaService.thrift
M common/thrift/beeswax.thrift
M fe/src/main/cup/sql-parser.cup
M fe/src/main/java/org/apache/impala/analysis/SetStmt.java
M fe/src/main/java/org/apache/impala/service/Frontend.java
M shell/impala_client.py
M shell/impala_shell.py
M tests/hs2/test_hs2.py
M tests/shell/test_shell_interactive.py
17 files changed, 359 insertions(+), 136 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/47/8447/11
--
To view, visit http://gerrit.cloudera.org:8080/8447
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I75720d0d454527e1a0ed19bb43cf9e4f018ce1d1
Gerrit-Change-Number: 8447
Gerrit-PatchSet: 11
Gerrit-Owner: Gabor Kaszab 
Gerrit-Reviewer: Attila Jeges 
Gerrit-Reviewer: Csaba Ringhofer 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Gabor Kaszab 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Laszlo Gaal 
Gerrit-Reviewer: Philip Zeyliger 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Reviewer: Zoltan Borok-Nagy

[Impala-ASF-CR] IMPALA-2181: Add query option levels for display

2017-11-09 Thread Gabor Kaszab (Code Review)

Gabor Kaszab has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8447 )

Change subject: IMPALA-2181: Add query option levels for display
..


Patch Set 10:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/8447/9/common/thrift/beeswax.thrift
File common/thrift/beeswax.thrift:

http://gerrit.cloudera.org:8080/#/c/8447/9/common/thrift/beeswax.thrift@103
PS9, Line 103: }
> Why is this a byte? Shouldn't this be an enum?
Done



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I75720d0d454527e1a0ed19bb43cf9e4f018ce1d1
Gerrit-Change-Number: 8447
Gerrit-PatchSet: 10
Gerrit-Owner: Gabor Kaszab 
Gerrit-Reviewer: Attila Jeges 
Gerrit-Reviewer: Csaba Ringhofer 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Gabor Kaszab 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Laszlo Gaal 
Gerrit-Reviewer: Philip Zeyliger 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Reviewer: Zoltan Borok-Nagy 
Gerrit-Comment-Date: Thu, 09 Nov 2017 19:27:42 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-2181: Add query option levels for display

2017-11-09 Thread Gabor Kaszab (Code Review)

Hello Lars Volker, Laszlo Gaal, Zoltan Borok-Nagy, Philip Zeyliger, Attila 
Jeges, Tim Armstrong, Csaba Ringhofer, Dan Hecht,

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

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

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

Change subject: IMPALA-2181: Add query option levels for display
..

IMPALA-2181: Add query option levels for display

Three display levels are introduced for each query option: REGULAR, ADVANCED
and DEPRECATED. When the query options are diplayed in Impala shell using
SET then only the REGULAR options are shown. A new command called SET ALL
shows all the options grouped by their option levels.

Change-Id: I75720d0d454527e1a0ed19bb43cf9e4f018ce1d1
---
M be/src/service/child-query.cc
M be/src/service/impala-server.cc
M be/src/service/impala-server.h
M be/src/service/query-options.cc
M be/src/service/query-options.h
M common/thrift/ImpalaService.thrift
M common/thrift/beeswax.thrift
M shell/impala_client.py
M shell/impala_shell.py
M tests/shell/test_shell_interactive.py
10 files changed, 222 insertions(+), 91 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/47/8447/10
--
To view, visit http://gerrit.cloudera.org:8080/8447
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I75720d0d454527e1a0ed19bb43cf9e4f018ce1d1
Gerrit-Change-Number: 8447
Gerrit-PatchSet: 10
Gerrit-Owner: Gabor Kaszab 
Gerrit-Reviewer: Attila Jeges 
Gerrit-Reviewer: Csaba Ringhofer 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Gabor Kaszab 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Laszlo Gaal 
Gerrit-Reviewer: Philip Zeyliger 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Reviewer: Zoltan Borok-Nagy

[Impala-ASF-CR] IMPALA-2181: Add query option levels for display

2017-11-08 Thread Philip Zeyliger (Code Review)

Philip Zeyliger has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8447 )

Change subject: IMPALA-2181: Add query option levels for display
..


Patch Set 9:

(1 comment)

I'm still a little weirded out that we want "SET" to behave differently in the 
impala-shell and in HS2. Tim/Dan: you ok with it?

http://gerrit.cloudera.org:8080/#/c/8447/9/common/thrift/beeswax.thrift
File common/thrift/beeswax.thrift:

http://gerrit.cloudera.org:8080/#/c/8447/9/common/thrift/beeswax.thrift@103
PS9, Line 103:   4: optional byte level
Why is this a byte? Shouldn't this be an enum?



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I75720d0d454527e1a0ed19bb43cf9e4f018ce1d1
Gerrit-Change-Number: 8447
Gerrit-PatchSet: 9
Gerrit-Owner: Gabor Kaszab 
Gerrit-Reviewer: Attila Jeges 
Gerrit-Reviewer: Csaba Ringhofer 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Gabor Kaszab 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Laszlo Gaal 
Gerrit-Reviewer: Philip Zeyliger 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Reviewer: Zoltan Borok-Nagy 
Gerrit-Comment-Date: Wed, 08 Nov 2017 20:56:55 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-2181: Add query option levels for display

2017-11-08 Thread Gabor Kaszab (Code Review)

Gabor Kaszab has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8447 )

Change subject: IMPALA-2181: Add query option levels for display
..


Patch Set 9:

(3 comments)

http://gerrit.cloudera.org:8080/#/c/8447/8/be/src/service/impala-server.h
File be/src/service/impala-server.h:

http://gerrit.cloudera.org:8080/#/c/8447/8/be/src/service/impala-server.h@516
PS8, Line 516:   /// This function sets the option level for parameter 'option' 
based on the mapping
 :   /// stored in 'query_option_levels_'.
 :   /// The option level is used by the Impala shell when it disp
> The old comment is still here.
Oh man, thanks :)
Done


http://gerrit.cloudera.org:8080/#/c/8447/8/shell/impala_shell.py
File shell/impala_shell.py:

http://gerrit.cloudera.org:8080/#/c/8447/8/shell/impala_shell.py@241
PS8, Line 241: advanced_options:
> nit: you don't need to call len() to check if a dictionary is empty:
Done


http://gerrit.cloudera.org:8080/#/c/8447/8/shell/impala_shell.py@244
PS8, Line 244: deprecated_options:
> Same as L241
Done



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I75720d0d454527e1a0ed19bb43cf9e4f018ce1d1
Gerrit-Change-Number: 8447
Gerrit-PatchSet: 9
Gerrit-Owner: Gabor Kaszab 
Gerrit-Reviewer: Attila Jeges 
Gerrit-Reviewer: Csaba Ringhofer 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Gabor Kaszab 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Laszlo Gaal 
Gerrit-Reviewer: Philip Zeyliger 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Reviewer: Zoltan Borok-Nagy 
Gerrit-Comment-Date: Wed, 08 Nov 2017 18:26:38 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-2181: Add query option levels for display

2017-11-08 Thread Gabor Kaszab (Code Review)

Hello Lars Volker, Laszlo Gaal, Zoltan Borok-Nagy, Philip Zeyliger, Attila 
Jeges, Tim Armstrong, Csaba Ringhofer, Dan Hecht,

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

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

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

Change subject: IMPALA-2181: Add query option levels for display
..

IMPALA-2181: Add query option levels for display

Three display levels are introduced for each query option: REGULAR, ADVANCED
and DEPRECATED. When the query options are diplayed in Impala shell using
SET then only the REGULAR options are shown. A new command called SET ALL
shows all the options grouped by their option levels.

Change-Id: I75720d0d454527e1a0ed19bb43cf9e4f018ce1d1
---
M be/src/service/child-query.cc
M be/src/service/impala-server.cc
M be/src/service/impala-server.h
M be/src/service/query-options.cc
M be/src/service/query-options.h
M common/thrift/ImpalaService.thrift
M common/thrift/beeswax.thrift
M shell/impala_client.py
M shell/impala_shell.py
M tests/shell/test_shell_interactive.py
10 files changed, 221 insertions(+), 91 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/47/8447/9
--
To view, visit http://gerrit.cloudera.org:8080/8447
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I75720d0d454527e1a0ed19bb43cf9e4f018ce1d1
Gerrit-Change-Number: 8447
Gerrit-PatchSet: 9
Gerrit-Owner: Gabor Kaszab 
Gerrit-Reviewer: Attila Jeges 
Gerrit-Reviewer: Csaba Ringhofer 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Gabor Kaszab 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Laszlo Gaal 
Gerrit-Reviewer: Philip Zeyliger 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Reviewer: Zoltan Borok-Nagy

[Impala-ASF-CR] IMPALA-2181: Add query option levels for display

2017-11-08 Thread Zoltan Borok-Nagy (Code Review)

Zoltan Borok-Nagy has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8447 )

Change subject: IMPALA-2181: Add query option levels for display
..


Patch Set 8:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/8447/8/be/src/service/impala-server.h
File be/src/service/impala-server.h:

http://gerrit.cloudera.org:8080/#/c/8447/8/be/src/service/impala-server.h@516
PS8, Line 516:   /// Given a query option name this function gets the option 
level for it to use when
 :   /// displaying from Impala shell and adds the level to the 
ConfigVariable parameter
 :   /// in numeric format. For values see TQueryOptionLevel enum.
The old comment is still here.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I75720d0d454527e1a0ed19bb43cf9e4f018ce1d1
Gerrit-Change-Number: 8447
Gerrit-PatchSet: 8
Gerrit-Owner: Gabor Kaszab 
Gerrit-Reviewer: Attila Jeges 
Gerrit-Reviewer: Csaba Ringhofer 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Gabor Kaszab 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Laszlo Gaal 
Gerrit-Reviewer: Philip Zeyliger 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Reviewer: Zoltan Borok-Nagy 
Gerrit-Comment-Date: Wed, 08 Nov 2017 09:50:04 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-2181: Add query option levels for display

2017-11-08 Thread Attila Jeges (Code Review)

Attila Jeges has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8447 )

Change subject: IMPALA-2181: Add query option levels for display
..


Patch Set 8:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/8447/8/shell/impala_shell.py
File shell/impala_shell.py:

http://gerrit.cloudera.org:8080/#/c/8447/8/shell/impala_shell.py@241
PS8, Line 241: len(advanced_options) > 0
nit: you don't need to call len() to check if a dictionary is empty:

if advanced_options:
  ..


http://gerrit.cloudera.org:8080/#/c/8447/8/shell/impala_shell.py@244
PS8, Line 244: len(deprecated_options) > 0
Same as L241



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I75720d0d454527e1a0ed19bb43cf9e4f018ce1d1
Gerrit-Change-Number: 8447
Gerrit-PatchSet: 8
Gerrit-Owner: Gabor Kaszab 
Gerrit-Reviewer: Attila Jeges 
Gerrit-Reviewer: Csaba Ringhofer 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Gabor Kaszab 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Laszlo Gaal 
Gerrit-Reviewer: Philip Zeyliger 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Reviewer: Zoltan Borok-Nagy 
Gerrit-Comment-Date: Wed, 08 Nov 2017 08:55:57 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-2181: Add query option levels for display

2017-11-07 Thread Gabor Kaszab (Code Review)

Gabor Kaszab has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8447 )

Change subject: IMPALA-2181: Add query option levels for display
..


Patch Set 8:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/8447/8/be/src/service/impala-server.cc
File be/src/service/impala-server.cc:

http://gerrit.cloudera.org:8080/#/c/8447/8/be/src/service/impala-server.cc@1215
PS8, Line 1215:   string_map["support_start_over"] = "false";
> This may not be related to this change, but it would be good to think a bit
I agree that support_start_over implementation is a bit weird.
Note1, this change is only about introducing visibility levels on query 
options, not to refactor how they are implemented.
Note2, currently this change only affects the Beeswax interface that is used by 
impala-shell, but doesn't change HS2 interface towards e.g. HUE. So this option 
won't be hidden from HUE.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I75720d0d454527e1a0ed19bb43cf9e4f018ce1d1
Gerrit-Change-Number: 8447
Gerrit-PatchSet: 8
Gerrit-Owner: Gabor Kaszab 
Gerrit-Reviewer: Attila Jeges 
Gerrit-Reviewer: Csaba Ringhofer 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Gabor Kaszab 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Laszlo Gaal 
Gerrit-Reviewer: Philip Zeyliger 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Reviewer: Zoltan Borok-Nagy 
Gerrit-Comment-Date: Wed, 08 Nov 2017 00:05:53 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-2181: Add query option levels for display

2017-11-07 Thread Csaba Ringhofer (Code Review)

Csaba Ringhofer has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8447 )

Change subject: IMPALA-2181: Add query option levels for display
..


Patch Set 8:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/8447/8/be/src/service/impala-server.cc
File be/src/service/impala-server.cc:

http://gerrit.cloudera.org:8080/#/c/8447/8/be/src/service/impala-server.cc@1215
PS8, Line 1215:   string_map["support_start_over"] = "false";
This may not be related to this change, but it would be good to think a bit 
about support_start_over option. Unlike other options, it is not an integer, 
and it should not be changed. It is read by Hue ccording to 
https://www.cloudera.com/documentation/enterprise/5-4-x/topics/impala_support_start_over.html

I do not know how Hue accesses query options, but if via the output of "set;" 
hiding it by default may actually change the behavior of Hue. Even if this is 
not an issue, it would be probably better to separate this option clearly e.g. 
by giving it a separate level like "capability" and treat these query options 
as read only.

( this comment added support_start_over a long time ago:
https://github.com/cloudera/Impala/commit/94a5ed487e04b7d2c9bf6bee399f0f464f289211b
 )



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I75720d0d454527e1a0ed19bb43cf9e4f018ce1d1
Gerrit-Change-Number: 8447
Gerrit-PatchSet: 8
Gerrit-Owner: Gabor Kaszab 
Gerrit-Reviewer: Attila Jeges 
Gerrit-Reviewer: Csaba Ringhofer 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Gabor Kaszab 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Laszlo Gaal 
Gerrit-Reviewer: Philip Zeyliger 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Reviewer: Zoltan Borok-Nagy 
Gerrit-Comment-Date: Tue, 07 Nov 2017 23:07:55 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-2181: Add query option levels for display

2017-11-07 Thread Gabor Kaszab (Code Review)

Hello Lars Volker, Laszlo Gaal, Zoltan Borok-Nagy, Philip Zeyliger, Attila 
Jeges, Tim Armstrong, Csaba Ringhofer, Dan Hecht,

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

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

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

Change subject: IMPALA-2181: Add query option levels for display
..

IMPALA-2181: Add query option levels for display

Three display levels are introduced for each query option: REGULAR, ADVANCED
and DEPRECATED. When the query options are diplayed in Impala shell using
SET then only the REGULAR options are shown. A new command called SET ALL
shows all the options grouped by their option levels.

Change-Id: I75720d0d454527e1a0ed19bb43cf9e4f018ce1d1
---
M be/src/service/child-query.cc
M be/src/service/impala-server.cc
M be/src/service/impala-server.h
M be/src/service/query-options.cc
M be/src/service/query-options.h
M common/thrift/ImpalaService.thrift
M common/thrift/beeswax.thrift
M shell/impala_client.py
M shell/impala_shell.py
M tests/shell/test_shell_interactive.py
10 files changed, 225 insertions(+), 91 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/47/8447/8
--
To view, visit http://gerrit.cloudera.org:8080/8447
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I75720d0d454527e1a0ed19bb43cf9e4f018ce1d1
Gerrit-Change-Number: 8447
Gerrit-PatchSet: 8
Gerrit-Owner: Gabor Kaszab 
Gerrit-Reviewer: Attila Jeges 
Gerrit-Reviewer: Csaba Ringhofer 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Gabor Kaszab 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Laszlo Gaal 
Gerrit-Reviewer: Philip Zeyliger 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Reviewer: Zoltan Borok-Nagy

[Impala-ASF-CR] IMPALA-2181: Add query option levels for display

2017-11-07 Thread Gabor Kaszab (Code Review)

Gabor Kaszab has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8447 )

Change subject: IMPALA-2181: Add query option levels for display
..


Patch Set 8:

(4 comments)

http://gerrit.cloudera.org:8080/#/c/8447/7/be/src/service/impala-server.h
File be/src/service/impala-server.h:

http://gerrit.cloudera.org:8080/#/c/8447/7/be/src/service/impala-server.h@516
PS7, Line 516:   /// Given a query option name this function gets the option 
level for it to use when
 :   /// displaying from Impala shell and adds the level to the 
ConfigVariable parameter
 :   /// in numeric format. For values see TQueryOptionLevel enum.
> nit: For me, this comment is too complex and tells too much implementation
Thx, that sounds better indeed.
Done


http://gerrit.cloudera.org:8080/#/c/8447/7/be/src/service/impala-server.cc
File be/src/service/impala-server.cc:

http://gerrit.cloudera.org:8080/#/c/8447/7/be/src/service/impala-server.cc@1228
PS7, Line 1228: const string& option_key
> Why do we pass option_key here? It is already there in config, isn't it?
In this case it is. But if I relied on that than it would introduce some risk 
when someone starts to rearrange the part where config is populated as this 
function can't guarantee that config contains the key every time.

To be on the safe side I think it's reasonable to keep the key as a parameter.


http://gerrit.cloudera.org:8080/#/c/8447/7/shell/impala_shell.py
File shell/impala_shell.py:

http://gerrit.cloudera.org:8080/#/c/8447/7/shell/impala_shell.py@240
PS7, Line 240:   if print_mode == QueryOptionDisplayModes.ALL_OPTIONS:
 : if len(advanced_options) > 0:
 :   print '\nAdvanced Query Options:'
 :   self._print_option_group(advanced_options)
 : if len(deprecated_options) > 0:
 :   print '\nDeprecated Query Options:'
> nit: this can be simplified as:
Done


http://gerrit.cloudera.org:8080/#/c/8447/7/shell/impala_shell.py@620
PS7, Line 620: ESS
 :
 : # Remove any extra spaces surrounding the tokens.
> These members are already available in _print_with_set through self. And as
Good point!
Done



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I75720d0d454527e1a0ed19bb43cf9e4f018ce1d1
Gerrit-Change-Number: 8447
Gerrit-PatchSet: 8
Gerrit-Owner: Gabor Kaszab 
Gerrit-Reviewer: Attila Jeges 
Gerrit-Reviewer: Csaba Ringhofer 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Gabor Kaszab 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Laszlo Gaal 
Gerrit-Reviewer: Philip Zeyliger 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Reviewer: Zoltan Borok-Nagy 
Gerrit-Comment-Date: Tue, 07 Nov 2017 20:51:31 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-2181: Add query option levels for display

2017-11-07 Thread Zoltan Borok-Nagy (Code Review)

Zoltan Borok-Nagy has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8447 )

Change subject: IMPALA-2181: Add query option levels for display
..


Patch Set 7:

(3 comments)

http://gerrit.cloudera.org:8080/#/c/8447/7/be/src/service/impala-server.h
File be/src/service/impala-server.h:

http://gerrit.cloudera.org:8080/#/c/8447/7/be/src/service/impala-server.h@516
PS7, Line 516:   /// Given a query option name this function gets the option 
level for it to use when
 :   /// displaying from Impala shell and adds the level to the 
ConfigVariable parameter
 :   /// in numeric format. For values see TQueryOptionLevel enum.
nit: For me, this comment is too complex and tells too much implementation 
details. How about something like this:
This function sets the option level for parameter 'option' based on the mapping 
stored in 'query_option_levels_'.
The option level is used by the Impala shell when it displays the options.


http://gerrit.cloudera.org:8080/#/c/8447/7/be/src/service/impala-server.cc
File be/src/service/impala-server.cc:

http://gerrit.cloudera.org:8080/#/c/8447/7/be/src/service/impala-server.cc@1228
PS7, Line 1228: const string& option_key
Why do we pass option_key here? It is already there in config, isn't it?


http://gerrit.cloudera.org:8080/#/c/8447/7/shell/impala_shell.py
File shell/impala_shell.py:

http://gerrit.cloudera.org:8080/#/c/8447/7/shell/impala_shell.py@620
PS7, Line 620: self.imp_client.default_query_options,
 :   self.set_query_options,
 :   self.imp_client.query_option_levels
These members are already available in _print_with_set through self. And as far 
as I can see the same members are passed on every callsites.
I think it would be easier to read if _print_with_set didn't take these params.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I75720d0d454527e1a0ed19bb43cf9e4f018ce1d1
Gerrit-Change-Number: 8447
Gerrit-PatchSet: 7
Gerrit-Owner: Gabor Kaszab 
Gerrit-Reviewer: Attila Jeges 
Gerrit-Reviewer: Csaba Ringhofer 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Gabor Kaszab 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Laszlo Gaal 
Gerrit-Reviewer: Philip Zeyliger 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Reviewer: Zoltan Borok-Nagy 
Gerrit-Comment-Date: Tue, 07 Nov 2017 16:25:06 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-2181: Add query option levels for display

2017-11-07 Thread Attila Jeges (Code Review)

Attila Jeges has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8447 )

Change subject: IMPALA-2181: Add query option levels for display
..


Patch Set 7:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/8447/7/shell/impala_shell.py
File shell/impala_shell.py:

http://gerrit.cloudera.org:8080/#/c/8447/7/shell/impala_shell.py@240
PS7, Line 240:   if len(advanced_options) > 0 and print_mode == 
QueryOptionDisplayModes.ALL_OPTIONS:
 : print '\nAdvanced Query Options:'
 : self._print_option_group(advanced_options, set_options)
 :   if len(deprecated_options) > 0 and print_mode == 
QueryOptionDisplayModes.ALL_OPTIONS:
 : print '\nDeprecated Query Options:'
 : self._print_option_group(deprecated_options, set_options)
nit: this can be simplified as:

if print_mode == QueryOptionDisplayModes.ALL_OPTIONS:
  if advanced_options:
print '\nAdvanced Query Options:'
self._print_option_group(advanced_options, set_options)
  if deprecated_options:
print '\nDeprecated Query Options:'
self._print_option_group(deprecated_options, set_options)



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I75720d0d454527e1a0ed19bb43cf9e4f018ce1d1
Gerrit-Change-Number: 8447
Gerrit-PatchSet: 7
Gerrit-Owner: Gabor Kaszab 
Gerrit-Reviewer: Attila Jeges 
Gerrit-Reviewer: Csaba Ringhofer 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Gabor Kaszab 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Laszlo Gaal 
Gerrit-Reviewer: Philip Zeyliger 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Reviewer: Zoltan Borok-Nagy 
Gerrit-Comment-Date: Tue, 07 Nov 2017 15:01:23 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-2181: Add query option levels for display

2017-11-06 Thread Gabor Kaszab (Code Review)

Gabor Kaszab has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8447 )

Change subject: IMPALA-2181: Add query option levels for display
..


Patch Set 6:

(5 comments)

Thanks All to take a look!

Phil, about the HS2 part: Initially we discussed with Tim to do this 
modification only on the beeswax interface and leaving HS2 part as it is now.

http://gerrit.cloudera.org:8080/#/c/8447/5/be/src/service/query-options.cc
File be/src/service/query-options.cc:

http://gerrit.cloudera.org:8080/#/c/8447/5/be/src/service/query-options.cc@618
PS5, Line 618: TQueryOptionLevel::ADVANCE
> Probably I'm missing something here, but shouldn't this be ADVANCED?
Nice catch, thx!
Done


http://gerrit.cloudera.org:8080/#/c/8447/5/common/thrift/ImpalaInternalService.thrift
File common/thrift/ImpalaInternalService.thrift:

http://gerrit.cloudera.org:8080/#/c/8447/5/common/thrift/ImpalaInternalService.thrift@297
PS5, Line 297: // Per-client session state
 : struct TSessionState {
 :   // A unique identifier for this session
> This worries me a bit, for a couple of reasons.
You're right. Keeping the levels inside TQueryOptions is not the best idea, I 
removed them and moved a map for the same purpose to ImpalaServer.


http://gerrit.cloudera.org:8080/#/c/8447/5/shell/impala_client.py
File shell/impala_client.py:

http://gerrit.cloudera.org:8080/#/c/8447/5/shell/impala_client.py@100
PS5, Line 100:   new_indent_level, output):
> Have we decided we can't change ConfigVariable to include a level? It seems
Good point. I introduced a new optional 'level' field to ConfigVariable.


http://gerrit.cloudera.org:8080/#/c/8447/2/shell/impala_shell.py
File shell/impala_shell.py:

http://gerrit.cloudera.org:8080/#/c/8447/2/shell/impala_shell.py@127
PS2, Line 127: 'LIVE_SUMMARY' : (lambda x: x in ("true", "TRUE", "True", 
"1"), "print_summary")
 :   }
> Ok, I see your point.
Done


http://gerrit.cloudera.org:8080/#/c/8447/2/tests/shell/test_shell_interactive.py
File tests/shell/test_shell_interactive.py:

http://gerrit.cloudera.org:8080/#/c/8447/2/tests/shell/test_shell_interactive.py@351
PS2, Line 351:result = shell1.get_result()
 : assert "Query options (defaults shown in []):" in 
result.stdout
 : assert "ABORT_ON_ERROR" in result.stdout
> Thanks for the explanation.
Yes, it should be advanced. Added a check for SUPPORT_START_OVER as well.
Done



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I75720d0d454527e1a0ed19bb43cf9e4f018ce1d1
Gerrit-Change-Number: 8447
Gerrit-PatchSet: 6
Gerrit-Owner: Gabor Kaszab 
Gerrit-Reviewer: Attila Jeges 
Gerrit-Reviewer: Csaba Ringhofer 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Gabor Kaszab 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Laszlo Gaal 
Gerrit-Reviewer: Philip Zeyliger 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Reviewer: Zoltan Borok-Nagy 
Gerrit-Comment-Date: Tue, 07 Nov 2017 01:36:37 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-2181: Add query option levels for display

2017-11-06 Thread Gabor Kaszab (Code Review)

Hello Lars Volker, Laszlo Gaal, Zoltan Borok-Nagy, Philip Zeyliger, Attila 
Jeges, Tim Armstrong, Csaba Ringhofer, Dan Hecht,

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

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

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

Change subject: IMPALA-2181: Add query option levels for display
..

IMPALA-2181: Add query option levels for display

Three display levels are introduced for each query option: REGULAR, ADVANCED
and DEPRECATED. When the query options are diplayed in Impala shell using
SET then only the REGULAR options are shown. A new command called SET ALL
shows all the options grouped by their option levels.

Change-Id: I75720d0d454527e1a0ed19bb43cf9e4f018ce1d1
---
M be/src/service/child-query.cc
M be/src/service/impala-server.cc
M be/src/service/impala-server.h
M be/src/service/query-options.cc
M be/src/service/query-options.h
M common/thrift/ImpalaService.thrift
M common/thrift/beeswax.thrift
M shell/impala_client.py
M shell/impala_shell.py
M tests/shell/test_shell_interactive.py
10 files changed, 229 insertions(+), 89 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/47/8447/6
--
To view, visit http://gerrit.cloudera.org:8080/8447
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I75720d0d454527e1a0ed19bb43cf9e4f018ce1d1
Gerrit-Change-Number: 8447
Gerrit-PatchSet: 6
Gerrit-Owner: Gabor Kaszab 
Gerrit-Reviewer: Attila Jeges 
Gerrit-Reviewer: Csaba Ringhofer 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Gabor Kaszab 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Laszlo Gaal 
Gerrit-Reviewer: Philip Zeyliger 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Reviewer: Zoltan Borok-Nagy

[Impala-ASF-CR] IMPALA-2181: Add query option levels for display

2017-11-03 Thread Philip Zeyliger (Code Review)

Philip Zeyliger has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8447 )

Change subject: IMPALA-2181: Add query option levels for display
..


Patch Set 5: Code-Review-1

(3 comments)

I took a quick look through. I'm uncomfortable with this change as it stands, 
because I feel it abuses the Thrift structures too much. Specifically, keeping 
the map of query option levels in TQueryOptions seems wrong: you're really 
describing "Query Option Metadata" (which might include name, description/help, 
and level), rather than the query options themselves. And, for sending this 
stuff to impala-shell, abusing the 'description' field seems wrong. (In fact, 
impala-shell should actually show useful help for these options. Things like 
"MT_DOP" are completely opaque.

Implementation wise, I think we also need to worry about what happens for the 
HiveServer2 side of things. Specifically, we respond to "SET" queries over HS2 
at 
https://github.com/apache/incubator-impala/blob/1803b403e38b3f952afc4ff3fd3e5f4d14c088f8/be/src/service/client-request-state.cc#L194.
 I think we want to stay away from adding information into the Beeswax API that 
isn't also exposed in HS2. One option is to let "SET ALL" be meaningful as a 
query for HS2, though I don't quite know what that means in terms of the schema 
returned: how would you divide the regular/advanced/deprecated options over 
that API?

http://gerrit.cloudera.org:8080/#/c/8447/5/common/thrift/ImpalaInternalService.thrift
File common/thrift/ImpalaInternalService.thrift:

http://gerrit.cloudera.org:8080/#/c/8447/5/common/thrift/ImpalaInternalService.thrift@65
PS5, Line 65: // Levels to use when displaying query options from Impala shell
Let's leave a pointer here indicating where the mapping from the options in 
TQueryOptions to these levels is defined.


http://gerrit.cloudera.org:8080/#/c/8447/5/common/thrift/ImpalaInternalService.thrift@297
PS5, Line 297:   // Map for query option name to option level mapping. 
Populated by ImpalaServer
 :   // on startup
 :   61: required map 
query_option_levels;
This worries me a bit, for a couple of reasons.

First, 'required' is forever. (See 
https://diwakergupta.github.io/thrift-missing-guide/#_defining_structs.) I 
think it means that a Thrift client will refuse to serialize a struct if this 
isn't set. For some uses, like TClientRequest, it makes no sense for us to 
carry this around. It's possible it works out because it can be set to the 
empty map often, but it's still odd.

Second, this isn't a query option. It's certainly describing query options, but 
it's very very different from, say, "mem_limit", which applies to the query.


http://gerrit.cloudera.org:8080/#/c/8447/5/shell/impala_client.py
File shell/impala_client.py:

http://gerrit.cloudera.org:8080/#/c/8447/5/shell/impala_client.py@100
PS5, Line 100:   def get_query_option_level_from_description(self, description):
Have we decided we can't change ConfigVariable to include a level? It seems 
like this is very much not a description, and we're abusing it.

/** Represents a Hadoop-style configuration variable. */
struct ConfigVariable {
  1: string key,
  2: string value,
  3: string description
}



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I75720d0d454527e1a0ed19bb43cf9e4f018ce1d1
Gerrit-Change-Number: 8447
Gerrit-PatchSet: 5
Gerrit-Owner: Gabor Kaszab 
Gerrit-Reviewer: Attila Jeges 
Gerrit-Reviewer: Csaba Ringhofer 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Gabor Kaszab 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Laszlo Gaal 
Gerrit-Reviewer: Philip Zeyliger 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Reviewer: Zoltan Borok-Nagy 
Gerrit-Comment-Date: Fri, 03 Nov 2017 16:33:03 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-2181: Add query option levels for display

2017-11-03 Thread Dan Hecht (Code Review)

Dan Hecht has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8447 )

Change subject: IMPALA-2181: Add query option levels for display
..


Patch Set 5:

Sorry I didn't see the discussion on the JIRA about "advanced". I don't think 
that solves the problem that the JIRA was meaning to solve (though I don't 
think implementation would have to change too much).  I'll add a comment to the 
JIRA.


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I75720d0d454527e1a0ed19bb43cf9e4f018ce1d1
Gerrit-Change-Number: 8447
Gerrit-PatchSet: 5
Gerrit-Owner: Gabor Kaszab 
Gerrit-Reviewer: Attila Jeges 
Gerrit-Reviewer: Csaba Ringhofer 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Gabor Kaszab 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Laszlo Gaal 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Reviewer: Zoltan Borok-Nagy 
Gerrit-Comment-Date: Fri, 03 Nov 2017 16:17:20 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-2181: Add query option levels for display

2017-11-03 Thread Attila Jeges (Code Review)

Attila Jeges has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8447 )

Change subject: IMPALA-2181: Add query option levels for display
..


Patch Set 2:

(3 comments)

http://gerrit.cloudera.org:8080/#/c/8447/5/be/src/service/query-options.cc
File be/src/service/query-options.cc:

http://gerrit.cloudera.org:8080/#/c/8447/5/be/src/service/query-options.cc@618
PS5, Line 618:
Probably I'm missing something here, but shouldn't this be ADVANCED?


http://gerrit.cloudera.org:8080/#/c/8447/2/shell/impala_shell.py
File shell/impala_shell.py:

http://gerrit.cloudera.org:8080/#/c/8447/2/shell/impala_shell.py@127
PS2, Line 127:   PRINT_REGULAR_OPTIONS_ONLY = 1
 :   PRINT_ALL_OPTIONS = 2
> I found it more verbose to have 2 const instead of a bool.
Ok, I see your point.

You could also put the PRINT_* members into a separate class to emulate enums 
(see class CmdStatus in L61-L69). Same goes for QUERY_OPTION_* members. On the 
other hand this might be an overkill, so use your best judgement :)


http://gerrit.cloudera.org:8080/#/c/8447/2/tests/shell/test_shell_interactive.py
File tests/shell/test_shell_interactive.py:

http://gerrit.cloudera.org:8080/#/c/8447/2/tests/shell/test_shell_interactive.py@351
PS2, Line 351:result = shell1.get_result()
 : assert "Query options (defaults shown in []):" in 
result.stdout
 : assert "ABORT_ON_ERROR" in result.stdout
> No. SUPPORT_START_OVER as I discussed with Tim is a tricky one and we don't
Thanks for the explanation.

I still find confusing that in impala::PopulateQueryOptionLevels() the query 
option level for SUPPORT_START_OVER is explicitly set to REGULAR. Shouldn't it 
be ADVANCED?



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I75720d0d454527e1a0ed19bb43cf9e4f018ce1d1
Gerrit-Change-Number: 8447
Gerrit-PatchSet: 2
Gerrit-Owner: Gabor Kaszab 
Gerrit-Reviewer: Attila Jeges 
Gerrit-Reviewer: Csaba Ringhofer 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Gabor Kaszab 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Laszlo Gaal 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Reviewer: Zoltan Borok-Nagy 
Gerrit-Comment-Date: Fri, 03 Nov 2017 11:20:08 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-2181: Add query option levels for display

2017-11-02 Thread Gabor Kaszab (Code Review)

Gabor Kaszab has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8447 )

Change subject: IMPALA-2181: Add query option levels for display
..


Patch Set 5:

(13 comments)

http://gerrit.cloudera.org:8080/#/c/8447/2/be/src/service/impala-server.h
File be/src/service/impala-server.h:

http://gerrit.cloudera.org:8080/#/c/8447/2/be/src/service/impala-server.h@520
PS2, Line 520:  void AddOptionLevelToConfigDescription(beeswax::ConfigVariable& 
option,
 :   const string& option_key) const;
> Remove 'default_query_options_' and make AddOptionLevelToConfigDescription(
Done


http://gerrit.cloudera.org:8080/#/c/8447/2/be/src/service/impala-server.cc
File be/src/service/impala-server.cc:

http://gerrit.cloudera.org:8080/#/c/8447/2/be/src/service/impala-server.cc@1221
PS2, Line 1221:
> Why pass in 'default_query_options_'? Isn't it already visible in AddOption
Done


http://gerrit.cloudera.org:8080/#/c/8447/2/be/src/service/impala-server.cc@1224
PS2, Line 1224: }
  : }
  :
  : void ImpalaServer::AddOptionLevelToConfigDescript
> Shouldn't you call AddOptionLevelToConfigDescription() with 'support_start_
As far as I know this support_start_over options is deliberately not added to 
the list of options in query-options.h because we don't want the same 
functionality to work on it like for the others.

Here I can do something like string_map["support_start_over"]=false before the 
for loop without any problems I think. Let me give it a try.


http://gerrit.cloudera.org:8080/#/c/8447/2/shell/impala_client.py
File shell/impala_client.py:

http://gerrit.cloudera.org:8080/#/c/8447/2/shell/impala_client.py@106
PS2, Line 106: != 2 o
> It is safer to use "!=" here.
Done


http://gerrit.cloudera.org:8080/#/c/8447/2/shell/impala_client.py@107
PS2, Line 107: return ""
> nit: maybe we should be more defensive and return "" as well if parsed_desc
Done


http://gerrit.cloudera.org:8080/#/c/8447/2/shell/impala_shell.py
File shell/impala_shell.py:

http://gerrit.cloudera.org:8080/#/c/8447/2/shell/impala_shell.py@127
PS2, Line 127:   PRINT_REGULAR_OPTIONS_ONLY = 1
 :   PRINT_ALL_OPTIONS = 2
> nit: why not use a bool flag instead?
I found it more verbose to have 2 const instead of a bool.
e.g.1: having a PRINT_ALL_OPTIONS=false doesn't tell us what we print just that 
not all the options.
e.g.2: PRINT_REGULAR_OPTIONS_ONLY=false has the same issue as below.

If you don't agree with me then I'm fine to turn these into a single bool.


http://gerrit.cloudera.org:8080/#/c/8447/2/shell/impala_shell.py@225
PS2, Line 225: =PR
> nit: remove spaces around '='
Done


http://gerrit.cloudera.org:8080/#/c/8447/2/shell/impala_shell.py@250
PS2, Line 250: then
> 'then'?
Done


http://gerrit.cloudera.org:8080/#/c/8447/2/shell/impala_shell.py@252
PS2, Line 252: for option_name, option_value in default_options.iteritems():
> This could be "for option_name, option_value in default_options.items():"
Done


http://gerrit.cloudera.org:8080/#/c/8447/2/shell/impala_shell.py@255
PS2, Line 255: continue
> I do not understand the meaning of this return.
Nice catch, thx!
Done


http://gerrit.cloudera.org:8080/#/c/8447/2/shell/impala_shell.py@266
PS2, Line 266: options, set_options):
> Why not use 'option_value'?
Done


http://gerrit.cloudera.org:8080/#/c/8447/2/shell/impala_shell.py@263
PS2, Line 263: advanced_options[option_name] = option_value
 : return (regular_options, advanced_options, 
deprecated_options)
 :
 :   def _print_option_group(self, default_options, set_options):
> How about this instead?
yeah, I wanted to do some extra handling for the else, like writing some error 
log or such, but the decided not to do so.
Thx!


http://gerrit.cloudera.org:8080/#/c/8447/2/tests/shell/test_shell_interactive.py
File tests/shell/test_shell_interactive.py:

http://gerrit.cloudera.org:8080/#/c/8447/2/tests/shell/test_shell_interactive.py@351
PS2, Line 351:result = shell1.get_result()
 : assert "Query options (defaults shown in []):" in 
result.stdout
 : assert "ABORT_ON_ERROR" in result.stdout
> SUPPORT_START_OVER should appear here in result.stdout, right?
No. SUPPORT_START_OVER as I discussed with Tim is a tricky one and we don't 
really like when the user alters it's value as it might result some queries to 
fail.
So we decided to push it to the 'ADVANCED' group so it will only be displayed 
with 'SET ALL'



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I75720d0d454527e1a0ed19bb43cf9e4f018ce1d1
Gerrit-Change-Number: 8447
Gerrit-PatchSet: 5
Gerrit-Owner: Gabor Kaszab 
Gerrit-Reviewer: Attila Jeges

[Impala-ASF-CR] IMPALA-2181: Add query option levels for display

2017-11-02 Thread Gabor Kaszab (Code Review)

Hello Lars Volker, Laszlo Gaal, Zoltan Borok-Nagy, Attila Jeges, Tim Armstrong, 
Csaba Ringhofer, Dan Hecht,

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

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

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

Change subject: IMPALA-2181: Add query option levels for display
..

IMPALA-2181: Add query option levels for display

Three display levels are introduced for each query option: REGULAR, ADVANCED
and DEPRECATED. When the query options are diplayed in Impala shell using
SET then only the REGULAR options are shown. A new command called SET ALL
shows all the options grouped by their option levels.

Change-Id: I75720d0d454527e1a0ed19bb43cf9e4f018ce1d1
---
M be/src/service/child-query.cc
M be/src/service/impala-server.cc
M be/src/service/impala-server.h
M be/src/service/query-options.cc
M be/src/service/query-options.h
M common/thrift/ImpalaInternalService.thrift
M shell/impala_client.py
M shell/impala_shell.py
M tests/shell/test_shell_interactive.py
9 files changed, 243 insertions(+), 88 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/47/8447/5
--
To view, visit http://gerrit.cloudera.org:8080/8447
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I75720d0d454527e1a0ed19bb43cf9e4f018ce1d1
Gerrit-Change-Number: 8447
Gerrit-PatchSet: 5
Gerrit-Owner: Gabor Kaszab 
Gerrit-Reviewer: Attila Jeges 
Gerrit-Reviewer: Csaba Ringhofer 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Laszlo Gaal 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Reviewer: Zoltan Borok-Nagy

[Impala-ASF-CR] IMPALA-2181: Add query option levels for display

2017-11-02 Thread Gabor Kaszab (Code Review)

Hello Lars Volker, Laszlo Gaal, Zoltan Borok-Nagy, Attila Jeges, Tim Armstrong, 
Csaba Ringhofer, Dan Hecht,

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

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

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

Change subject: IMPALA-2181: Add query option levels for display
..

IMPALA-2181: Add query option levels for display

Three display levels are introduced for each query option: REGULAR, ADVANCED
and DEPRECATED. When the query options are diplayed in Impala shell using
SET then only the REGULAR options are shown. A new command called SET ALL
shows all the options grouped by their option levels.

Change-Id: I75720d0d454527e1a0ed19bb43cf9e4f018ce1d1
---
M be/src/service/child-query.cc
M be/src/service/impala-server.cc
M be/src/service/impala-server.h
M be/src/service/query-options.cc
M be/src/service/query-options.h
M common/thrift/ImpalaInternalService.thrift
M shell/impala_client.py
M shell/impala_shell.py
M tests/shell/test_shell_interactive.py
9 files changed, 245 insertions(+), 88 deletions(-)


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I75720d0d454527e1a0ed19bb43cf9e4f018ce1d1
Gerrit-Change-Number: 8447
Gerrit-PatchSet: 4
Gerrit-Owner: Gabor Kaszab 
Gerrit-Reviewer: Attila Jeges 
Gerrit-Reviewer: Csaba Ringhofer 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Laszlo Gaal 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Reviewer: Zoltan Borok-Nagy

[Impala-ASF-CR] IMPALA-2181: Add query option levels for display

2017-11-02 Thread Gabor Kaszab (Code Review)

Hello Lars Volker, Laszlo Gaal, Zoltan Borok-Nagy, Attila Jeges, Tim Armstrong, 
Csaba Ringhofer, Dan Hecht,

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

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

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

Change subject: IMPALA-2181: Add query option levels for display
..

IMPALA-2181: Add query option levels for display

Three display levels are introduced for each query option: REGULAR, ADVANCED
and DEPRECATED. When the query options are diplayed in Impala shell using
SET then only the REGULAR options are shown. A new command called SET ALL
shows all the options grouped by their option levels.

Change-Id: I75720d0d454527e1a0ed19bb43cf9e4f018ce1d1
---
M be/src/service/child-query.cc
M be/src/service/impala-server.cc
M be/src/service/impala-server.h
M be/src/service/query-options.cc
M be/src/service/query-options.h
M common/thrift/ImpalaInternalService.thrift
M shell/impala_client.py
M shell/impala_shell.py
M tests/shell/test_shell_interactive.py
9 files changed, 245 insertions(+), 88 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/47/8447/3
--
To view, visit http://gerrit.cloudera.org:8080/8447
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I75720d0d454527e1a0ed19bb43cf9e4f018ce1d1
Gerrit-Change-Number: 8447
Gerrit-PatchSet: 3
Gerrit-Owner: Gabor Kaszab 
Gerrit-Reviewer: Attila Jeges 
Gerrit-Reviewer: Csaba Ringhofer 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Laszlo Gaal 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Reviewer: Zoltan Borok-Nagy

[Impala-ASF-CR] IMPALA-2181: Add query option levels for display

2017-11-02 Thread Csaba Ringhofer (Code Review)

Csaba Ringhofer has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8447 )

Change subject: IMPALA-2181: Add query option levels for display
..


Patch Set 2:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/8447/2/shell/impala_shell.py
File shell/impala_shell.py:

http://gerrit.cloudera.org:8080/#/c/8447/2/shell/impala_shell.py@252
PS2, Line 252: for option_name in default_options:
This could be "for option_name, option_value in default_options.items():"


http://gerrit.cloudera.org:8080/#/c/8447/2/shell/impala_shell.py@255
PS2, Line 255: return (regular_options, advanced_options, 
deprecated_options)
I do not understand the meaning of this return.
What is the goal of this block? If it is to add options with unknown level to 
advanced, then you could use the get function with default value:
level = query_option_levels(option_name, self.QUERY_OPTION_ADVANCED)



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I75720d0d454527e1a0ed19bb43cf9e4f018ce1d1
Gerrit-Change-Number: 8447
Gerrit-PatchSet: 2
Gerrit-Owner: Gabor Kaszab 
Gerrit-Reviewer: Attila Jeges 
Gerrit-Reviewer: Csaba Ringhofer 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Laszlo Gaal 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Reviewer: Zoltan Borok-Nagy 
Gerrit-Comment-Date: Thu, 02 Nov 2017 18:38:03 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-2181: Add query option levels for display

2017-11-02 Thread Attila Jeges (Code Review)

Attila Jeges has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8447 )

Change subject: IMPALA-2181: Add query option levels for display
..


Patch Set 2:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/8447/2/be/src/service/impala-server.h
File be/src/service/impala-server.h:

http://gerrit.cloudera.org:8080/#/c/8447/2/be/src/service/impala-server.h@520
PS2, Line 520:  void AddOptionLevelToConfigDescription(beeswax::ConfigVariable& 
option,
 :   const TQueryOptions& default_query_options_, const string& 
option_key);
Remove 'default_query_options_' and make AddOptionLevelToConfigDescription() 
const.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I75720d0d454527e1a0ed19bb43cf9e4f018ce1d1
Gerrit-Change-Number: 8447
Gerrit-PatchSet: 2
Gerrit-Owner: Gabor Kaszab 
Gerrit-Reviewer: Attila Jeges 
Gerrit-Reviewer: Csaba Ringhofer 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Laszlo Gaal 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Reviewer: Zoltan Borok-Nagy 
Gerrit-Comment-Date: Thu, 02 Nov 2017 18:12:44 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-2181: Add query option levels for display

2017-11-02 Thread Attila Jeges (Code Review)

Attila Jeges has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8447 )

Change subject: IMPALA-2181: Add query option levels for display
..


Patch Set 2:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/8447/2/tests/shell/test_shell_interactive.py
File tests/shell/test_shell_interactive.py:

http://gerrit.cloudera.org:8080/#/c/8447/2/tests/shell/test_shell_interactive.py@351
PS2, Line 351:result = shell1.get_result()
 : assert "Query options (defaults shown in []):" in 
result.stdout
 : assert "ABORT_ON_ERROR" in result.stdout
SUPPORT_START_OVER should appear here in result.stdout, right?



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I75720d0d454527e1a0ed19bb43cf9e4f018ce1d1
Gerrit-Change-Number: 8447
Gerrit-PatchSet: 2
Gerrit-Owner: Gabor Kaszab 
Gerrit-Reviewer: Attila Jeges 
Gerrit-Reviewer: Csaba Ringhofer 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Laszlo Gaal 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Reviewer: Zoltan Borok-Nagy 
Gerrit-Comment-Date: Thu, 02 Nov 2017 18:01:26 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-2181: Add query option levels for display

2017-11-02 Thread Attila Jeges (Code Review)

Attila Jeges has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8447 )

Change subject: IMPALA-2181: Add query option levels for display
..


Patch Set 2:

(9 comments)

http://gerrit.cloudera.org:8080/#/c/8447/2/be/src/service/impala-server.cc
File be/src/service/impala-server.cc:

http://gerrit.cloudera.org:8080/#/c/8447/2/be/src/service/impala-server.cc@1221
PS2, Line 1221: default_query_options_
Why pass in 'default_query_options_'? Isn't it already visible in 
AddOptionLevelToConfigDescription()?


http://gerrit.cloudera.org:8080/#/c/8447/2/be/src/service/impala-server.cc@1224
PS2, Line 1224: ConfigVariable support_start_over;
  :   support_start_over.__set_key("support_start_over");
  :   support_start_over.__set_value("false");
  :   default_configs_.push_back(support_start_over);
Shouldn't you call AddOptionLevelToConfigDescription() with 
'support_start_over' as well?


http://gerrit.cloudera.org:8080/#/c/8447/2/shell/impala_client.py
File shell/impala_client.py:

http://gerrit.cloudera.org:8080/#/c/8447/2/shell/impala_client.py@106
PS2, Line 106: is not
It is safer to use "!=" here.

"is not" works for small integers with the CPython interpreter but it is 
implementation specific behavior and might be changed in the future.


http://gerrit.cloudera.org:8080/#/c/8447/2/shell/impala_client.py@107
PS2, Line 107: return ""
nit: maybe we should be more defensive and return "" as well if 
parsed_description[0] != "OptionLevel".


http://gerrit.cloudera.org:8080/#/c/8447/2/shell/impala_shell.py
File shell/impala_shell.py:

http://gerrit.cloudera.org:8080/#/c/8447/2/shell/impala_shell.py@127
PS2, Line 127:   PRINT_REGULAR_OPTIONS_ONLY = 1
 :   PRINT_ALL_OPTIONS = 2
nit: why not use a bool flag instead?


http://gerrit.cloudera.org:8080/#/c/8447/2/shell/impala_shell.py@225
PS2, Line 225:  =
nit: remove spaces around '='


http://gerrit.cloudera.org:8080/#/c/8447/2/shell/impala_shell.py@250
PS2, Line 250: that
'then'?


http://gerrit.cloudera.org:8080/#/c/8447/2/shell/impala_shell.py@266
PS2, Line 266: default_options[option_name]
Why not use 'option_value'?


http://gerrit.cloudera.org:8080/#/c/8447/2/shell/impala_shell.py@263
PS2, Line 263:   elif level == self.QUERY_OPTION_ADVANCED:
 : advanced_options[option_name] = option_value
 :   else:
 : advanced_options[option_name] = 
default_options[option_name]
How about this instead?

  else:
advanced_options[option_name] = option_value



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I75720d0d454527e1a0ed19bb43cf9e4f018ce1d1
Gerrit-Change-Number: 8447
Gerrit-PatchSet: 2
Gerrit-Owner: Gabor Kaszab 
Gerrit-Reviewer: Attila Jeges 
Gerrit-Reviewer: Csaba Ringhofer 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Laszlo Gaal 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Reviewer: Zoltan Borok-Nagy 
Gerrit-Comment-Date: Thu, 02 Nov 2017 17:32:23 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-2181: Add query option levels for display

2017-11-01 Thread Gabor Kaszab (Code Review)

Hello Lars Volker, Laszlo Gaal, Zoltan Borok-Nagy, Attila Jeges, Tim Armstrong, 
Csaba Ringhofer, Dan Hecht,

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

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

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

Change subject: IMPALA-2181: Add query option levels for display
..

IMPALA-2181: Add query option levels for display

Three display levels are introduced for each query option: REGULAR, ADVANCED
and DEPRECATED. When the query options are diplayed in Impala shell using
SET then only the REGULAR options are shown. A new command called SET ALL
shows all the options grouped by their option levels.

Change-Id: I75720d0d454527e1a0ed19bb43cf9e4f018ce1d1
---
M be/src/service/child-query.cc
M be/src/service/impala-server.cc
M be/src/service/impala-server.h
M be/src/service/query-options.cc
M be/src/service/query-options.h
M common/thrift/ImpalaInternalService.thrift
M shell/impala_client.py
M shell/impala_shell.py
M tests/shell/test_shell_interactive.py
9 files changed, 242 insertions(+), 84 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/47/8447/2
--
To view, visit http://gerrit.cloudera.org:8080/8447
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I75720d0d454527e1a0ed19bb43cf9e4f018ce1d1
Gerrit-Change-Number: 8447
Gerrit-PatchSet: 2
Gerrit-Owner: Gabor Kaszab 
Gerrit-Reviewer: Attila Jeges 
Gerrit-Reviewer: Csaba Ringhofer 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Laszlo Gaal 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Reviewer: Zoltan Borok-Nagy

[Impala-ASF-CR] IMPALA-2181: Add query option levels for display

2017-11-01 Thread Gabor Kaszab (Code Review)

Gabor Kaszab has uploaded this change for review. ( 
http://gerrit.cloudera.org:8080/8447


Change subject: IMPALA-2181: Add query option levels for display
..

IMPALA-2181: Add query option levels for display

Three display levels are introduced for each query option: REGULAR, ADVANCED
and DEPRECATED. When the query options are diplayed in Impala shell using
SET then only the REGULAR options are shown. A new command called SET ALL
shows all the options grouped by their option levels.

Change-Id: I75720d0d454527e1a0ed19bb43cf9e4f018ce1d1
---
M be/src/service/child-query.cc
M be/src/service/impala-server.cc
M be/src/service/impala-server.h
M be/src/service/query-options.cc
M be/src/service/query-options.h
M common/thrift/ImpalaInternalService.thrift
M shell/impala_client.py
M shell/impala_shell.py
M tests/shell/test_shell_interactive.py
9 files changed, 244 insertions(+), 84 deletions(-)



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: I75720d0d454527e1a0ed19bb43cf9e4f018ce1d1
Gerrit-Change-Number: 8447
Gerrit-PatchSet: 1
Gerrit-Owner: Gabor Kaszab

[Impala-ASF-CR] IMPALA-2181: Add query option levels for display

[Impala-ASF-CR] IMPALA-2181: Add query option levels for display

[Impala-ASF-CR] IMPALA-2181: Add query option levels for display

[Impala-ASF-CR] IMPALA-2181: Add query option levels for display

[Impala-ASF-CR] IMPALA-2181: Add query option levels for display

[Impala-ASF-CR] IMPALA-2181: Add query option levels for display

[Impala-ASF-CR] IMPALA-2181: Add query option levels for display

[Impala-ASF-CR] IMPALA-2181: Add query option levels for display

[Impala-ASF-CR] IMPALA-2181: Add query option levels for display

[Impala-ASF-CR] IMPALA-2181: Add query option levels for display

[Impala-ASF-CR] IMPALA-2181: Add query option levels for display

[Impala-ASF-CR] IMPALA-2181: Add query option levels for display

[Impala-ASF-CR] IMPALA-2181: Add query option levels for display

[Impala-ASF-CR] IMPALA-2181: Add query option levels for display

[Impala-ASF-CR] IMPALA-2181: Add query option levels for display

[Impala-ASF-CR] IMPALA-2181: Add query option levels for display

[Impala-ASF-CR] IMPALA-2181: Add query option levels for display

[Impala-ASF-CR] IMPALA-2181: Add query option levels for display

[Impala-ASF-CR] IMPALA-2181: Add query option levels for display

[Impala-ASF-CR] IMPALA-2181: Add query option levels for display

[Impala-ASF-CR] IMPALA-2181: Add query option levels for display

[Impala-ASF-CR] IMPALA-2181: Add query option levels for display

[Impala-ASF-CR] IMPALA-2181: Add query option levels for display

[Impala-ASF-CR] IMPALA-2181: Add query option levels for display

[Impala-ASF-CR] IMPALA-2181: Add query option levels for display

[Impala-ASF-CR] IMPALA-2181: Add query option levels for display

[Impala-ASF-CR] IMPALA-2181: Add query option levels for display

[Impala-ASF-CR] IMPALA-2181: Add query option levels for display

[Impala-ASF-CR] IMPALA-2181: Add query option levels for display

[Impala-ASF-CR] IMPALA-2181: Add query option levels for display

[Impala-ASF-CR] IMPALA-2181: Add query option levels for display

[Impala-ASF-CR] IMPALA-2181: Add query option levels for display

[Impala-ASF-CR] IMPALA-2181: Add query option levels for display

33 matches

Site Navigation

Mail list logo

Footer information