Attila Jeges has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/9508 )

Change subject: IMPALA-6606: date_trunc() misinterprets MILLENNIUM/CENTURY 
precision
......................................................................


Patch Set 2:

(6 comments)

http://gerrit.cloudera.org:8080/#/c/9508/1/be/src/exprs/expr-test.cc
File be/src/exprs/expr-test.cc:

http://gerrit.cloudera.org:8080/#/c/9508/1/be/src/exprs/expr-test.cc@8351
PS1, Line 8351:       TimestampValue::Parse("2110-01-01 00:00:00"));
> please double check the behavior of DECADE, CENTURY and MILLENNIUM against
Done


http://gerrit.cloudera.org:8080/#/c/9508/1/be/src/exprs/expr-test.cc@8351
PS1, Line 8351:       TimestampValue::Parse("2110-01-01 00:00:00"));
> We already depend on PostgreSQL in development environments because of the
We could have some tests like that, but we would still have to test the "valid 
input - invalid output" cases separately as the range of TIMESTAMP values 
supported by PostgreSQL and Impala is different. The SQL syntax they use is 
also slightly different (PostgreSQL needs explicit type cast to TIMESTAMP).


http://gerrit.cloudera.org:8080/#/c/9508/1/be/src/exprs/udf-builtins.cc
File be/src/exprs/udf-builtins.cc:

http://gerrit.cloudera.org:8080/#/c/9508/1/be/src/exprs/udf-builtins.cc@144
PS1, Line 144: n
> missing an 'n' in spelling of Millennium
Done


http://gerrit.cloudera.org:8080/#/c/9508/1/be/src/exprs/udf-builtins.cc@145
PS1, Line 145:   DCHECK_GT(orig_date.year(), 2000);
> maybe add a quick comment explaining this, from your commit message:
Done


http://gerrit.cloudera.org:8080/#/c/9508/1/be/src/exprs/udf-builtins.cc@150
PS1, Line 150: }
> I think this handles both cases?
It does, thanks.


http://gerrit.cloudera.org:8080/#/c/9508/1/be/src/exprs/udf-builtins.cc@163
PS1, Line 163:   date new_date(orig_date.year() / 10 * 10, 1, 1);
> same comments
Done



--
To view, visit http://gerrit.cloudera.org:8080/9508
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I4f4aac17875dbf17dcabff25473ffeb006fce20b
Gerrit-Change-Number: 9508
Gerrit-PatchSet: 2
Gerrit-Owner: Attila Jeges <atti...@cloudera.com>
Gerrit-Reviewer: Attila Jeges <atti...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dhe...@cloudera.com>
Gerrit-Reviewer: Philip Zeyliger <phi...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <tarmstr...@cloudera.com>
Gerrit-Comment-Date: Tue, 06 Mar 2018 19:58:46 +0000
Gerrit-HasComments: Yes

Reply via email to