Riza Suminto has posted comments on this change. ( http://gerrit.cloudera.org:8080/20770 )
Change subject: IMPALA-12426: Query History Table ...................................................................... Patch Set 9: (4 comments) Will do another pass, but here are few comments from me: http://gerrit.cloudera.org:8080/#/c/20770/9/be/src/exprs/expr-test.cc File be/src/exprs/expr-test.cc: http://gerrit.cloudera.org:8080/#/c/20770/9/be/src/exprs/expr-test.cc@4350 PS9, Line 4350: TestStringValue("prettyprint_duration(-1)", "-1000.000ns"); Can you please add the new string functions to BenchmarkStringFunctions in be/src/benchmarks/expr-benchmark.cc? If possible, please update the perf table there as well. To get consistent number, I usually do the following before running microbenchmark. sudo cpupower frequency-set --governor performance echo "1" | sudo tee /sys/devices/system/cpu/intel_pstate/no_turbo http://gerrit.cloudera.org:8080/#/c/20770/9/be/src/exprs/string-functions-ir.cc File be/src/exprs/string-functions-ir.cc: http://gerrit.cloudera.org:8080/#/c/20770/9/be/src/exprs/string-functions-ir.cc@1811 PS9, Line 1811: const string& fmt_str = PrettyPrinter::Print(val, unit); PrettyPrinter::Print use stringstream internally. I'm worried using it as row function will cause memory and performance issue like IMPALA-10984. Perhaps a more performant variant that involve less string allocation and copying is needed here. http://gerrit.cloudera.org:8080/#/c/20770/9/tests/custom_cluster/test_query_log.py File tests/custom_cluster/test_query_log.py: http://gerrit.cloudera.org:8080/#/c/20770/9/tests/custom_cluster/test_query_log.py@51 PS9, Line 51: sleep(4) This might takes longer in sanitized build. Some test delay for longer like this: https://github.com/apache/impala/blob/9ecf0cbfc79cd4ee0b1b0ff9143f3eda20cbdcb4/tests/query_test/test_runtime_filters.py#L37 Maybe it is more deterministic to wait until impala-server.completed_queries.written metric increased? example: https://github.com/apache/impala/blob/9ecf0cbfc79cd4ee0b1b0ff9143f3eda20cbdcb4/tests/custom_cluster/test_executor_groups.py#L175-L176 http://gerrit.cloudera.org:8080/#/c/20770/9/tests/custom_cluster/test_query_log.py@359 PS9, Line 359: shell_result = run_impala_shell_cmd_no_expect(vector, : ["--query={0}".format(create_tbl_sql), : "--show_profiles"]) Why not use self.execute_query() and retrive the text profile using query handle? -- 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: 9 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: Wed, 17 Jan 2024 01:52:15 +0000 Gerrit-HasComments: Yes
