Home | About | Sematext search-lucene.com search-hadoop.com
 Search Hadoop and all its subprojects:

Switch to Threaded View
Drill >> mail # dev >> Re: Review Request 18055: DRILL-356 : Support for Date, Time data types


Copy link to this message
-
Re: Review Request 18055: DRILL-356 : Support for Date, Time data types

This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/18055/#review36110
Looks good to me. Reviewed most of the code except compare functions for interval type.
common/src/main/java/org/apache/drill/common/expression/OutputTypeDeterminer.java
<https://reviews.apache.org/r/18055/#comment67018>

    Typo: FIXED_INTERNAL

exec/java-exec/src/main/codegen/data/ValueVectorTypes.tdd
<https://reviews.apache.org/r/18055/#comment67021>

    From the JIRA it looks like you mentioned long is used to store TIME type. Looks like int type range is sufficient, can you update the JIRA as well?

exec/java-exec/src/main/codegen/templates/DateCastFunctions.java
<https://reviews.apache.org/r/18055/#comment67022>

    minor: For readability of generated class names, can you add "to" in between ${type.form} and ${type.to}. It gets confusing when you have Date to Time* and DateTime to {type}.

exec/java-exec/src/main/codegen/templates/DateCastFunctions.java
<https://reviews.apache.org/r/18055/#comment67025>

    dateFields[0] corresponds to YYYY or we get a different format when casting to Time? Similarly indexes 1->MM, 2->DD and 3->HH

exec/java-exec/src/main/codegen/templates/DateCastFunctions.java
<https://reviews.apache.org/r/18055/#comment67026>

    throw exceptions if not in ISO format?

exec/java-exec/src/main/codegen/templates/DateCastFunctions.java
<https://reviews.apache.org/r/18055/#comment67027>

    is this output in any standard format? are we adding methods to cast into a specific format later?

exec/java-exec/src/main/codegen/templates/DateFunctions.java
<https://reviews.apache.org/r/18055/#comment67028>

    What about comparing timezone for TimeStamp? are we expecting lhs and rhs to have same timezone or is it compared somewhere else?
- Venki Korukanti
On Feb. 18, 2014, 12:30 a.m., Mehant Baid wrote: