Dan Burkert has posted comments on this change. Change subject: Update debug partition and row printing ......................................................................
Patch Set 1: (21 comments) http://gerrit.cloudera.org:8080/#/c/5262/1/src/kudu/common/partition-test.cc File src/kudu/common/partition-test.cc: Line 411: EXPECT_EQ(string("", 0), partitions[0].partition_key_start()); > warning: constructor creating an empty string [misc-string-constructor] Done Line 428: EXPECT_EQ(string("", 0), partitions[2].partition_key_end()); > warning: constructor creating an empty string [misc-string-constructor] Done Line 500: EXPECT_EQ(string("", 0), partitions[0].partition_key_start()); > warning: constructor creating an empty string [misc-string-constructor] Done Line 520: EXPECT_EQ(string("", 0), partitions[2].range_key_end()); > warning: constructor creating an empty string [misc-string-constructor] Done Line 551: EXPECT_EQ(string("", 0), partitions[5].range_key_end()); > warning: constructor creating an empty string [misc-string-constructor] Done Line 582: EXPECT_EQ(string("", 0), partitions[8].range_key_end()); > warning: constructor creating an empty string [misc-string-constructor] Done Line 613: EXPECT_EQ(string("", 0), partitions[11].range_key_end()); > warning: constructor creating an empty string [misc-string-constructor] Done Line 615: EXPECT_EQ(string("", 0), partitions[11].partition_key_end()); > warning: constructor creating an empty string [misc-string-constructor] Done http://gerrit.cloudera.org:8080/#/c/5262/1/src/kudu/common/partition.cc File src/kudu/common/partition.cc: Line 539: for (ColumnId column_id : range_schema_.column_ids) { > warning: loop variable is copied but only used as const reference; consider Done Line 549: for (ColumnId column_id : range_schema_.column_ids) { > warning: loop variable is copied but only used as const reference; consider Done Line 574: for (ColumnId column_id : column_ids) { > warning: loop variable is copied but only used as const reference; consider Done PS1, Line 585: if (partition.hash_buckets_.size() != hash_bucket_schemas_.size()) { : return "<hash-partition-error>"; : } > should this be a CHECK? would be indicative of programmer error, right? or It's not always indicative of a programmer error. These debug print routines are used to construct good error messages in the case of malformed DDL requests. I also found that PartitionPruner calls them with partition keys that aren't necessarily valid according to the schema (but that's not a bug). Line 621: components.emplace_back(Substitute("<hash-error: $0>", s.ToString())); > same Done Line 652: return "<hash-decode-error>"; > same Done Line 681: } else if (lower_unbounded) { > warning: don't use else after return [readability-else-after-return] Done PS1, Line 689: oakc > not following this reference Done Line 706: return "<range-key-decode-error>"; > same Done Line 728: } else { > warning: don't use else after return [readability-else-after-return] Done Line 736: for (ColumnId column_id : range_schema_.column_ids) { > warning: loop variable is copied but only used as const reference; consider Done Line 750: } else { > warning: don't use else after return [readability-else-after-return] Done Line 810: ColumnIdsToColumnNames(schema, hash_bucket_schema.column_ids)); > for these HTML output functions, we should be escaping the user-provided co Done -- To view, visit http://gerrit.cloudera.org:8080/5262 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I4c444b155fe6621af65b86020be105fe56ae18ef Gerrit-PatchSet: 1 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Dan Burkert <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: Dan Burkert <[email protected]> Gerrit-Reviewer: Dimitris Tsirogiannis <[email protected]> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon <[email protected]> Gerrit-HasComments: Yes
