You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@impala.apache.org by "Tim Armstrong (Code Review)" <ge...@cloudera.org> on 2019/04/02 17:49:56 UTC

[Impala-ASF-CR] IMPALA-7368: Add initial support for DATE type

Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/12481 )

Change subject: IMPALA-7368: Add initial support for DATE type
......................................................................


Patch Set 16: Code-Review+1

(1 comment)

http://gerrit.cloudera.org:8080/#/c/12481/11/be/src/exprs/conditional-functions-ir.cc
File be/src/exprs/conditional-functions-ir.cc:

http://gerrit.cloudera.org:8080/#/c/12481/11/be/src/exprs/conditional-functions-ir.cc@37
PS11, Line 37: IS_NULL_COMPUTE_
> Done
Sorry for the churn here induced by my original comment. I do think there's something worth revisiting later just as a maintainability tihng - there would be some value in infrastructure to "stamp out" these more mechanical aspects of a data type, but it requires some thought. E.g. for something like a numeric type, where there's a set of common operations and functions, it seems like we should be able to define a new numeric type in one place and have the required definitions added automatically elsewhere, rather than having to chase down all the locations in the codebase.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iea8155ef09557e0afa2f8b2d0b2dc9d0896dc30f
Gerrit-Change-Number: 12481
Gerrit-PatchSet: 16
Gerrit-Owner: Attila Jeges <at...@cloudera.com>
Gerrit-Reviewer: Attila Jeges <at...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Tue, 02 Apr 2019 17:49:56 +0000
Gerrit-HasComments: Yes