Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/7009 )
Change subject: IMPALA-5315: Cast to timestamp fails for YYYY-M-D format ...................................................................... Patch Set 16: (5 comments) I think this should be the last round of comments. http://gerrit.cloudera.org:8080/#/c/7009/16/be/src/exprs/expr-test.cc File be/src/exprs/expr-test.cc: http://gerrit.cloudera.org:8080/#/c/7009/16/be/src/exprs/expr-test.cc@6176 PS16, Line 6176: TestStringValue("monthname(cast('2011-02-18 09:10:11.000000' as timestamp))", "February"); nit: long lines. It might be easier to run the patch through clang-format to fix some of these whitespace and include ordering issues: https://cwiki.apache.org/confluence/pages/viewpage.action?pageId=65868536 http://gerrit.cloudera.org:8080/#/c/7009/16/be/src/runtime/timestamp-parse-util.cc File be/src/runtime/timestamp-parse-util.cc: http://gerrit.cloudera.org:8080/#/c/7009/16/be/src/runtime/timestamp-parse-util.cc@28 PS16, Line 28: #include <algorithm> Standard C++ headers go before the 3rd-party library headers. I.e. line 19 above. http://gerrit.cloudera.org:8080/#/c/7009/16/testdata/workloads/functional-query/queries/QueryTest/cast-lazy-datetime-string.test File testdata/workloads/functional-query/queries/QueryTest/cast-lazy-datetime-string.test: http://gerrit.cloudera.org:8080/#/c/7009/16/testdata/workloads/functional-query/queries/QueryTest/cast-lazy-datetime-string.test@3 PS16, Line 3: select cast(dt_str as timestamp) from lazy_dt_str should not need a cast here, we want to test the text scanner parsing the datetime column directly http://gerrit.cloudera.org:8080/#/c/7009/16/testdata/workloads/functional-query/queries/QueryTest/cast-lazy-datetime-string.test@4 PS16, Line 4: ---- RESULTS Let's make this RESULTS: VERIFY_IS_EQUAL_SORTED We don't guarantee the order of returned rows without an order by clause (although in practice it will be in the file order for this query plan). http://gerrit.cloudera.org:8080/#/c/7009/16/tests/query_test/test_scanners.py File tests/query_test/test_scanners.py: http://gerrit.cloudera.org:8080/#/c/7009/16/tests/query_test/test_scanners.py@874 PS16, Line 874: STRING This should be a timestamp column, so we can make sure that the text scanner does the conversion correctly. -- To view, visit http://gerrit.cloudera.org:8080/7009 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ib9a184a09d7e7783f04d47588537612c2ecec28f Gerrit-Change-Number: 7009 Gerrit-PatchSet: 16 Gerrit-Owner: Vincent Tran <vtt...@cloudera.com> Gerrit-Reviewer: Dan Hecht <dhe...@cloudera.com> Gerrit-Reviewer: Lars Volker <l...@cloudera.com> Gerrit-Reviewer: Tim Armstrong <tarmstr...@cloudera.com> Gerrit-Reviewer: Vincent Tran <vtt...@cloudera.com> Gerrit-Comment-Date: Mon, 12 Mar 2018 21:15:51 +0000 Gerrit-HasComments: Yes
