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

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


Patch Set 1:

(5 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 
postgres, if you haven't already, just to be extra sure we get it right this 
time around.


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


http://gerrit.cloudera.org:8080/#/c/9508/1/be/src/exprs/udf-builtins.cc@145
PS1, Line 145:   unsigned short y = orig_date.year();
maybe add a quick comment explaining this, from your commit message:
// First year of current millennium is 2001.


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

y = (y - 1) / 1000 * 1000 + 1


http://gerrit.cloudera.org:8080/#/c/9508/1/be/src/exprs/udf-builtins.cc@163
PS1, Line 163:   }
same comments



--
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: 1
Gerrit-Owner: Attila Jeges <atti...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dhe...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <tarmstr...@cloudera.com>
Gerrit-Comment-Date: Tue, 06 Mar 2018 17:32:36 +0000
Gerrit-HasComments: Yes

Reply via email to