Zoltan Borok-Nagy has posted comments on this change. ( http://gerrit.cloudera.org:8080/16066 )
Change subject: WIP IMPALA-9482 Support for BINARY columns ...................................................................... Patch Set 2: (5 comments) Did a first pass. Only had some questions and a few nits. http://gerrit.cloudera.org:8080/#/c/16066/2/be/src/exec/hbase-scan-node.cc File be/src/exec/hbase-scan-node.cc: http://gerrit.cloudera.org:8080/#/c/16066/2/be/src/exec/hbase-scan-node.cc@49 PS2, Line 49: NULL nit: nullptr? I see there's a lot of NULLs in the file, so maybe just update the NULLs in this init-list? http://gerrit.cloudera.org:8080/#/c/16066/2/be/src/exec/hbase-table-writer.cc File be/src/exec/hbase-table-writer.cc: http://gerrit.cloudera.org:8080/#/c/16066/2/be/src/exec/hbase-table-writer.cc@159 PS2, Line 159: Base64Encode Why do we encode it for HBase? http://gerrit.cloudera.org:8080/#/c/16066/2/be/src/exec/hdfs-rcfile-scanner.cc File be/src/exec/hdfs-rcfile-scanner.cc: http://gerrit.cloudera.org:8080/#/c/16066/2/be/src/exec/hdfs-rcfile-scanner.cc@593 PS2, Line 593: const ColumnDescriptor& col_desc = : scan_node_->hdfs_table()->GetColumnDesc(slot_desc); Wouldn't it be better to have a BINARY type in the BE so we could save passing 'col_desc' all the time? Maybe it would be a cleaner design overall. What will happen when we start supporting UTF-8 strings? But I'm OK with it if that'd require too much changes. http://gerrit.cloudera.org:8080/#/c/16066/2/be/src/runtime/types.h File be/src/runtime/types.h: http://gerrit.cloudera.org:8080/#/c/16066/2/be/src/runtime/types.h@67 PS2, Line 67: Aux nit: 'Aux' is not too descriptive. Probably EncodedColumnType/StoredColumnType? http://gerrit.cloudera.org:8080/#/c/16066/2/be/src/service/hs2-util.cc File be/src/service/hs2-util.cc: http://gerrit.cloudera.org:8080/#/c/16066/2/be/src/service/hs2-util.cc@289 PS2, Line 289: Same as for STRING with the exception of using a different : // Thrift field. nit: refactor to a common code that takes the Thrift field as parameter? -- To view, visit http://gerrit.cloudera.org:8080/16066 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I36861a9ca6c2047b0d76862507c86f7f153bc582 Gerrit-Change-Number: 16066 Gerrit-PatchSet: 2 Gerrit-Owner: Csaba Ringhofer <[email protected]> Gerrit-Reviewer: Gabor Kaszab <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Zoltan Borok-Nagy <[email protected]> Gerrit-Comment-Date: Tue, 16 Jun 2020 16:42:13 +0000 Gerrit-HasComments: Yes
