Andrew Wong 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: client_has_ids nit: maybe call this "use_client_schema_as_tablet_schema" or something? Or something along those lines (i know it's a mouthful) 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 Also can you describe why the distinction is important as a comment? 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 something like 'use_client_schema_as_tablet_schema ? &client_schema : &schema'? 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 description? And maybe allude to how it's exercising specific codepaths. 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 to define a mode of operation. How about defining some enum DecoderSchemaUsage { kClientSchemaNoColIds, kClientSchemaEqualsTabletSchema, }; 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 use pointer comparison? Then we wouldn't even need to define this bool (or enum, as mentioned in my other comment) -- we could just use if (client_schema_ != tablet_schema_) { ... everywhere. And of course we should also then document that it is the only time it's acceptable for the client schema to have column IDs. 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: client_equals_to_tablet_schema, nit: document what this does and any expectations users should have in setting it. 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&. -- 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-Comment-Date: Tue, 09 Mar 2021 01:22:54 +0000 Gerrit-HasComments: Yes
