Mike Percy has posted comments on this change. Change subject: KUDU-237 (part 1) - Support proper mutation encoding for reinserts ......................................................................
Patch Set 15: (6 comments) Overall looks good, mostly just had some naming suggestions http://gerrit.cloudera.org:8080/#/c/4791/15/src/kudu/common/row_changelist-test.cc File src/kudu/common/row_changelist-test.cc: Line 201: RowChangeListDecoder decoder(RowChangeList(Slice("\x03"))); I know you didn't do this but instead of Slice("\x03") it would be nicer to see Substitute("$0", kReinsert) even though it requires another variable. http://gerrit.cloudera.org:8080/#/c/4791/15/src/kudu/common/row_changelist.h File src/kudu/common/row_changelist.h: Line 163: SetType(RowChangeList::kReinsert); Does anything in the encoder protect us from having an incomplete but nonempty RCL on a reinsert with non-nullable columns? Line 169: void SetToReinsert(const RowType& src_row); How about EncodeReinsert() Line 222: void EncodeUpdate(const ColumnSchema& col_schema, How about EncodeColumnMutation() Line 235: void EncodeUpdateRaw(int col_id, bool is_null, Slice new_val); How about EncodeColumnMutationRaw() Line 332: RowChangeListEncoder* out); nit: why change the variable name from undo_encoder to 'out'? The previous one seemed more descriptive. -- 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: 15 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: David Ribeiro Alves <dral...@apache.org> Gerrit-Reviewer: Alexey Serbin <aser...@cloudera.com> Gerrit-Reviewer: David Ribeiro Alves <dral...@apache.org> Gerrit-Reviewer: Jean-Daniel Cryans <jdcry...@apache.org> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy <mpe...@apache.org> Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon <t...@apache.org> Gerrit-HasComments: Yes
