[Impala-ASF-CR] IMPALA-2181: Add query option levels for display
Impala Public Jenkins has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/8447 ) 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 displayed 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. When the query options are displayed through the SET SQL statement then the result set would contain an extra column indicating the level of each option. Similarly to Impala shell here the SET command only diplays the REGULAR and ADVANCED options while SET ALL shows them all. If the Impala shell connects to an Impala daemon that predates this change then all the options would be displayed in the REGULAR group. Change-Id: I75720d0d454527e1a0ed19bb43cf9e4f018ce1d1 Reviewed-on: http://gerrit.cloudera.org:8080/8447 Reviewed-by: Dan Hecht Tested-by: Impala Public Jenkins --- 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/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 18 files changed, 486 insertions(+), 226 deletions(-) Approvals: Dan Hecht: Looks good to me, approved Impala Public Jenkins: Verified -- 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: merged Gerrit-Change-Id: I75720d0d454527e1a0ed19bb43cf9e4f018ce1d1 Gerrit-Change-Number: 8447 Gerrit-PatchSet: 24 Gerrit-Owner: Gabor Kaszab Gerrit-Reviewer: Attila Jeges Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Dan Hecht Gerrit-Reviewer: Gabor Kaszab Gerrit-Reviewer: Impala Public Jenkins 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
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/8447 ) Change subject: IMPALA-2181: Add query option levels for display .. Patch Set 23: Verified+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: comment Gerrit-Change-Id: I75720d0d454527e1a0ed19bb43cf9e4f018ce1d1 Gerrit-Change-Number: 8447 Gerrit-PatchSet: 23 Gerrit-Owner: Gabor Kaszab Gerrit-Reviewer: Attila Jeges Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Dan Hecht Gerrit-Reviewer: Gabor Kaszab Gerrit-Reviewer: Impala Public Jenkins 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, 28 Nov 2017 00:31:14 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-2181: Add query option levels for display
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 22: Code-Review+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: comment Gerrit-Change-Id: I75720d0d454527e1a0ed19bb43cf9e4f018ce1d1 Gerrit-Change-Number: 8447 Gerrit-PatchSet: 22 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: Mon, 27 Nov 2017 20:59:54 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-2181: Add query option levels for display
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/8447 ) Change subject: IMPALA-2181: Add query option levels for display .. Patch Set 23: Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/1527/ -- 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: 23 Gerrit-Owner: Gabor Kaszab Gerrit-Reviewer: Attila Jeges Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Dan Hecht Gerrit-Reviewer: Gabor Kaszab Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Lars Volker Gerrit-Reviewer: Laszlo Gaal Gerrit-Reviewer: Philip Zeyliger Gerrit-Reviewer: Tim Armstrong Gerrit-Reviewer: Zoltan Borok-Nagy Gerrit-Comment-Date: Mon, 27 Nov 2017 21:00:07 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-2181: Add query option levels for display
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 23: Code-Review+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: comment Gerrit-Change-Id: I75720d0d454527e1a0ed19bb43cf9e4f018ce1d1 Gerrit-Change-Number: 8447 Gerrit-PatchSet: 23 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: Mon, 27 Nov 2017 21:00:01 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-2181: Add query option levels for display
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 22: (2 comments) Thanks Dan for taking a look! http://gerrit.cloudera.org:8080/#/c/8447/21/be/src/service/impala-server.h File be/src/service/impala-server.h: http://gerrit.cloudera.org:8080/#/c/8447/21/be/src/service/impala-server.h@543 PS21, Line 543: _ > missing _ Done http://gerrit.cloudera.org:8080/#/c/8447/21/be/src/service/impala-server.cc File be/src/service/impala-server.cc: http://gerrit.cloudera.org:8080/#/c/8447/21/be/src/service/impala-server.cc@1235 PS21, Line 1235: } > okay to ignore, but consider removing this subroutine now that it's so simp I prefer to keep this function to make the config population part of InitializeConfigVariables() as prompt and self-descriptive as possible. -- 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: 22 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: Mon, 27 Nov 2017 20:07:05 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-2181: Add query option levels for display
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 (#22). 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 displayed 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. When the query options are displayed through the SET SQL statement then the result set would contain an extra column indicating the level of each option. Similarly to Impala shell here the SET command only diplays the REGULAR and ADVANCED options while SET ALL shows them all. If the Impala shell connects to an Impala daemon that predates this change then all the options would be displayed in the REGULAR group. 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/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 18 files changed, 486 insertions(+), 226 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/47/8447/22 -- 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: 22 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
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 21: Code-Review+2 (2 comments) http://gerrit.cloudera.org:8080/#/c/8447/21/be/src/service/impala-server.h File be/src/service/impala-server.h: http://gerrit.cloudera.org:8080/#/c/8447/21/be/src/service/impala-server.h@543 PS21, Line 543: ' missing _ http://gerrit.cloudera.org:8080/#/c/8447/21/be/src/service/impala-server.cc File be/src/service/impala-server.cc: http://gerrit.cloudera.org:8080/#/c/8447/21/be/src/service/impala-server.cc@1235 PS21, Line 1235: } okay to ignore, but consider removing this subroutine now that it's so simple and just put the code inside InitializeConfigVariables(). -- 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: 21 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: Mon, 27 Nov 2017 18:39:17 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-2181: Add query option levels for display
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 21: (8 comments) http://gerrit.cloudera.org:8080/#/c/8447/20//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/8447/20//COMMIT_MSG@14 PS20, Line 14: the SET SQL s > I think that should say "the SET SQL statement" or similar, since SET can Done http://gerrit.cloudera.org:8080/#/c/8447/20/be/src/service/impala-server.h File be/src/service/impala-server.h: http://gerrit.cloudera.org:8080/#/c/8447/20/be/src/service/impala-server.h@541 PS20, Line 541: ets the optio > nit: delete (Obvious that the comment is talking about this function). Done http://gerrit.cloudera.org:8080/#/c/8447/20/be/src/service/impala-server.h@544 PS20, Line 544: query option. > if that's modified, you should use ConfigVariable *. We prefer to use poin Done http://gerrit.cloudera.org:8080/#/c/8447/20/be/src/service/impala-server.h@545 PS20, Line 545: lToConfig( > that's not explained. and is it even needed -- why not just use the key fro I didn't want to make this function dependent on whether the key field of the ConfigVariable was set by the caller or not. I found it safer this way. I'll mention this in the comment. http://gerrit.cloudera.org:8080/#/c/8447/20/be/src/service/impala-server.cc File be/src/service/impala-server.cc: http://gerrit.cloudera.org:8080/#/c/8447/20/be/src/service/impala-server.cc@1236 PS20, Line 1236: > does query_option_levels_ not contain all keys? i.e. should line 1233 be a In fact, a DCHECK is enough here. (Changing this to a DCHECK actually caught a bug: Where support_start_over was added to the options map the name should have been added with uppercase letters.) http://gerrit.cloudera.org:8080/#/c/8447/20/be/src/service/query-options.h File be/src/service/query-options.h: http://gerrit.cloudera.org:8080/#/c/8447/20/be/src/service/query-options.h@34 PS20, Line 34: Maps query > comments shouldn't talk about current code vs. older code. They should just Done http://gerrit.cloudera.org:8080/#/c/8447/20/be/src/service/query-options.h@175 PS20, Line 175: QueryOptionLevels* > use pointer Done http://gerrit.cloudera.org:8080/#/c/8447/20/common/thrift/beeswax.thrift File common/thrift/beeswax.thrift: http://gerrit.cloudera.org:8080/#/c/8447/20/common/thrift/beeswax.thrift@98 PS20, Line 98: > maybe we should be super explicit here and say: 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: 21 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, 23 Nov 2017 05:56:49 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-2181: Add query option levels for display
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 (#21). 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 displayed 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. When the query options are displayed through the SET SQL statement then the result set would contain an extra column indicating the level of each option. Similarly to Impala shell here the SET command only diplays the REGULAR and ADVANCED options while SET ALL shows them all. If the Impala shell connects to an Impala daemon that predates this change then all the options would be displayed in the REGULAR group. 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/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 18 files changed, 486 insertions(+), 226 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/47/8447/21 -- 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: 21 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
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 20: (2 comments) (Also didn't look too much at the python shell code.) http://gerrit.cloudera.org:8080/#/c/8447/20//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/8447/20//COMMIT_MSG@14 PS20, Line 14: HS2 interface I think that should say "the SET SQL statement" or similar, since SET can be executed via HS2 or beeswax. It's just that the shell doesn't use it today. http://gerrit.cloudera.org:8080/#/c/8447/20/common/thrift/beeswax.thrift File common/thrift/beeswax.thrift: http://gerrit.cloudera.org:8080/#/c/8447/20/common/thrift/beeswax.thrift@98 PS20, Line 98: maybe we should be super explicit here and say: // Impala extension: ... to point out that this isn't part of the "standard" beeswax interface. I don't think anything else in this file was customized for Impala besides this. (The customization usually happens in ImpalaService.thrift. We could even add this functionality there instead, but I don't feel strongly given that this seems like a backwards compatible modification and beeswax will go away. -- 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: 20 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, 23 Nov 2017 01:03:55 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-2181: Add query option levels for display
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 20: (6 comments) Looks good to me, though I didn't look at the tests very carefully myself. Just some minor comments. http://gerrit.cloudera.org:8080/#/c/8447/20/be/src/service/impala-server.h File be/src/service/impala-server.h: http://gerrit.cloudera.org:8080/#/c/8447/20/be/src/service/impala-server.h@541 PS20, Line 541: his function nit: delete (Obvious that the comment is talking about this function). http://gerrit.cloudera.org:8080/#/c/8447/20/be/src/service/impala-server.h@544 PS20, Line 544: beeswax::ConfigVariable& option if that's modified, you should use ConfigVariable *. We prefer to use pointers rather than references for things that get modified, so that it's clear at the callsite. (i.e. only use const references). http://gerrit.cloudera.org:8080/#/c/8447/20/be/src/service/impala-server.h@545 PS20, Line 545: option_key that's not explained. and is it even needed -- why not just use the key from 'option'? http://gerrit.cloudera.org:8080/#/c/8447/20/be/src/service/impala-server.cc File be/src/service/impala-server.cc: http://gerrit.cloudera.org:8080/#/c/8447/20/be/src/service/impala-server.cc@1236 PS20, Line 1236: config.__set_level(TQueryOptionLevel::ADVANCED); does query_option_levels_ not contain all keys? i.e. should line 1233 be a DCHECK? http://gerrit.cloudera.org:8080/#/c/8447/20/be/src/service/query-options.h File be/src/service/query-options.h: http://gerrit.cloudera.org:8080/#/c/8447/20/be/src/service/query-options.h@34 PS20, Line 34: Introduced comments shouldn't talk about current code vs. older code. They should just talk about the code. So, reword this: Maps from query option name to option level used ... or similar http://gerrit.cloudera.org:8080/#/c/8447/20/be/src/service/query-options.h@175 PS20, Line 175: QueryOptionLevels& use pointer -- 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: 20 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, 23 Nov 2017 00:55:31 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-2181: Add query option levels for display
Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/8447 ) Change subject: IMPALA-2181: Add query option levels for display .. Patch Set 20: Code-Review+1 Looks good to me. Did anyone else on the long list of reviewers want to take another look? -- 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: 20 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, 23 Nov 2017 00:28:26 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-2181: Add query option levels for display
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 20: (1 comment) http://gerrit.cloudera.org:8080/#/c/8447/18/shell/impala_client.py File shell/impala_client.py: http://gerrit.cloudera.org:8080/#/c/8447/18/shell/impala_client.py@100 PS18, Line 100: option.level is not None: > in theory, if the shell with the feature comes from an Impala that is compi 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: 20 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, 22 Nov 2017 20:08:00 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-2181: Add query option levels for display
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 (#20). 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 displayed 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. When the query options are displayed through HS2 interface then the result set would contain an extra column indicating the level of each option. Similarly to Impala shell on this interface the SET command only diplays the REGULAR and ADVANCED options while SET ALL shows them all. If the Impala shell connects to an Impala daemon that predates this change then all the options would be displayed in the REGULAR group. 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/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 18 files changed, 487 insertions(+), 226 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/47/8447/20 -- 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: 20 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
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 19: (4 comments) http://gerrit.cloudera.org:8080/#/c/8447/18/be/src/service/query-options.h File be/src/service/query-options.h: http://gerrit.cloudera.org:8080/#/c/8447/18/be/src/service/query-options.h@113 PS18, Line 113: QUERY_OPT_FN(strict_mode, STRICT_MODE, TQueryOptionLevel::DEVELOPMENT)\ > per discussion on the JIRA, I think this should be under DEVELOPMENT until Done http://gerrit.cloudera.org:8080/#/c/8447/18/shell/impala_client.py File shell/impala_client.py: http://gerrit.cloudera.org:8080/#/c/8447/18/shell/impala_client.py@100 PS18, Line 100: hasattr(option, 'level') and > This check shouldn't be necessary, right? Unless you're doing what I did an in theory, if the shell with the feature comes from an Impala that is compiled than this is not necessary. I would still keep it be make 100% sure, that nothing breaks if you don't mind. http://gerrit.cloudera.org:8080/#/c/8447/18/shell/impala_shell.py File shell/impala_shell.py: http://gerrit.cloudera.org:8080/#/c/8447/18/shell/impala_shell.py@83 PS18, Line 83: correspond > correspond Done http://gerrit.cloudera.org:8080/#/c/8447/18/shell/impala_shell.py@242 PS18, Line 242: # If the shell is connected to an Impala that predates IMPALA-2181 then > This is subtle - can you add a comment? If I understand correctly, this is 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: 19 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, 22 Nov 2017 19:45:46 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-2181: Add query option levels for display
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 (#19). 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 displayed 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. When the query options are displayed through HS2 interface then the result set would contain an extra column indicating the level of each option. Similarly to Impala shell on this interface the SET command only diplays the REGULAR and ADVANCED options while SET ALL shows them all. If the Impala shell connects to an Impala daemon that predates this change then all the options would be displayed in the REGULAR group. 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/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 18 files changed, 487 insertions(+), 226 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/47/8447/19 -- 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: 19 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
Tim Armstrong 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: (5 comments) http://gerrit.cloudera.org:8080/#/c/8447/18/be/src/service/query-options.h File be/src/service/query-options.h: http://gerrit.cloudera.org:8080/#/c/8447/18/be/src/service/query-options.h@113 PS18, Line 113: per discussion on the JIRA, I think this should be under DEVELOPMENT until it's documented. http://gerrit.cloudera.org:8080/#/c/8447/18/shell/impala_client.py File shell/impala_client.py: http://gerrit.cloudera.org:8080/#/c/8447/18/shell/impala_client.py@100 PS18, Line 100: _indent_level, output): This check shouldn't be necessary, right? Unless you're doing what I did and running impala-shell against the wrong thrift output because you're too lazy to rebuild Impala :). http://gerrit.cloudera.org:8080/#/c/8447/18/shell/impala_shell.py File shell/impala_shell.py: http://gerrit.cloudera.org:8080/#/c/8447/18/shell/impala_shell.py@83 PS18, Line 83: correspond http://gerrit.cloudera.org:8080/#/c/8447/18/shell/impala_shell.py@242 PS18, Line 242: self._print_option_group(advanced_options) This is subtle - can you add a comment? If I understand correctly, this is detecting that the impala server didn't send back any advanced query options, so we're assuming it's an old one. http://gerrit.cloudera.org:8080/#/c/8447/14/tests/hs2/test_hs2.py File tests/hs2/test_hs2.py: http://gerrit.cloudera.org:8080/#/c/8447/14/tests/hs2/test_hs2.py@53 PS14, Line 53: fetch_results_req.operationHandle = execute_statement_resp.operationHandle > This function is actually a copy paste: I moved it out from test_session_op Seems ok then. -- 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, 22 Nov 2017 19:06:09 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-2181: Add query option levels for display
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 (#18). 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 displayed 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. When the query options are displayed through HS2 interface then the result set would contain an extra column indicating the level of each option. Similarly to Impala shell on this interface the SET command only diplays the REGULAR and ADVANCED options while SET ALL shows them all. If the Impala shell connects to an Impala daemon that predates this change then all the options would be displayed in the REGULAR group. 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/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 18 files changed, 484 insertions(+), 226 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/47/8447/18 -- 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: 18 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
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 17: (17 comments) http://gerrit.cloudera.org:8080/#/c/8447/14//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/8447/14//COMMIT_MSG@10 PS14, Line 10: displaye > displayed Done http://gerrit.cloudera.org:8080/#/c/8447/14//COMMIT_MSG@12 PS14, Line 12: command called SET ALL shows all the options grouped by their option levels. > Mention that there are also server-side versions of these commands? Done http://gerrit.cloudera.org:8080/#/c/8447/14/be/src/service/impala-server.cc File be/src/service/impala-server.cc: http://gerrit.cloudera.org:8080/#/c/8447/14/be/src/service/impala-server.cc@1236 PS14, Line 1236: config.__set_level(TQueryOptionLevel::ADVANCED); > Nit: Done http://gerrit.cloudera.org:8080/#/c/8447/15/be/src/service/query-options.h File be/src/service/query-options.h: http://gerrit.cloudera.org:8080/#/c/8447/15/be/src/service/query-options.h@45 PS15, Line 45: TQueryOptionLevel::DEPRECATED)\ > Looks like disable_cached_reads is deprecated: see IMPALA-2963 . I missed t Done http://gerrit.cloudera.org:8080/#/c/8447/15/be/src/service/query-options.h@89 PS15, Line 89: QUERY_OPT_FN(disable_streaming_preaggregations, DISABLE_STREAMING_PREAGGREGATIONS,\ > I think enable_expr_rewrites should be advanced. It's only disabled as a wo Done http://gerrit.cloudera.org:8080/#/c/8447/15/be/src/service/query-options.h@91 PS15, Line 91: QUERY_OPT_FN(runtime_filter_mode, RUNTIME_FILTER_MODE, TQueryOptionLevel::REGULAR)\ > parquet_dictionary_filtering should probably be advanced, there's typically Done http://gerrit.cloudera.org:8080/#/c/8447/15/be/src/service/query-options.h@93 PS15, Line 93: TQueryOptionLevel::ADVANCED)\ > Same for parquet_read_statistics. Done http://gerrit.cloudera.org:8080/#/c/8447/14/be/src/service/query-options.h File be/src/service/query-options.h: http://gerrit.cloudera.org:8080/#/c/8447/14/be/src/service/query-options.h@39 PS14, Line 39: // If the DCHECK is hit then handle the missing query option below and update > nit: can we wrap these to 90 chars? Done http://gerrit.cloudera.org:8080/#/c/8447/14/be/src/service/query-options.cc File be/src/service/query-options.cc: http://gerrit.cloudera.org:8080/#/c/8447/14/be/src/service/query-options.cc@41 PS14, Line 41: using namespace strings; > nit: probably best to individually import the names so it's easier to figur Done http://gerrit.cloudera.org:8080/#/c/8447/14/common/thrift/ImpalaService.thrift File common/thrift/ImpalaService.thrift: http://gerrit.cloudera.org:8080/#/c/8447/14/common/thrift/ImpalaService.thrift@294 PS14, Line 294: // TODO: Rename to reflect that this is for all DML. > Does this need to be a typedef here? I don't see any references in any thri sure. I'll move this to query-options.h http://gerrit.cloudera.org:8080/#/c/8447/14/common/thrift/beeswax.thrift File common/thrift/beeswax.thrift: http://gerrit.cloudera.org:8080/#/c/8447/14/common/thrift/beeswax.thrift@111 PS14, Line 111: // For displaying purposes in Impala shell > Comment that this was added for impala-shell? Done http://gerrit.cloudera.org:8080/#/c/8447/14/fe/src/main/java/org/apache/impala/analysis/SetStmt.java File fe/src/main/java/org/apache/impala/analysis/SetStmt.java: http://gerrit.cloudera.org:8080/#/c/8447/14/fe/src/main/java/org/apache/impala/analysis/SetStmt.java@55 PS14, Line 55: if (isSetAll_) return "SET ALL"; > Nit: could be one line Done http://gerrit.cloudera.org:8080/#/c/8447/14/fe/src/main/java/org/apache/impala/analysis/SetStmt.java@76 PS14, Line 76: } > nit: could be one line Done http://gerrit.cloudera.org:8080/#/c/8447/14/shell/impala_client.py File shell/impala_client.py: http://gerrit.cloudera.org:8080/#/c/8447/14/shell/impala_client.py@97 PS14, Line 97: # If connected to an Impala that predates IMPALA-2181 then the received options > Does this still work if we're talking to an older Impala server that doesn' Nice catch! Done http://gerrit.cloudera.org:8080/#/c/8447/14/shell/impala_shell.py File shell/impala_shell.py: http://gerrit.cloudera.org:8080/#/c/8447/14/shell/impala_shell.py@82 PS14, Line 82: """These are the levels used when displaying query options. > Can you add a pointer to the thrift definition. E.g. "These correspond to t Done http://gerrit.cloudera.org:8080/#/c/8447/14/tests/hs2/test_hs2.py File tests/hs2/test_hs2.py: http://gerrit.cloudera.org:8080/#/c/8447/14/tests/hs2/test_hs2.py@53 PS14, Line 53: fetch_results_req.operationHandle = execute_statement_resp.operationHandle > Not a big deal but we could pass in these fields as keyword args to the con This function is actually a copy paste: I moved it out from test_session_options_via_set so that I can re-use it. To be honest I'd leave it as
[Impala-ASF-CR] IMPALA-2181: Add query option levels for display
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 (#17). 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 displayed 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. When the query options are displayed through HS2 interface then the result set would contain an extra column indicating the level of each option. Similarly to Impala shell on this interface the SET command only diplays the REGULAR and ADVANCED options while SET ALL shows them all. If the Impala shell connects to an Impala daemon that predates this change then all the options would be displayed in the REGULAR group. 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/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 18 files changed, 483 insertions(+), 226 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/47/8447/17 -- 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: 17 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
Tim Armstrong has posted comments on this change. (
http://gerrit.cloudera.org:8080/8447 )
Change subject: IMPALA-2181: Add query option levels for display
..
Patch Set 15:
(17 comments)
http://gerrit.cloudera.org:8080/#/c/8447/14//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/8447/14//COMMIT_MSG@10
PS14, Line 10: diplayed
displayed
http://gerrit.cloudera.org:8080/#/c/8447/14//COMMIT_MSG@12
PS14, Line 12: called SET ALL shows all the options grouped by their option
levels.
Mention that there are also server-side versions of these commands?
http://gerrit.cloudera.org:8080/#/c/8447/14/be/src/service/impala-server.cc
File be/src/service/impala-server.cc:
http://gerrit.cloudera.org:8080/#/c/8447/14/be/src/service/impala-server.cc@1236
PS14, Line 1236: else {
Nit:
} else {
http://gerrit.cloudera.org:8080/#/c/8447/15/be/src/service/query-options.h
File be/src/service/query-options.h:
http://gerrit.cloudera.org:8080/#/c/8447/15/be/src/service/query-options.h@45
PS15, Line 45: QUERY_OPT_FN(disable_cached_reads, DISABLE_CACHED_READS,
TQueryOptionLevel::REGULAR)\
Looks like disable_cached_reads is deprecated: see IMPALA-2963 . I missed that
on an earlier pass
http://gerrit.cloudera.org:8080/#/c/8447/15/be/src/service/query-options.h@89
PS15, Line 89: QUERY_OPT_FN(enable_expr_rewrites, ENABLE_EXPR_REWRITES,
TQueryOptionLevel::REGULAR)\
I think enable_expr_rewrites should be advanced. It's only disabled as a
workaround to planner bugs.
http://gerrit.cloudera.org:8080/#/c/8447/15/be/src/service/query-options.h@91
PS15, Line 91: QUERY_OPT_FN(parquet_dictionary_filtering,
PARQUET_DICTIONARY_FILTERING, TQueryOptionLevel::REGULAR)\
parquet_dictionary_filtering should probably be advanced, there's typically not
much reason to disable it unless working around a bug.
http://gerrit.cloudera.org:8080/#/c/8447/15/be/src/service/query-options.h@93
PS15, Line 93: QUERY_OPT_FN(parquet_read_statistics, PARQUET_READ_STATISTICS,
TQueryOptionLevel::REGULAR)\
Same for parquet_read_statistics.
http://gerrit.cloudera.org:8080/#/c/8447/14/be/src/service/query-options.h
File be/src/service/query-options.h:
http://gerrit.cloudera.org:8080/#/c/8447/14/be/src/service/query-options.h@39
PS14, Line 39: QUERY_OPT_FN(abort_on_default_limit_exceeded,
ABORT_ON_DEFAULT_LIMIT_EXCEEDED, TQueryOptionLevel::DEPRECATED)\
nit: can we wrap these to 90 chars?
http://gerrit.cloudera.org:8080/#/c/8447/14/be/src/service/query-options.cc
File be/src/service/query-options.cc:
http://gerrit.cloudera.org:8080/#/c/8447/14/be/src/service/query-options.cc@41
PS14, Line 41: using namespace beeswax;
nit: probably best to individually import the names so it's easier to figure
out what got imported.
http://gerrit.cloudera.org:8080/#/c/8447/14/common/thrift/ImpalaService.thrift
File common/thrift/ImpalaService.thrift:
http://gerrit.cloudera.org:8080/#/c/8447/14/common/thrift/ImpalaService.thrift@294
PS14, Line 294: typedef map
TQueryOptionLevels
Does this need to be a typedef here? I don't see any references in any thrift
files. Maybe we can just use the map type directly where its needed in the
backend
http://gerrit.cloudera.org:8080/#/c/8447/14/common/thrift/beeswax.thrift
File common/thrift/beeswax.thrift:
http://gerrit.cloudera.org:8080/#/c/8447/14/common/thrift/beeswax.thrift@111
PS14, Line 111: 4: optional TQueryOptionLevel level
Comment that this was added for impala-shell?
http://gerrit.cloudera.org:8080/#/c/8447/14/fe/src/main/java/org/apache/impala/analysis/SetStmt.java
File fe/src/main/java/org/apache/impala/analysis/SetStmt.java:
http://gerrit.cloudera.org:8080/#/c/8447/14/fe/src/main/java/org/apache/impala/analysis/SetStmt.java@55
PS14, Line 55: if (isSetAll_) {
Nit: could be one line
if (isSetAll_) return ...;
http://gerrit.cloudera.org:8080/#/c/8447/14/fe/src/main/java/org/apache/impala/analysis/SetStmt.java@76
PS14, Line 76: request.setIs_set_all(true);
nit: could be one line
http://gerrit.cloudera.org:8080/#/c/8447/14/shell/impala_client.py
File shell/impala_client.py:
http://gerrit.cloudera.org:8080/#/c/8447/14/shell/impala_client.py@97
PS14, Line 97: self.query_option_levels[option.key.upper()] = option.level
Does this still work if we're talking to an older Impala server that doesn't
return the level?
Update: looks like it doesn't:
Traceback (most recent call last):
File "/home/tarmstrong/Impala/incubator-impala/shell/impala_shell.py", line 15
27, in
shell = ImpalaShell(options, query_options)
File "/home/tarmstrong/Impala/incubator-impala/shell/impala_shell.py", line 20
5, in __init__
self.do_connect(options.impalad)
File "/home/tarmstrong/Impala/incubator-impala/shell/impala_shell.py", line 73
1, in do_connect
self.imp_client.build_default_query_options_dict()
File "/home/tarmstrong/Impala/incubator-impala/shell/impala_client.py", line 9
7, in build_defaul
[Impala-ASF-CR] IMPALA-2181: Add query option levels for display
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: FYI, both the core and the exhaustive tests passed for this patch. -- 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: Mon, 20 Nov 2017 18:23:35 + Gerrit-HasComments: No
