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

Reply via email to