Daniel Becker has posted comments on this change. ( http://gerrit.cloudera.org:8080/18868 )
Change subject: IMPALA-5323: Support BINARY columns in Kudu tables ...................................................................... Patch Set 5: (7 comments) http://gerrit.cloudera.org:8080/#/c/18868/5/be/src/exprs/cast-functions-ir.cc File be/src/exprs/cast-functions-ir.cc: http://gerrit.cloudera.org:8080/#/c/18868/5/be/src/exprs/cast-functions-ir.cc@378 PS5, Line 378: TODO Could you create a Jira ticket for this and include its number in the comment? http://gerrit.cloudera.org:8080/#/c/18868/5/be/src/exprs/cast-functions-ir.cc@384 PS5, Line 384: AnyValUtil::TruncateIfNecessary(ctx->GetReturnType(), &sv); Do we also cast to VARCHAR? If not, this line is not needed; if yes, please include it in the comment around L372. http://gerrit.cloudera.org:8080/#/c/18868/5/be/src/runtime/types.cc File be/src/runtime/types.cc: http://gerrit.cloudera.org:8080/#/c/18868/5/be/src/runtime/types.cc@263 PS5, Line 263: case TYPE_STRING: Optional: We could take this branch out of the SWITCH and before the creation of the stringstream because this branch doesn't need a stream. On the other hand the code would be more complicated. http://gerrit.cloudera.org:8080/#/c/18868/5/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/18868/5/fe/src/main/java/org/apache/impala/analysis/CastExpr.java@198 PS5, Line 198: TODO Do you plan to resolve this in this patch? http://gerrit.cloudera.org:8080/#/c/18868/5/testdata/datasets/functional/schema_constraints.csv File testdata/datasets/functional/schema_constraints.csv: http://gerrit.cloudera.org:8080/#/c/18868/5/testdata/datasets/functional/schema_constraints.csv@295 PS5, Line 295: table_name:binary_tbl, constraint:only, table_format:kudu/none/none Do we only use these tables for Kudu? If so, can we omit the LOAD and DEPENDENT_LOAD parts from functional_schema_template.sql? http://gerrit.cloudera.org:8080/#/c/18868/5/testdata/workloads/functional-planner/queries/PlannerTest/kudu.test File testdata/workloads/functional-planner/queries/PlannerTest/kudu.test: http://gerrit.cloudera.org:8080/#/c/18868/5/testdata/workloads/functional-planner/queries/PlannerTest/kudu.test@774 PS5, Line 774: Non Nit: Not valid? Or Non-utf-8 strings? http://gerrit.cloudera.org:8080/#/c/18868/5/testdata/workloads/functional-query/queries/QueryTest/binary-type.test File testdata/workloads/functional-query/queries/QueryTest/binary-type.test: http://gerrit.cloudera.org:8080/#/c/18868/5/testdata/workloads/functional-query/queries/QueryTest/binary-type.test@170 PS5, Line 170: constants folding and non-ascii Is any of 1) constant folding 2) non-ascii characters enough for not being able to push down the predicates? If so, we should make two separate tests, one with only 1) and one with only 2). If both are required, the comment could be made clearer. -- To view, visit http://gerrit.cloudera.org:8080/18868 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Iff701a4b3a09ce7b6982c5d238e65f3d4f3d1151 Gerrit-Change-Number: 18868 Gerrit-PatchSet: 5 Gerrit-Owner: Csaba Ringhofer <[email protected]> Gerrit-Reviewer: Daniel Becker <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Peter Rozsa <[email protected]> Gerrit-Comment-Date: Wed, 27 Mar 2024 13:29:22 +0000 Gerrit-HasComments: Yes
