[Impala-ASF-CR] IMPALA-9482: Support for BINARY columns

["Steve Carlin (Code Review)" <ge...@cloudera.org>]

Steve Carlin has posted comments on this change. ( http://gerrit.cloudera.org:8080/16066 )

Change subject: IMPALA-9482: Support for BINARY columns
......................................................................


Patch Set 14:

(7 comments)

Hopefully these comments are helpful.  Logic seems good, these are all pretty much nits.

Thanks!

http://gerrit.cloudera.org:8080/#/c/16066/14/be/src/runtime/types.cc
File be/src/runtime/types.cc:

http://gerrit.cloudera.org:8080/#/c/16066/14/be/src/runtime/types.cc@141
PS14, Line 141:       return TPrimitiveType::BINARY;
Should we return INVALID_TYPE here because of DCHECK(false) so that we catch the problem earlier on?


http://gerrit.cloudera.org:8080/#/c/16066/14/be/src/service/hs2-util.cc
File be/src/service/hs2-util.cc:

http://gerrit.cloudera.org:8080/#/c/16066/14/be/src/service/hs2-util.cc@872
PS14, Line 872:   AuxColumnType aux_type(columnType);
If this is only used in the TYPE_STRING, perhaps it should be initialized there.


http://gerrit.cloudera.org:8080/#/c/16066/14/be/src/service/hs2-util.cc@950
PS14, Line 950:       type_entry.__set_type(thrift::TTypeId::STRING_TYPE);
If DCHECK is false, do we want to set this to STRING_TYPE?  Or maybe we should set it to INVALID_TYPE?  Not sure what is best.


http://gerrit.cloudera.org:8080/#/c/16066/14/be/src/service/impala-beeswax-server.cc
File be/src/service/impala-beeswax-server.cc:

http://gerrit.cloudera.org:8080/#/c/16066/14/be/src/service/impala-beeswax-server.cc@212
PS14, Line 212:     return "string";
How come we don't return TypeToOdbcString here?

(sidenote, I'm rusty on my C++, but why is TypeToOdbcString not inlined?)


http://gerrit.cloudera.org:8080/#/c/16066/14/fe/src/main/java/org/apache/impala/analysis/CastExpr.java
File fe/src/main/java/org/apache/impala/analysis/CastExpr.java:

http://gerrit.cloudera.org:8080/#/c/16066/14/fe/src/main/java/org/apache/impala/analysis/CastExpr.java@187
PS14, Line 187:         // No built-in function needed for BINARY <-> STRING conversion.
I think the comment should also reflect other datatypes versus binary are not in the support matrix.


http://gerrit.cloudera.org:8080/#/c/16066/14/fe/src/main/java/org/apache/impala/analysis/CastExpr.java@357
PS14, Line 357:     boolean isStringBinaryCast =
I may be understanding this wrong...

This is slightly inconsistent with the above code.  In the above code, we assume that if one is binary, the other must be either binary or string.  Do we need to check explicitly here that the other type is string?  I understand why, but then maybe we should have a Preconditions check sayin git is either of type string or binary?

Not sure what is best here.


http://gerrit.cloudera.org:8080/#/c/16066/14/fe/src/main/java/org/apache/impala/analysis/LikePredicate.java
File fe/src/main/java/org/apache/impala/analysis/LikePredicate.java:

http://gerrit.cloudera.org:8080/#/c/16066/14/fe/src/main/java/org/apache/impala/analysis/LikePredicate.java@116
PS14, Line 116:   private static boolean isLikeableType(Type type) {
No real comment here. But I feel the other types might get upset for not being liked.



-- 
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: 14
Gerrit-Owner: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Attila Jeges <at...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Steve Carlin <sc...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Comment-Date: Thu, 14 Jul 2022 00:01:00 +0000
Gerrit-HasComments: Yes