Todd Lipcon has posted comments on this change. Change subject: KUDU-237 (part 1) - Support proper mutation encoding for reinserts ......................................................................
Patch Set 5: (12 comments) http://gerrit.cloudera.org:8080/#/c/4791/5/src/kudu/common/row_changelist-test.cc File src/kudu/common/row_changelist-test.cc: Line 176 no equivalent test of an invalid reinsert delta? http://gerrit.cloudera.org:8080/#/c/4791/5/src/kudu/common/row_changelist.cc File src/kudu/common/row_changelist.cc: Line 44: string ret; move this variable down below the DELETE case Line 190: encoder.SetType(decoder.type_); should this be outside the loop? what if, in the projected result, none of the columns are still there. Do we expect it to be empty or to be an empty update/reinsert? wonder if we have a test for this case. Line 222: undo_encoder->SetType(type_); shouldn't the UNDO of a REINSERT be a DELETE? Line 278: out->SetType(decoder.type_); same note as above. I dont think it's a change, but the docs should probably say that the resulting RCL is uninitialized if the removal of columns yields a no-op RCL http://gerrit.cloudera.org:8080/#/c/4791/5/src/kudu/common/row_changelist.h File src/kudu/common/row_changelist.h: Line 156: // The change list includes direct and indirect data. can you be more explicit here that the data is copied? Line 239: Slice val_slice; since this val_slice is never actually set to anything, I think it's clearer to just use a temporary 'Slice()' on line 241 Line 309: // state of the row into the undo_encoder. can you amend this doc to explain what happens with reinsert? Line 313: // Apply this RowChangeList to row number 'row_idx' in 'dst_col', but only same Line 391: Status DecodeNext(DecodedUpdate* update); should this now say REQUIRES is_update or is_reinsert? http://gerrit.cloudera.org:8080/#/c/4791/5/src/kudu/tablet/compaction.cc File src/kudu/tablet/compaction.cc: Line 728: // row but disregard the undo reinsert that is created. why disregard instead of having ApplyRowChanges create the right undo? PS5, Line 734: undo_encoder.Reset(); : undo_encoder.SetToDelete(); this should probably be done by the ApplyRowChanges itself -- To view, visit http://gerrit.cloudera.org:8080/4791 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I2b0a95e3f618bf20eb18ca4f4b03b9e2ce5704e8 Gerrit-PatchSet: 5 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: David Ribeiro Alves <[email protected]> Gerrit-Reviewer: Jean-Daniel Cryans <[email protected]> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy <[email protected]> Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon <[email protected]> Gerrit-HasComments: Yes
