Bikramjeet Vig has posted comments on this change. ( http://gerrit.cloudera.org:8080/17894 )
Change subject: IMPALA-10950: Update expr-benchmark.cc ...................................................................... Patch Set 3: (8 comments) http://gerrit.cloudera.org:8080/#/c/17894/3//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/17894/3//COMMIT_MSG@9 PS3, Line 9: Query planner has move the scalar expression's thrift definition from : 'fragments[0].output_sink.output_exprs' to : 'fragments[0].plan.nodes[0].union_node.const_expr_lists[0]'. which change made this switch? curious as to what the context there was http://gerrit.cloudera.org:8080/#/c/17894/3//COMMIT_MSG@16 PS3, Line 16: some expressions : in the benchmark now will crash without codegen due to missing symbols why are they now crashing if the benchmark earlier used to run fine without codegen? http://gerrit.cloudera.org:8080/#/c/17894/3/be/src/benchmarks/expr-benchmark.cc File be/src/benchmarks/expr-benchmark.cc: http://gerrit.cloudera.org:8080/#/c/17894/3/be/src/benchmarks/expr-benchmark.cc@108 PS3, Line 108: const TQueryExecRequest& query_request = request.query_exec_request; nit: might be worth adding context here about the kind of plan created for a constant query http://gerrit.cloudera.org:8080/#/c/17894/3/be/src/benchmarks/expr-benchmark.cc@119 PS3, Line 119: // Uncomment the following lines for debugging. : // union_node.printTo(cout); : // cout << endl << " Need Codegen: " << test->fragment_state->ScalarExprNeedsCodegen() : // << endl << endl; do these need to be removed? http://gerrit.cloudera.org:8080/#/c/17894/3/be/src/benchmarks/expr-benchmark.cc@134 PS3, Line 134: codegen->EnableOptimizations(false); why are we disabling this? http://gerrit.cloudera.org:8080/#/c/17894/3/be/src/benchmarks/expr-benchmark.cc@141 PS3, Line 141: void ReleaseTestData() { : test_data_.clear(); : mem_pool_.FreeAll(); : } nit: you can probably just add this to the destructor instead of a separate function http://gerrit.cloudera.org:8080/#/c/17894/3/be/src/benchmarks/expr-benchmark.cc@183 PS3, Line 183: #define BENCHMARK(name, stmt) \ : suite->AddBenchmark(name, BenchmarkQueryFn, GenerateBenchmarkExprs(stmt, true)) we should probably do both with and without codegen and also keep the benchmark results for both, will be good to have historical data. http://gerrit.cloudera.org:8080/#/c/17894/3/be/src/benchmarks/expr-benchmark.cc@234 PS3, Line 234: // Cast: Function Rate Comparison : // ---------------------------------------------------------------------- : // int_to_int 824 1X : // int_to_bool 878 1.066X : // int_to_double 775.4 0.941X : // int_to_string 32.47 0.03941X : // double_to_boolean 823.5 0.9994X : // double_to_bigint 775.4 0.941X : // double_to_string 4.682 0.005682X : // string_to_int 402.6 0.4886X : // string_to_float 145.8 0.1769X : // string_to_timestamp 83.76 0.1017X all of these values are for benchmark without codegen, can you update these too? -- To view, visit http://gerrit.cloudera.org:8080/17894 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I5b17434d85e32a58622bffb64a697b062a8bf43f Gerrit-Change-Number: 17894 Gerrit-PatchSet: 3 Gerrit-Owner: Riza Suminto <riza.sumi...@cloudera.com> Gerrit-Reviewer: Bikramjeet Vig <bikramjeet....@cloudera.com> Gerrit-Reviewer: Gabor Kaszab <gaborkas...@cloudera.com> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Reviewer: Riza Suminto <riza.sumi...@cloudera.com> Gerrit-Comment-Date: Tue, 05 Oct 2021 07:01:06 +0000 Gerrit-HasComments: Yes
