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

Reply via email to