Csaba Ringhofer has posted comments on this change. ( http://gerrit.cloudera.org:8080/18868 )
Change subject: IMPALA-5323: Support BINARY columns in Kudu tables ...................................................................... Patch Set 6: (8 comments) http://gerrit.cloudera.org:8080/#/c/18868/5/be/src/exec/kudu/kudu-util-ir.cc File be/src/exec/kudu/kudu-util-ir.cc: http://gerrit.cloudera.org:8080/#/c/18868/5/be/src/exec/kudu/kudu-util-ir.cc@75 PS5, Line 75: > nit: A bit out of the scope of this patch, but this string literal is used Done 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: nyVa > Could you create a Jira ticket for this and include its number in the comme Did the optimization in the end, now TruncateIfNecessary is called in cast( ... as varchar) http://gerrit.cloudera.org:8080/#/c/18868/5/be/src/exprs/cast-functions-ir.cc@384 PS5, Line 384: // STRING -> BINARY > Do we also cast to VARCHAR? If not, this line is not needed; if yes, please cleaned up the comments 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: return is_binary_ ? "BINARY" : "STRING"; > Optional: We could take this branch out of the SWITCH and before the creati rewritten all to Substitute I don't know how much constructing an empty stringstream costs (probably not much), but my experience is that creating/deleting lot of small stringstreams is slow. 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: > Do you plan to resolve this in this patch? I agree with Peter that it should be a separate patch. 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: Not > Nit: Not valid? Or Non-utf-8 strings? Done 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@159 PS5, Line 159: simp > typo Done http://gerrit.cloudera.org:8080/#/c/18868/5/testdata/workloads/functional-query/queries/QueryTest/binary-type.test@170 PS5, Line 170: ants folding and non-ascii char > Is any of 1) constant folding 2) non-ascii characters enough for not being The reason for not having such a test is that in case of BINARY I cannot have a literal on the right side, it needs to be an explicit cast to binary. The literal use case is only relevant for strings. -- 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: 6 Gerrit-Owner: Csaba Ringhofer <[email protected]> Gerrit-Reviewer: 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, 03 Apr 2024 11:22:17 +0000 Gerrit-HasComments: Yes
