We should really control this by having a static factory method in the
expression class and making the other one private. It's not supported to
construct a RoundTimestampExpression with a timeunit of other than
MILLISECONDS (it really makes no sense). We can have other constructors for
RoundTimestampExpression for the convenience of RowValueConstructor, but
they shouldn't allow a List<Expression>. Better if they just take a single
Expression and let the constructor do the right thing. That way, you're not
leaking the implementation outside the class.



On Fri, Dec 13, 2013 at 12:30 PM, Samarth Jain <notifications@github.com>wrote:

> In
> src/main/java/com/salesforce/phoenix/expression/function/CeilTimestampExpression.java:
>
> >      }
> >
> >      @Override
> >      public boolean evaluate(Tuple tuple, ImmutableBytesWritable ptr) {
> > -        Expression child =  getChild();
> > -        if (child.evaluate(tuple, ptr)) {
> > -            PDataType childType = child.getDataType();
> > -            childType.coerceBytes(ptr, childType, child.getColumnModifier(), null);
> > -            BigDecimal value = (BigDecimal) childType.toObject(ptr);
> > -            value = value.round(getMathContext());
> > -            byte[] b = childType.toBytes(value, child.getColumnModifier());
> > -            ptr.set(b);
> > +        if (children.get(0).evaluate(tuple, ptr)) {
>
> Sorry, I meant CEIL. We do support CEIL(timestamp, day, 1).
>
> One can construct CeilTimestampExpression or RoundTimestampExpression by
> not going through the built in functions CEIL or ROUND. For ex - in case of
> RowValueConstructorExpression when we need to do the CEIL on timestamp.
> While going through the RoundParseNode, we can look at data types and the
> timeunit value to decide which expression we should instantiate. Like this:
>
>
> if(firstChildDataType == PDataType.TIMESTAMP) {
> if(tu == TimeUnit.MILLISECOND) {
> return new RoundTimestampExpression(children);
> }
> return new RoundDateExpression(children);
> }
>
> But when one is instantiating a RoundTimestamp or a CeilTimestamp
> expression directly, we would need to support all the time units.
>
> —
> Reply to this email directly or view it on GitHub<https://github.com/forcedotcom/phoenix/pull/623/files#r8344170>
> .
>