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 1: (8 comments) http://gerrit.cloudera.org:8080/#/c/17161/1/src/kudu/common/row_operations-test.cc File src/kudu/common/row_operations-test.cc: http://gerrit.cloudera.org:8080/#/c/17161/1/src/kudu/common/row_operations-test.cc@761 PS1, Line 761: const bool& > nit: just use bool, or an enum Not entirely sure what distinction you're referring to here. Are you talking about the impact that the bool being true/false has on the client schema? http://gerrit.cloudera.org:8080/#/c/17161/1/src/kudu/common/row_operations-test.cc@761 PS1, Line 761: client_has_ids > nit: maybe call this "use_client_schema_as_tablet_schema" or something? Or Ack http://gerrit.cloudera.org:8080/#/c/17161/1/src/kudu/common/row_operations-test.cc@794 PS1, Line 794: &client_schema > nit: would it be more representative of your expected usages to do somethin sure, although '&client_schema' and '&schema' would be flipped in your example. Can get rid of the if check on L 791 too. http://gerrit.cloudera.org:8080/#/c/17161/1/src/kudu/common/row_operations-test.cc@937 PS1, Line 937: TEST_F(RowOperationsTest, SplitKeysRoundTrip) { > nit: it's not clear what exactly this is testing. Could you add a descripti Ack http://gerrit.cloudera.org:8080/#/c/17161/1/src/kudu/common/row_operations.h File src/kudu/common/row_operations.h: http://gerrit.cloudera.org:8080/#/c/17161/1/src/kudu/common/row_operations.h@176 PS1, Line 176: bool > nit: for readability, we typically prefer using enums instead of booleans t Ack http://gerrit.cloudera.org:8080/#/c/17161/1/src/kudu/common/row_operations.cc File src/kudu/common/row_operations.cc: http://gerrit.cloudera.org:8080/#/c/17161/1/src/kudu/common/row_operations.cc@221 PS1, Line 221: client_schema->Equals(*tablet_schema, Schema::COMPARE_ALL) > Won't the main usage here use the same schema object? If so, can't we just That's a good point, the main use case is when the same schema object is used. My only worry with this approach is that it's more generic than comparing two schemas' fields, but since the end user is us, documentation should be enough of a safeguard. Do you think it would be more clear to still have an enum field that reflects this? It also gives us a place to include documentation of only allowing client schema to have column IDs when it's the same object. http://gerrit.cloudera.org:8080/#/c/17161/1/src/kudu/common/schema.h File src/kudu/common/schema.h: http://gerrit.cloudera.org:8080/#/c/17161/1/src/kudu/common/schema.h@874 PS1, Line 874: const bool& > nit: primitive types don't need const&. Ack http://gerrit.cloudera.org:8080/#/c/17161/1/src/kudu/common/schema.h@874 PS1, Line 874: client_equals_to_tablet_schema, > nit: document what this does and any expectations users should have in sett Ack -- 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: 1 Gerrit-Owner: Mahesh Reddy <mre...@cloudera.com> Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Mahesh Reddy <mre...@cloudera.com> Gerrit-Comment-Date: Tue, 09 Mar 2021 03:09:17 +0000 Gerrit-HasComments: Yes
