ZhangYao has posted comments on this change. ( http://gerrit.cloudera.org:8080/13749 )
Change subject: Extend benchmark for ColumnarRowBlockToPB. ...................................................................... Patch Set 3: (10 comments) http://gerrit.cloudera.org:8080/#/c/13749/2/src/kudu/common/wire_protocol-test.cc File src/kudu/common/wire_protocol-test.cc: http://gerrit.cloudera.org:8080/#/c/13749/2/src/kudu/common/wire_protocol-test.cc@91 PS2, Line 91: } > nit: use 'num_columns' instead of 'n' so it's more descriptive Done http://gerrit.cloudera.org:8080/#/c/13749/2/src/kudu/common/wire_protocol-test.cc@98 PS2, Line 98: > nit: the UINT type is more or less deprecated, we only use signed ints in t Done http://gerrit.cloudera.org:8080/#/c/13749/2/src/kudu/common/wire_protocol-test.cc@99 PS2, Line 99: benchmark_schema_.Reset(column_schemas, 1); > I think it'd be more readable to just write the above 6 lines as : Done http://gerrit.cloudera.org:8080/#/c/13749/2/src/kudu/common/wire_protocol-test.cc@115 PS2, Line 115: memcpy(row.mutable_cell_ptr(j), &i, sizeof(int32_t)); > nit: use memcpy here instead of *reinterpret_cast Done http://gerrit.cloudera.org:8080/#/c/13749/2/src/kudu/common/wire_protocol-test.cc@117 PS2, Line 117: LOG(FATAL) << "Unexpected type."; > same Done http://gerrit.cloudera.org:8080/#/c/13749/2/src/kudu/common/wire_protocol-test.cc@118 PS2, Line 118: } > else { LOG(FATAL); } Done http://gerrit.cloudera.org:8080/#/c/13749/2/src/kudu/common/wire_protocol-test.cc@123 PS2, Line 123: Row > nit: Rows Done http://gerrit.cloudera.org:8080/#/c/13749/2/src/kudu/common/wire_protocol-test.cc@137 PS2, Line 137: select_vector->SetAllFalse(); > nit: this loop would run very long when rate approaches 1. Done http://gerrit.cloudera.org:8080/#/c/13749/2/src/kudu/common/wire_protocol-test.cc@149 PS2, Line 149: na arena(102 > nit: "RunBenchmark" (Benchmark is one word, so not camel case) My mistake :( http://gerrit.cloudera.org:8080/#/c/13749/2/src/kudu/common/wire_protocol-test.cc@426 PS2, Line 426: column_counts = {3, 30, 300}; > maybe set these values if AllowSlowTests is true? Done -- To view, visit http://gerrit.cloudera.org:8080/13749 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ie29937c316be624151e6c51e54545c4f023e603d Gerrit-Change-Number: 13749 Gerrit-PatchSet: 3 Gerrit-Owner: ZhangYao <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Todd Lipcon <[email protected]> Gerrit-Reviewer: ZhangYao <[email protected]> Gerrit-Comment-Date: Tue, 02 Jul 2019 11:33:38 +0000 Gerrit-HasComments: Yes
