Grant Henke has posted comments on this change. ( http://gerrit.cloudera.org:8080/13760 )
Change subject: KUDU-1938 Add support for CHAR/VARCHAR pt 1 ...................................................................... Patch Set 10: (6 comments) http://gerrit.cloudera.org:8080/#/c/13760/10/src/kudu/common/column_predicate-test.cc File src/kudu/common/column_predicate-test.cc: http://gerrit.cloudera.org:8080/#/c/13760/10/src/kudu/common/column_predicate-test.cc@1124 PS10, Line 1124: ColumnSchema chars("chars", CHAR); > I changed the name, 'char' is a reserved keyword, so I can't rename the var ah, right. http://gerrit.cloudera.org:8080/#/c/13760/13/src/kudu/common/partial_row-test.cc File src/kudu/common/partial_row-test.cc: http://gerrit.cloudera.org:8080/#/c/13760/13/src/kudu/common/partial_row-test.cc@270 PS13, Line 270: EXPECT_EQ("char char_val=\"shortval \"", row.ToString()); Should char values be padded in row.ToString()? http://gerrit.cloudera.org:8080/#/c/13760/13/src/kudu/common/partial_row.cc File src/kudu/common/partial_row.cc: http://gerrit.cloudera.org:8080/#/c/13760/13/src/kudu/common/partial_row.cc@432 PS13, Line 432: Status KuduPartialRow::SetCharVarchar(const Slice& col_name, const Slice& val) { Maybe move this truncating logic to char_util.cc? http://gerrit.cloudera.org:8080/#/c/13760/10/src/kudu/common/types.h File src/kudu/common/types.h: http://gerrit.cloudera.org:8080/#/c/13760/10/src/kudu/common/types.h@634 PS10, Line 634: struct DataTypeTraits<CHAR> : public DerivedTypeTraits<BINARY>{ > DerivedTypeTraits is template<DataType PhysicalType>, so I think only physi I forgot STRING was already a "DerivedType". This makes sense. http://gerrit.cloudera.org:8080/#/c/13760/10/src/kudu/common/types.h@647 PS10, Line 647: struct DataTypeTraits<VARCHAR> : public DerivedTypeTraits<BINARY>{ > Can this be DerivedTypeTraits STRING to unify all the UTF8 and debug string Done http://gerrit.cloudera.org:8080/#/c/13760/13/src/kudu/util/slice.h File src/kudu/util/slice.h: http://gerrit.cloudera.org:8080/#/c/13760/13/src/kudu/util/slice.h@176 PS13, Line 176: std::string ToString() const; Should this live in char_util.cc instead of slice? -- To view, visit http://gerrit.cloudera.org:8080/13760 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I998982dba93831db91c43a97ce30d3e68c2a4a54 Gerrit-Change-Number: 13760 Gerrit-PatchSet: 10 Gerrit-Owner: Attila Bukor <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Attila Bukor <[email protected]> Gerrit-Reviewer: Grant Henke <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: Todd Lipcon <[email protected]> Gerrit-Reviewer: Will Berkeley <[email protected]> Gerrit-Comment-Date: Sat, 29 Jun 2019 20:44:59 +0000 Gerrit-HasComments: Yes
