Xiaoqing Gao has posted comments on this change. ( http://gerrit.cloudera.org:8080/18430 )
Change subject: IMPALA-11233: Unset all query option ...................................................................... Patch Set 11: (9 comments) http://gerrit.cloudera.org:8080/#/c/18430/8//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/18430/8//COMMIT_MSG@9 PS8, Line 9: options > nit: options. Done http://gerrit.cloudera.org:8080/#/c/18430/8//COMMIT_MSG@12 PS8, Line 12: effect > nit: effect? Done http://gerrit.cloudera.org:8080/#/c/18430/8//COMMIT_MSG@13 PS8, Line 13: recreating a ne > I think users can re-create a connection instead of restarting the impalad. Done http://gerrit.cloudera.org:8080/#/c/18430/8/be/src/service/client-request-state.cc File be/src/service/client-request-state.cc: http://gerrit.cloudera.org:8080/#/c/18430/8/be/src/service/client-request-state.cc@284 PS8, Line 284: } else if (exec_request_->set_query_option_request.__isset.query_option_type > I think we should also check "exec_request_->set_query_option_request.__iss Done http://gerrit.cloudera.org:8080/#/c/18430/8/be/src/service/query-options.cc File be/src/service/query-options.cc: http://gerrit.cloudera.org:8080/#/c/18430/8/be/src/service/query-options.cc@1309 PS8, Line 1309: TQueryOptions* que > I think this should be "DCHECK_GE(option, 0)". Query options generated in Q Thanks for the great suggestion. http://gerrit.cloudera.org:8080/#/c/18430/8/fe/src/test/java/org/apache/impala/analysis/AnalyzeStmtsTest.java File fe/src/test/java/org/apache/impala/analysis/AnalyzeStmtsTest.java: http://gerrit.cloudera.org:8080/#/c/18430/8/fe/src/test/java/org/apache/impala/analysis/AnalyzeStmtsTest.java@4639 PS8, Line 4639: AnalyzesOk("set"); > Could you add a test here for "unset all"? Done http://gerrit.cloudera.org:8080/#/c/18430/8/shell/impala_shell.py File shell/impala_shell.py: http://gerrit.cloudera.org:8080/#/c/18430/8/shell/impala_shell.py@895 PS8, Line 895: elif option == 'ALL': > Let's print some info like the above cases. Done http://gerrit.cloudera.org:8080/#/c/18430/8/tests/custom_cluster/test_set_and_unset.py File tests/custom_cluster/test_set_and_unset.py: http://gerrit.cloudera.org:8080/#/c/18430/8/tests/custom_cluster/test_set_and_unset.py@86 PS8, Line 86: > Could you add some tests here? We need to test that if impala is launched w Done http://gerrit.cloudera.org:8080/#/c/18430/8/tests/shell/test_shell_interactive.py File tests/shell/test_shell_interactive.py: http://gerrit.cloudera.org:8080/#/c/18430/8/tests/shell/test_shell_interactive.py@686 PS8, Line 686: assert "\tDEFAULT_FILE_FORMAT: avro" in result.stdout > Can we add a test for "unset all" here? We currently don't have tests on im Done -- To view, visit http://gerrit.cloudera.org:8080/18430 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Iabf23622daab733ddab20dd3ca73af6c9bd5c250 Gerrit-Change-Number: 18430 Gerrit-PatchSet: 11 Gerrit-Owner: Xiaoqing Gao <[email protected]> Gerrit-Reviewer: Gabor Kaszab <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Jian Zhang <[email protected]> Gerrit-Reviewer: Quanlong Huang <[email protected]> Gerrit-Reviewer: Xiaoqing Gao <[email protected]> Gerrit-Comment-Date: Wed, 08 Jun 2022 10:10:48 +0000 Gerrit-HasComments: Yes
