Gabor Kaszab has posted comments on this change. ( http://gerrit.cloudera.org:8080/12267 )
Change subject: IMPALA-4018 Part1: Add FORMAT clause in CAST() ...................................................................... Patch Set 6: (4 comments) Sorry, the last patch set came together with a rebase. http://gerrit.cloudera.org:8080/#/c/12267/5/be/src/exprs/cast-expr.h File be/src/exprs/cast-expr.h: http://gerrit.cloudera.org:8080/#/c/12267/5/be/src/exprs/cast-expr.h@43 PS5, Line 43: mutable std::unique_ptr<datetime_parse_util::DateTimeFormatContext> dt_ctx > I do not have a clear preference, but other functions seem to simply delete Smart pointer seemed a more error proof: This way one won't forget to do a delete in case returning early due to an error. http://gerrit.cloudera.org:8080/#/c/12267/5/be/src/exprs/cast-expr.cc File be/src/exprs/cast-expr.cc: http://gerrit.cloudera.org:8080/#/c/12267/5/be/src/exprs/cast-expr.cc@30 PS5, Line 30: RETURN_IF_ERROR(ScalarFnCall::OpenEvaluator(scope, state, eval)); > As far as I understand this will be called with both FRAGMENT_LOCAL and THR Thanks for pointing this out! Done http://gerrit.cloudera.org:8080/#/c/12267/5/testdata/workloads/functional-query/queries/QueryTest/cast_format.test File testdata/workloads/functional-query/queries/QueryTest/cast_format.test: http://gerrit.cloudera.org:8080/#/c/12267/5/testdata/workloads/functional-query/queries/QueryTest/cast_format.test@73 PS5, Line 73: . > nit: . instead of ;? Done http://gerrit.cloudera.org:8080/#/c/12267/5/tests/query_test/test_cast_with_format.py File tests/query_test/test_cast_with_format.py: http://gerrit.cloudera.org:8080/#/c/12267/5/tests/query_test/test_cast_with_format.py@37 PS5, Line 37: def run_and_expect_error(self, query, error_msg): > self.execute_query_expect_failure(self.client, query) could be used instead I've also considered using that function but then I won't be able to assert on the error text, right? -- To view, visit http://gerrit.cloudera.org:8080/12267 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ia514aaa9e8f5487d396587d5ed24c7348a492697 Gerrit-Change-Number: 12267 Gerrit-PatchSet: 6 Gerrit-Owner: Gabor Kaszab <[email protected]> Gerrit-Reviewer: Attila Jeges <[email protected]> Gerrit-Reviewer: Csaba Ringhofer <[email protected]> Gerrit-Reviewer: Gabor Kaszab <[email protected]> Gerrit-Reviewer: Greg Rahn <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Paul Rogers <[email protected]> Gerrit-Comment-Date: Wed, 13 Feb 2019 09:46:20 +0000 Gerrit-HasComments: Yes
