Attila Bukor 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 16: (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("char", CHAR); > ah, right. Done 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()? I don't think we can do that without storing the value with padding. We would need to have the length which isn't available in the physical SLICE type. http://gerrit.cloudera.org:8080/#/c/13760/10/src/kudu/common/partial_row.cc File src/kudu/common/partial_row.cc: http://gerrit.cloudera.org:8080/#/c/13760/10/src/kudu/common/partial_row.cc@455 PS10, Line 455: Status KuduPartialRow::SetNull(int col_idx) { > I think we should follow the same semantics as Impala: https://impala.apach Done 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: if (PREDICT_FALSE(T::type != col.type_info()->type())) { > Maybe move this truncating logic to char_util.cc? Done 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>{ > I forgot STRING was already a "DerivedType". This makes sense. 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? Done -- 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: 16 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: Sun, 30 Jun 2019 21:27:06 +0000 Gerrit-HasComments: Yes
