David Ribeiro Alves has posted comments on this change. Change subject: KUDU-237 (part 1) - Support proper mutation encoding for reinserts ......................................................................
Patch Set 15: (6 comments) 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 this doesn't actually work. the string in your suggestion ends up with the incorrect type (i tried). 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 nonem not really, but rlcs for reinserts are always built from full rows. Line 169: void SetToReinsert(const RowType& src_row); > How about EncodeReinsert() would prefer to avoid externally visible renaming refactorings since that makes it harder for patches down the line. happy to rename after the series if you fell strongly about it. Line 222: void EncodeUpdate(const ColumnSchema& col_schema, > How about EncodeColumnMutation() Done Line 235: void EncodeUpdateRaw(int col_id, bool is_null, Slice new_val); > How about EncodeColumnMutationRaw() Done Line 332: RowChangeListEncoder* out); > nit: why change the variable name from undo_encoder to 'out'? The previous because what's in 'out' is the old state of the row and not an undo. for instance if you do this on a delete you don't get a reinsert encoded back, so undo is a misnomer (see compaction.cc ApplyMutationsAndGenerateUndos) -- 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 <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: 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
