Gabor Kaszab has posted comments on this change. ( http://gerrit.cloudera.org:8080/16066 )
Change subject: WIP IMPALA-9482 Support for BINARY columns ...................................................................... Patch Set 2: (8 comments) I did a go through on this patch and left some minor comments. However, I feel that a second look from someone more competent to introducing new types should take a look. I see that there are a high number of places you had to touch but I can't judge if there is anything else missing. http://gerrit.cloudera.org:8080/#/c/16066/2//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/16066/2//COMMIT_MSG@47 PS2, Line 47: Testing Do you plan to do interop tests between Hive and Impala? http://gerrit.cloudera.org:8080/#/c/16066/2/be/src/exec/text-converter.h File be/src/exec/text-converter.h: http://gerrit.cloudera.org:8080/#/c/16066/2/be/src/exec/text-converter.h@63 PS2, Line 63: col_desc Could you mention the new param in the comment above? http://gerrit.cloudera.org:8080/#/c/16066/2/be/src/exec/text-converter.h@88 PS2, Line 88: col_desc Could you mention the new param in the comment above? http://gerrit.cloudera.org:8080/#/c/16066/2/be/src/runtime/types.cc File be/src/runtime/types.cc: http://gerrit.cloudera.org:8080/#/c/16066/2/be/src/runtime/types.cc@180 PS2, Line 180: // ODBC driver requires types in lower case nit: I'd move this comment right before the switch to L186. 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@729 PS2, Line 729: // HiveServer2 does not have a type for invalid, date, datetime or : // fixed_uda_intermediate. Shouldn't you change this comment to reflect that now BINARY can also trigger this branch? http://gerrit.cloudera.org:8080/#/c/16066/2/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/2/fe/src/main/java/org/apache/impala/analysis/CastExpr.java@341 PS2, Line 341: boolean isStringBinaryCast = This can go after the Preconditions check, L347. http://gerrit.cloudera.org:8080/#/c/16066/2/fe/src/test/java/org/apache/impala/analysis/AnalyzerTest.java File fe/src/test/java/org/apache/impala/analysis/AnalyzerTest.java: http://gerrit.cloudera.org:8080/#/c/16066/2/fe/src/test/java/org/apache/impala/analysis/AnalyzerTest.java@263 PS2, Line 263: For tables with unsupported primitive types (e.g., binary) This comment is no longer valid, right? http://gerrit.cloudera.org:8080/#/c/16066/2/tests/common/impala_connection.py File tests/common/impala_connection.py: http://gerrit.cloudera.org:8080/#/c/16066/2/tests/common/impala_connection.py@426 PS2, Line 426: ` nit: typo -- 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 <csringho...@cloudera.com> Gerrit-Reviewer: Gabor Kaszab <gaborkas...@cloudera.com> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Comment-Date: Tue, 16 Jun 2020 13:39:03 +0000 Gerrit-HasComments: Yes
