Csaba Ringhofer has posted comments on this change. ( http://gerrit.cloudera.org:8080/12267 )
Change subject: IMPALA-4018 Part1: Add FORMAT clause in CAST() ...................................................................... Patch Set 5: (4 comments) 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 the FunctionContext* after reinterpret casting it in the close function. 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 THREAD_LOCAL scope, while it is only used as FRAGMENT_LOCAL, because later operations won't modify it. So I think that we should return after calling the parents OpenEvaluator if the scope is not FRAGMENT_LOCAL. If this change is done, then CloseEvaluator should be also modified to touch function state only if the scope is FRAGMENT_LOCAL. 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 ;? 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 of the try - catch. -- 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: 5 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: Mon, 11 Feb 2019 11:42:00 +0000 Gerrit-HasComments: Yes
