Mahesh Reddy has posted comments on this change. ( http://gerrit.cloudera.org:8080/17161 )
Change subject: KUDU-2671: No-op ClientServerMapping if only one schema exists. ...................................................................... Patch Set 3: (2 comments) http://gerrit.cloudera.org:8080/#/c/17161/3/src/kudu/common/row_operations.cc File src/kudu/common/row_operations.cc: http://gerrit.cloudera.org:8080/#/c/17161/3/src/kudu/common/row_operations.cc@449 PS3, Line 449: size_t tablet_col_idx = client_col_idx; > nit: would it make sense to separate this into an utility method? I considered this as well, I can make that change. http://gerrit.cloudera.org:8080/#/c/17161/3/src/kudu/common/row_operations.cc@683 PS3, Line 683: mapping > BTW, what's the motivation behind keeping this as a location variable and p To answer the first part, I didn't consider the tradeoff between keeping it as a location variable or turning it into a member of this class since the scope of this patch was to just allow the client schema to have column IDs if its the same schema object as the tablet schema. I could turn the mapping object into a member of RowOperationsPBDecoder, of course it would involve touching more areas of the code. I'm ok keeping it as is, but if you feel strongly about it I can make 'mapping' a member of this class. -- To view, visit http://gerrit.cloudera.org:8080/17161 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I836f157201509a9b38a3ad42d653236f240dda5e Gerrit-Change-Number: 17161 Gerrit-PatchSet: 3 Gerrit-Owner: Mahesh Reddy <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Andrew Wong <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Mahesh Reddy <[email protected]> Gerrit-Comment-Date: Wed, 10 Mar 2021 03:19:18 +0000 Gerrit-HasComments: Yes
