Adam Tamas has posted comments on this change. ( http://gerrit.cloudera.org:8080/15866 )
Change subject: IMPALA-9531: Dropped support for dateless timestamps ...................................................................... Patch Set 2: (19 comments) http://gerrit.cloudera.org:8080/#/c/15866/1//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/15866/1//COMMIT_MSG@7 PS1, Line 7: d > no need for quotation marks Done http://gerrit.cloudera.org:8080/#/c/15866/1//COMMIT_MSG@10 PS1, Line 10: During dateless timestamp casts if the format doesn't contain : date part we get an error during tokenization of the format > I'd rather mention that when a format is provided for a cast then if the fo Done http://gerrit.cloudera.org:8080/#/c/15866/1//COMMIT_MSG@14 PS1, Line 14: s: > I'd use this example: Done http://gerrit.cloudera.org:8080/#/c/15866/1//COMMIT_MSG@15 PS1, Line 15: estam > I'd use an example as '01:02:59' Done http://gerrit.cloudera.org:8080/#/c/15866/1//COMMIT_MSG@16 PS1, Line 16: select cast('01:02:59' as timestamp); > I'd also add an example for to_timestamp(input_str, format_str) Done http://gerrit.cloudera.org:8080/#/c/15866/1//COMMIT_MSG@17 PS1, Line 17: This will come back as NULL values > Another example: Done http://gerrit.cloudera.org:8080/#/c/15866/1//COMMIT_MSG@23 PS1, Line 23: select cast('01:02:59' as timestamp format 'HH12:MI:SS'); : select cast('12 AM' as timestamp FORMAT 'AM.HH12'); : These will come back with a parsing error which is: : ERROR: PARSE ERROR: No date tokens provid > No need to list the modified test files here. If there is something in part Done http://gerrit.cloudera.org:8080/#/c/15866/1/be/src/exprs/expr-test.cc File be/src/exprs/expr-test.cc: http://gerrit.cloudera.org:8080/#/c/15866/1/be/src/exprs/expr-test.cc@6777 PS1, Line 6777: TestValue("dayofweek(cast('2011-12-22' as timestamp))", TYPE_INT, 5); : TestValue("dayofweek(cast('2011-12-24' as timestamp))", TYPE_INT, 7); : TestStringValue( : "to_date(cast('2011-12-22 09:10:11.12345678' as timestamp))", "2011-12-22"); : > these are the same tests as in L6759-6762. No need to duplicate them. Done http://gerrit.cloudera.org:8080/#/c/15866/1/be/src/exprs/expr-test.cc@7155 PS1, Line 7155: : TestStringValue( > These 2 tests lost their purpose with your change, A comment in L7151 refer If we call trunc() on timestamp is gives back everything that stands before that value and the value that stands in the given place, as I see we can no longer create a valid test which will give back NULL value since we dropped the timeless timestamps. I will remove these tests since they are indeed misleading and there are other test case where we trunc() a NULL value. http://gerrit.cloudera.org:8080/#/c/15866/1/be/src/runtime/datetime-iso-sql-format-tokenizer.cc File be/src/runtime/datetime-iso-sql-format-tokenizer.cc: http://gerrit.cloudera.org:8080/#/c/15866/1/be/src/runtime/datetime-iso-sql-format-tokenizer.cc@195 PS1, Line 195: > I'd move this check after L-197-202. We exit there if cast_mode_ == FORMAT Done http://gerrit.cloudera.org:8080/#/c/15866/1/be/src/runtime/datetime-simple-date-format-parser.h File be/src/runtime/datetime-simple-date-format-parser.h: http://gerrit.cloudera.org:8080/#/c/15866/1/be/src/runtime/datetime-simple-date-format-parser.h@77 PS1, Line 77: DEFAULT_SHORT_DATE_T > Is this needed anymore? Done http://gerrit.cloudera.org:8080/#/c/15866/1/be/src/runtime/datetime-simple-date-format-parser.cc File be/src/runtime/datetime-simple-date-format-parser.cc: http://gerrit.cloudera.org:8080/#/c/15866/1/be/src/runtime/datetime-simple-date-format-parser.cc@38 PS1, Line 38: DEFAULT_SHORT_DATE_T > Is this still needed? Done http://gerrit.cloudera.org:8080/#/c/15866/1/be/src/runtime/datetime-simple-date-format-parser.cc@101 PS1, Line 101: bool accept_time_toks, bool parse) { > It would be nice to see tests for to_timestamp as well. I haven't seem any I looked around a bit and as I seen there was no test for to_timestamp() with dateless conversions, but I tried it out with manual tests and it is no longer accepting dateless formats. I will look around and add a failing test with dateless timestamp to the to_timestamp() tests. http://gerrit.cloudera.org:8080/#/c/15866/1/be/src/runtime/datetime-simple-date-format-parser.cc@344 PS1, Line 344: // Check if this string starts with a date > I don't think that this check is needed. Done http://gerrit.cloudera.org:8080/#/c/15866/1/be/src/runtime/timestamp-test.cc File be/src/runtime/timestamp-test.cc: http://gerrit.cloudera.org:8080/#/c/15866/1/be/src/runtime/timestamp-test.cc@a617 PS1, Line 617: : : : : : : : : : : : : : > These still make sense, you should fix them to work with your patch rather Done http://gerrit.cloudera.org:8080/#/c/15866/1/be/src/runtime/timestamp-test.cc@a688 PS1, Line 688: : > If I understand this test correctly, here the direction is TS to string so Done http://gerrit.cloudera.org:8080/#/c/15866/1/be/src/runtime/timestamp-test.cc@a704 PS1, Line 704: : > Same as above. Done http://gerrit.cloudera.org:8080/#/c/15866/1/testdata/workloads/functional-query/queries/QueryTest/date.test File testdata/workloads/functional-query/queries/QueryTest/date.test: http://gerrit.cloudera.org:8080/#/c/15866/1/testdata/workloads/functional-query/queries/QueryTest/date.test@a604 PS1, Line 604: > Have you checked if this error message/error handling is used anywhere else Done http://gerrit.cloudera.org:8080/#/c/15866/1/testdata/workloads/functional-query/queries/QueryTest/exprs.test File testdata/workloads/functional-query/queries/QueryTest/exprs.test: http://gerrit.cloudera.org:8080/#/c/15866/1/testdata/workloads/functional-query/queries/QueryTest/exprs.test@a2969 PS1, Line 2969: : : : : : : : : : : : : : > Did you leave a test somewhere to cover the case when we get a null for dat Done -- To view, visit http://gerrit.cloudera.org:8080/15866 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I48c49bf027cc4b917849b3d58518facba372b322 Gerrit-Change-Number: 15866 Gerrit-PatchSet: 2 Gerrit-Owner: Adam Tamas <ta...@cloudera.com> Gerrit-Reviewer: Adam Tamas <ta...@cloudera.com> Gerrit-Reviewer: Gabor Kaszab <gaborkas...@cloudera.com> Gerrit-Comment-Date: Mon, 25 May 2020 09:00:25 +0000 Gerrit-HasComments: Yes
