David Ribeiro Alves has posted comments on this change. Change subject: Allow to pad UNIXTIME_MICROS slots in scan results ......................................................................
Patch Set 4: (6 comments) http://gerrit.cloudera.org:8080/#/c/6623/4/src/kudu/common/wire_protocol-test.cc File src/kudu/common/wire_protocol-test.cc: PS4, Line 248: *reinterpret_cast<int64_t*>(row.mutable_cell_ptr(0)) = i; : Slice col1; : // See: FillRowBlockWithTestRows() for the reason why we relocate these : // to 'test_data_arena_'. : CHECK(test_data_arena_.RelocateSlice("hello world col1", &col1)); : *reinterpret_cast<Slice*>(row.mutable_cell_ptr(1)) = col1; : *reinterpret_cast<int64_t*>(row.mutable_cell_ptr(2)) = i; : *reinterpret_cast<int32_t*>(row.mutable_cell_ptr(3)) = i; : row.cell(3).set_null(false); : *reinterpret_cast<int64_t*>(row.mutable_cell_ptr(4)) = i; : row.cell(4).set_null(true); > would using RowBuilder be simpler here? this is the method we use for the other tests, so keeping it similar helps to understand this one a bit better IMO. plus rowbuilder has its own arena inside, per row, and requires copies. PS4, Line 298: col3 > col2 Done PS4, Line 301: bytes. : EXPECT_FALSE(BitmapTest(base_data + 68, 3)); : // 'col4' is supposed to be null, but should also read 0 since we memsetted the : // memory to 0. It should come at 52 byt > I think these comments should be merged, since the second gives a bit more I moved the null bitmap pointer calculation to the top. http://gerrit.cloudera.org:8080/#/c/6623/4/src/kudu/common/wire_protocol.cc File src/kudu/common/wire_protocol.cc: PS4, Line 487: size_t row_size = ContiguousRowHelper::row_size(schema); > I was actually planning to do this on the next patch up, since this is clie Done PS4, Line 691: has_nullable_cols > Right, but has_nullable_cols only gets set within this if block, which is p Done Line 700: size_t new_base_size = row_stride * num_rows; > hrm, I would expect this variable to be equal to old_size + row_stride * nu Done -- To view, visit http://gerrit.cloudera.org:8080/6623 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I99fc6d3be089d19ebe2e70c938f2405c381578b4 Gerrit-PatchSet: 4 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: David Ribeiro Alves <[email protected]> Gerrit-Reviewer: Andrew Wong <[email protected]> Gerrit-Reviewer: David Ribeiro Alves <[email protected]> Gerrit-Reviewer: Jean-Daniel Cryans <[email protected]> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Matthew Jacobs <[email protected]> Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon <[email protected]> Gerrit-HasComments: Yes
