Attila Jeges has posted comments on this change. ( http://gerrit.cloudera.org:8080/13722 )
Change subject: IMPALA-8703: ISO:SQL:2016 datetime patterns - Milestone 1 ...................................................................... Patch Set 2: (7 comments) http://gerrit.cloudera.org:8080/#/c/13722/2/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/13722/2/fe/src/main/java/org/apache/impala/analysis/CastExpr.java@51 PS2, Line 51: private final String castFormat_; Add a comment to describe the new member. http://gerrit.cloudera.org:8080/#/c/13722/2/fe/src/main/java/org/apache/impala/analysis/CastExpr.java@208 PS2, Line 208: castFormat_ What if 'castFormat_' includes an apostrophe? http://gerrit.cloudera.org:8080/#/c/13722/2/fe/src/main/java/org/apache/impala/analysis/CastExpr.java@226 PS2, Line 226: boolean isIntermediateStep = : type_.getPrimitiveType() == PrimitiveType.CHAR && : children_.get(0).getType().getPrimitiveType() == PrimitiveType.STRING; The naming of this variable is a bit confusing. 'isIntermediateStep' is set to true even if we have a simple CAST(string_col AS char(20)) expression. I would just put the right-hand side directly into the if condition. http://gerrit.cloudera.org:8080/#/c/13722/2/fe/src/main/java/org/apache/impala/analysis/CastExpr.java@280 PS2, Line 280: if (null != castFormat_ && !isIntermediateStepNeeded) { : if (!(type_.isDateOrTimeType() && getChild(0).getType().isStringType()) && : !(type_.isStringType() && getChild(0).getType().isDateOrTimeType())) { : // FORMAT clause works only for casting between date types and string types : throw new AnalysisException("FORMAT clause is not applicable from " + : getChild(0).getType() + " to " + type_); : } : if (castFormat_.isEmpty()) { : throw new AnalysisException("FORMAT clause can't be empty"); : } : } This block can be used as the else branch of he previous 'if' statement. And then you don't need to introduce the 'isIntermediateStepNeeded' variable. http://gerrit.cloudera.org:8080/#/c/13722/2/testdata/workloads/functional-query/queries/QueryTest/date.test File testdata/workloads/functional-query/queries/QueryTest/date.test: http://gerrit.cloudera.org:8080/#/c/13722/2/testdata/workloads/functional-query/queries/QueryTest/date.test@629 PS2, Line 629: #==== : #---- QUERY : #select cast("2014/-; ,11'/:05" as date format "YYYY-MM-DD"); : #---- RESULTS : #2014-11-05 : #---- TYPES : #DATE This should work right? Any reason it is commented out? http://gerrit.cloudera.org:8080/#/c/13722/2/testdata/workloads/functional-query/queries/QueryTest/date.test@841 PS2, Line 841: Extra space http://gerrit.cloudera.org:8080/#/c/13722/2/tests/query_test/test_cast_with_format.py File tests/query_test/test_cast_with_format.py: http://gerrit.cloudera.org:8080/#/c/13722/2/tests/query_test/test_cast_with_format.py@22 PS2, Line 22: TestCastWithFormat Could the tests in this script be moved to cast_format.test? Or to another '.test' file? -- To view, visit http://gerrit.cloudera.org:8080/13722 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I19d8d097a45ae6f103b6cd1b2d81aad38dfd9e23 Gerrit-Change-Number: 13722 Gerrit-PatchSet: 2 Gerrit-Owner: Gabor Kaszab <gaborkas...@cloudera.com> Gerrit-Reviewer: Attila Jeges <atti...@cloudera.com> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Comment-Date: Wed, 03 Jul 2019 12:50:07 +0000 Gerrit-HasComments: Yes
