[
https://issues.apache.org/jira/browse/PHOENIX-688?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=14222339#comment-14222339
]
Gabriel Reid commented on PHOENIX-688:
--------------------------------------
I've just taken a go through the code [~aliciashu]. This looks like a good work
in progress. A few remarks:
* I noticed that the default format for time and timestamp are both "yyyy-MM-dd
hh:mm:ss". Seeing as a SQL time is actually just a time of day (and not really
a timestamp as it's modeled in Phoenix), I would suggest just using "hh:mm:ss"
as the default time format, and "yyyy-MM-dd hh:mm:ss.SSS" as the default
timestamp format (to include milliseconds). We've already got a dependency on
Joda time, so if using that makes things easier, there no reason not to do it
* I notice that the tests in TestTimeFunctions aren't actually testing
anything. They should be verifying the results of the function output. Ideally,
the following conditions should be covered by the tests
** select using default time/timestamp format
** select using a user-supplied time/timestamp format
** upsert of data using the function
** use of the function within a where clause
* The name of the test case should end with IT (i.e. the test should be called
TimeFunctionsIT). This is (probably) necessary to ensure that it's picked up
when running the full test suite within maven.
* ToTimeParseNode is a subclass of ToDateParseNode, but I don't see why. It
doesn't seem to inherit anything from its parent class. It probably is possible
to clean this up to actually inherit some stuff so that the subclass is smaller
(or just make it a subclass of FunctionParseNode.
* Code formatting: in some of the files, 2 spaces are used for indentation
instead of 4. Also, there are some wildcard imports (i.e. import java.io.*)
that should be cleaned up
[~jamestaylor] one thing I'm not totally clear on from the original description
that you wrote is the use of a custom FunctionParseNode class. Is this just
needed for configuration-based setting of the format string, or is there
another reason for this?
> Add to_time and to_timestamp built-in functions
> -----------------------------------------------
>
> Key: PHOENIX-688
> URL: https://issues.apache.org/jira/browse/PHOENIX-688
> Project: Phoenix
> Issue Type: Task
> Affects Versions: 3.0-Release
> Reporter: James Taylor
> Assignee: Alicia Ying Shu
> Attachments: PHOENIX-688.patch
>
>
> We already have a to_date function implemented by ToDateFunction, so adding a
> ToTimeFunction could be done by just deriving the class from ToDateFunction
> and changing the getDataType() to be PDataType.TIME instead.
> For a general overview on adding a new built-in function, see the phoenix
> blog
> [here](http://phoenix-hbase.blogspot.com/2013/04/how-to-add-your-own-built-in-function.html)
> The to_timestamp function would be similar as well, but in this case we'd
> want to register a new ToTimestampParseNode (very similar to
> ToDateParseNode), that uses the DateUtil.getTimestampParser(format) to create
> the timestamp instance. This class would then be defined in the
> ToTimestampFunction as the nodeClass attribute (which would cause it to be
> used to construct a ToTimestampFunction at compile time).
--
This message was sent by Atlassian JIRA
(v6.3.4#6332)
