Riza Suminto has posted comments on this change. ( http://gerrit.cloudera.org:8080/20770 )
Change subject: IMPALA-12426: Query History Table ...................................................................... Patch Set 15: (6 comments) http://gerrit.cloudera.org:8080/#/c/20770/15//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/20770/15//COMMIT_MSG@25 PS15, Line 25: Negative testing consists of attempting sql injection : attacks and syntactically incorrect queries. Mention that you run expr-benchmark in commit message. Also mention your machine info for note. http://gerrit.cloudera.org:8080/#/c/20770/12/be/src/benchmarks/expr-benchmark.cc File be/src/benchmarks/expr-benchmark.cc: http://gerrit.cloudera.org:8080/#/c/20770/12/be/src/benchmarks/expr-benchmark.cc@591 PS12, Line 591: // prettyprint_duration 5.67 5.67 5.67 0.048X 0.0475X 0.0471X > Done Thanks for updating the table. I found that the following functions now slower more than 2x compared to the baseline lower upper reverse space ascii concat concat2 concatws2 repeat lpad rpad We should investigate separately whether this is a real issue or not. http://gerrit.cloudera.org:8080/#/c/20770/12/be/src/service/CMakeLists.txt File be/src/service/CMakeLists.txt: http://gerrit.cloudera.org:8080/#/c/20770/12/be/src/service/CMakeLists.txt@46 PS12, Line 46: query-state-re > Good idea. Renamed. Done http://gerrit.cloudera.org:8080/#/c/20770/13/be/src/service/impala-server.h File be/src/service/impala-server.h: http://gerrit.cloudera.org:8080/#/c/20770/13/be/src/service/impala-server.h@1261 PS13, Line 1261: /// The QueryDriverMap maps query ids to QueryDrivers. The QueryDrivers are owned by the : /// ImpalaServer and QueryDriverMap references them using shared_ptr to allow : /// asynchronous deletion. : /// > Done Done http://gerrit.cloudera.org:8080/#/c/20770/13/be/src/service/workload-management.cc File be/src/service/workload-management.cc: http://gerrit.cloudera.org:8080/#/c/20770/13/be/src/service/workload-management.cc@615 PS13, Line 615: || !FLAGS_enable_workload_mgmt){ : return; : } : > Good catch. This code does miss use/show sql statements that start with co Done http://gerrit.cloudera.org:8080/#/c/20770/15/be/src/service/workload-management.cc File be/src/service/workload-management.cc: http://gerrit.cloudera.org:8080/#/c/20770/15/be/src/service/workload-management.cc@622 PS15, Line 622: string op_type = to_string(query_handle->catalog_op_type()); : : if (boost::algorithm::equals(op_type.substr(0,3), "USE") : || boost::algorithm::equals(op_type.substr(0,4), "SHOW")) { : return; : } I think using switch and fall through is more reliable rather than parsing to string. There is no guarantee that TCatalogOpType will be consistent with "USE" and "SHOW_*" naming in the future. See ClientRequestState::ExecLocalCatalogOp for an example. -- To view, visit http://gerrit.cloudera.org:8080/20770 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I2d2da9d450fba4e789400cfa62927fc25d34f844 Gerrit-Change-Number: 20770 Gerrit-PatchSet: 15 Gerrit-Owner: Jason Fehr <jf...@cloudera.com> Gerrit-Reviewer: Andrew Sherman <asher...@cloudera.com> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Reviewer: Jason Fehr <jf...@cloudera.com> Gerrit-Reviewer: Michael Smith <michael.sm...@cloudera.com> Gerrit-Reviewer: Riza Suminto <riza.sumi...@cloudera.com> Gerrit-Comment-Date: Tue, 30 Jan 2024 16:29:02 +0000 Gerrit-HasComments: Yes
