Gabor Kaszab has posted comments on this change. ( http://gerrit.cloudera.org:8080/13680 )
Change subject: IMPALA-8665 Include extra info in error message when date cast fails ...................................................................... Patch Set 2: (4 comments) Thanks for taking care of this! http://gerrit.cloudera.org:8080/#/c/13680/2/be/src/exprs/cast-functions-ir.cc File be/src/exprs/cast-functions-ir.cc: http://gerrit.cloudera.org:8080/#/c/13680/2/be/src/exprs/cast-functions-ir.cc@310 PS2, Line 310: DateVal CastFunctions::CastToDateVal(FunctionContext* ctx, const StringVal& val) { Please cover this change with tests. If you grep for "String to Date parse failed." string in the tests directory you find the ones needed an update. E.g. testdata/workloads/functional-query/queries/QueryTest/date.test has some of these. http://gerrit.cloudera.org:8080/#/c/13680/2/be/src/exprs/cast-functions-ir.cc@314 PS2, Line 314: (char *) The preferred way of converting here is using reinterpret_cast as seen above. You don't have to do it twice if you move the result above to a variable. http://gerrit.cloudera.org:8080/#/c/13680/2/be/src/exprs/cast-functions-ir.cc@316 PS2, Line 316: invalidVal Have you tried to simply pass the char* here? Is that a null terminated string? http://gerrit.cloudera.org:8080/#/c/13680/2/be/src/exprs/cast-functions-ir.cc@316 PS2, Line 316: c_str() Do you need to convert the return value of Substitute() to char*? -- To view, visit http://gerrit.cloudera.org:8080/13680 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: If800b7696515cd61afee27220c55ff2440a86f04 Gerrit-Change-Number: 13680 Gerrit-PatchSet: 2 Gerrit-Owner: Jiawei Wang <jiawei.w...@cloudera.com> Gerrit-Reviewer: Attila Jeges <atti...@cloudera.com> Gerrit-Reviewer: Gabor Kaszab <gaborkas...@cloudera.com> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Comment-Date: Thu, 20 Jun 2019 06:48:12 +0000 Gerrit-HasComments: Yes
