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 4: (18 comments) http://gerrit.cloudera.org:8080/#/c/15866/3//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/15866/3//COMMIT_MSG@15 PS3, Line 15: select cast('01:02:59' as timestamp); > This should return parse error because the format itself doesn't contain da Done http://gerrit.cloudera.org:8080/#/c/15866/3//COMMIT_MSG@17 PS3, Line 17: > nit: dot at the end of a sentence. Done http://gerrit.cloudera.org:8080/#/c/15866/3//COMMIT_MSG@17 PS3, Line 17: > nit: This should be plural in my opinion as it stands for 2 separate exampl Done http://gerrit.cloudera.org:8080/#/c/15866/3//COMMIT_MSG@28 PS3, Line 28: timesta > I still don't see tests where you use a file populated by dateless timestam Done http://gerrit.cloudera.org:8080/#/c/15866/3//COMMIT_MSG@29 PS3, Line 29: are requ > nit: start a sentence with capital letter. Done http://gerrit.cloudera.org:8080/#/c/15866/3//COMMIT_MSG@29 PS3, Line 29: utpu > nit: tests Done http://gerrit.cloudera.org:8080/#/c/15866/3//COMMIT_MSG@30 PS3, Line 30: > It's just a matter of your point of view which direction you call backwards Done http://gerrit.cloudera.org:8080/#/c/15866/3/be/src/exprs/expr-test.cc File be/src/exprs/expr-test.cc: http://gerrit.cloudera.org:8080/#/c/15866/3/be/src/exprs/expr-test.cc@4507 PS3, Line 4507: 1.00 > Is this related to your change or does it come with a rebase you've made in Sorry for this one, I really did a rebase between the commits. http://gerrit.cloudera.org:8080/#/c/15866/3/be/src/exprs/expr-test.cc@7536 PS3, Line 7536: H:mm:ss' > Actually, we should get an error when parsing the format and see that there Done http://gerrit.cloudera.org:8080/#/c/15866/3/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/3/be/src/runtime/datetime-simple-date-format-parser.h@88 PS3, Line 88: > Can you re-use the CastDirection type similarly as it is used in e.g. ISO S Done http://gerrit.cloudera.org:8080/#/c/15866/3/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/3/be/src/runtime/datetime-simple-date-format-parser.cc@179 PS3, Line 179: if (cast_mode == PARSE) return (dt_ctx->has_date_toks); > Please don't leave commented code Done http://gerrit.cloudera.org:8080/#/c/15866/3/be/src/runtime/datetime-simple-date-format-parser.cc@180 PS3, Line 180: return (dt_ctx->has_date_toks || dt_ctx->has_time_toks); > This seems logically correct but not that readable. Additionally, this is n Done http://gerrit.cloudera.org:8080/#/c/15866/3/be/src/runtime/timestamp-test.cc File be/src/runtime/timestamp-test.cc: http://gerrit.cloudera.org:8080/#/c/15866/3/be/src/runtime/timestamp-test.cc@610 PS3, Line 610: (TimestampTC("yyyy-MM-dd HH:m:ss", "2020-05-11 1:24:34", false, true)) > Here the intention was to also cover the case when the minute part of the i Here we are checking the format tokens not the given string themselves which has a short minute part("yyyy-MM-dd HH:m:ss") We use the short form here because we expect it to fail. http://gerrit.cloudera.org:8080/#/c/15866/3/be/src/runtime/timestamp-test.cc@612 PS3, Line 612: (TimestampTC("yyyy-MM-dd HH:mm:s", "2020-05-11 14:24:34", false, true, true, 2020, : 05, 11, 14, 24, 34)) > Similarly to the other tests, please add another one where the second part Done http://gerrit.cloudera.org:8080/#/c/15866/3/be/src/runtime/timestamp-test.cc@679 PS3, Line 679: mestampFormatTC(1382337792, "yyy > nit: This sentence sounds a bit weird for me. Done http://gerrit.cloudera.org:8080/#/c/15866/3/be/src/runtime/timestamp-test.cc@695 PS3, Line 695: Test padding on double di > nit: same as above Done http://gerrit.cloudera.org:8080/#/c/15866/3/testdata/workloads/functional-query/queries/QueryTest/exprs.test File testdata/workloads/functional-query/queries/QueryTest/exprs.test: http://gerrit.cloudera.org:8080/#/c/15866/3/testdata/workloads/functional-query/queries/QueryTest/exprs.test@2976 PS3, Line 2976: select to_timestamp('01:01:01', 'HH:mm:ss'); > As mentioned elsewhere, this should result a parse error as the format does Done http://gerrit.cloudera.org:8080/#/c/15866/3/testdata/workloads/functional-query/queries/QueryTest/exprs.test@2981 PS3, Line 2981: select to_timestamp('01:01:01.123', 'HH:mm:ss.SSS'); > same as above 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: 4 Gerrit-Owner: Adam Tamas <ta...@cloudera.com> Gerrit-Reviewer: Adam Tamas <ta...@cloudera.com> Gerrit-Reviewer: Gabor Kaszab <gaborkas...@cloudera.com> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Comment-Date: Tue, 16 Jun 2020 13:56:06 +0000 Gerrit-HasComments: Yes
