Quanlong Huang has posted comments on this change. ( http://gerrit.cloudera.org:8080/18430 )
Change subject: IMPALA-11233: Unset all query option ...................................................................... Patch Set 8: (9 comments) Thanks for adding this useful feature! The solution LGTM. Asking for more tests. 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: option, nit: options. http://gerrit.cloudera.org:8080/#/c/18430/8//COMMIT_MSG@12 PS8, Line 12: affect nit: effect? http://gerrit.cloudera.org:8080/#/c/18430/8//COMMIT_MSG@13 PS8, Line 13: restart impalad I think users can re-create a connection instead of restarting the impalad. Maybe rework this to "without recreating a new connection" 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.query_option_type I think we should also check "exec_request_->set_query_option_request.__isset.query_option_type" here. 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: if (option >= 0) { I think this should be "DCHECK_GE(option, 0)". Query options generated in QUERY_OPT_FN macro should be valid. 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"? 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. 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 with customized default_query_options, "unset all" will reset query options to the customized ones. 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 impala-shell. -- 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: 8 Gerrit-Owner: Xiaoqing Gao <gaoxq...@gmail.com> Gerrit-Reviewer: Gabor Kaszab <gaborkas...@cloudera.com> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Reviewer: Jian Zhang <zjsar...@gmail.com> Gerrit-Reviewer: Quanlong Huang <huangquanl...@gmail.com> Gerrit-Reviewer: Xiaoqing Gao <gaoxq...@gmail.com> Gerrit-Comment-Date: Tue, 31 May 2022 09:20:59 +0000 Gerrit-HasComments: Yes
