[jira] [Comment Edited] (PHOENIX-2018) Implement math build-in function SQRT

["James Taylor (JIRA)" <ji...@apache.org>]

    [ https://issues.apache.org/jira/browse/PHOENIX-2018?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=14570149#comment-14570149 ] 

James Taylor edited comment on PHOENIX-2018 at 6/3/15 2:22 AM:
---------------------------------------------------------------

Thanks for the patch, [~shuxi0ng]. It looks very good. Here's some minor feedback:
- In your evaluate method, check for the child evaluating to null (which you can check with ptr.getLength() == 0), and in that case, just return true.
- Remove that if (ptr.getLength() < returnType.getByteSize()) statement. You definitely always need to allocate a new buffer for the return value, otherwise you're potentially overriding some random memory location (based on what the child returned).
- Don't bother coercing the value after the Math.sqrt call. Instead, just don't specialize the getSortOrder() method (which will end up defaulting to SortOrder.ASC).
{code}
+    @Override
+    public boolean evaluate(Tuple tuple, ImmutableBytesWritable ptr) {
+        Expression childExpr = children.get(0);
+        PDataType returnType = getDataType();
+        if (childExpr.evaluate(tuple, ptr)) {
+            double result;
+            if (childExpr.getDataType() == PDecimal.INSTANCE) {
+                result =
+                        ((BigDecimal) childExpr.getDataType().toObject(ptr,
+                            childExpr.getSortOrder())).doubleValue();
+            } else {
+                result =
+                        childExpr.getDataType().getCodec()
+                                .decodeDouble(ptr, childExpr.getSortOrder());
+            }
+            if (ptr.getLength() < returnType.getByteSize()) {
+                ptr.set(new byte[returnType.getByteSize()]);
+            }
+            returnType.getCodec().encodeDouble(Math.sqrt(result), ptr);
+            returnType.coerceBytes(ptr, returnType, SortOrder.ASC, childExpr.getSortOrder());
+            return true;
+        }
+        return false;
+    }
+
+    @Override
+    public PDataType getDataType() {
+        return PDouble.INSTANCE;
+    }
+
+    @Override
+    public String getName() {
+        return NAME;
+    }
+
+    @Override
+    public OrderPreserving preservesOrder() {
+        return OrderPreserving.YES;
+    }
+
+    @Override
+    public SortOrder getSortOrder() {
+        return children.get(0).getSortOrder();
+    }
{code}


was (Author: jamestaylor):
Thanks for the patch, [~shuxi0ng]. It looks very good. Here's some minor feedback:
- In your evaluate method, check for the child evaluating to null (which you can check with ptr.getLength() == 0), and in that case, just return true.
- Remove that if (ptr.getLength() < returnType.getByteSize()) statement. You definitely always need to allocate a new buffer for the return value, otherwise you're potentially overriding some random memory location (based on what the child returned).
- Don't bother coercing the value after the Math.sqrt call. Instead, just don't specialize the getSortOrder() method (which will end up defaulting to SortOrder.ASC).
{code}
+    @Override
+    public boolean evaluate(Tuple tuple, ImmutableBytesWritable ptr) {
+        Expression childExpr = children.get(0);
+        PDataType returnType = getDataType();
+        if (childExpr.evaluate(tuple, ptr)) {
+            double result;
+            if (childExpr.getDataType() == PDecimal.INSTANCE) {
+                result =
+                        ((BigDecimal) childExpr.getDataType().toObject(ptr,
+                            childExpr.getSortOrder())).doubleValue();
+            } else {
+                result =
+                        childExpr.getDataType().getCodec()
+                                .decodeDouble(ptr, childExpr.getSortOrder());
+            }
+            if (ptr.getLength() < returnType.getByteSize()) {
+                ptr.set(new byte[returnType.getByteSize()]);
+            }
+            returnType.getCodec().encodeDouble(Math.sqrt(result), ptr);
+            returnType.coerceBytes(ptr, returnType, SortOrder.ASC, childExpr.getSortOrder());
+            return true;
+        }
+        return false;
+    }
{code}
+
+    @Override
+    public PDataType getDataType() {
+        return PDouble.INSTANCE;
+    }
+
+    @Override
+    public String getName() {
+        return NAME;
+    }
+
+    @Override
+    public OrderPreserving preservesOrder() {
+        return OrderPreserving.YES;
+    }
+
+    @Override
+    public SortOrder getSortOrder() {
+        return children.get(0).getSortOrder();
+    }
{code}

> Implement math build-in function SQRT
> -------------------------------------
>
>                 Key: PHOENIX-2018
>                 URL: https://issues.apache.org/jira/browse/PHOENIX-2018
>             Project: Phoenix
>          Issue Type: Sub-task
>            Reporter: Shuxiong Ye
>            Assignee: Shuxiong Ye
>             Fix For: 5.0.0, 4.5.0, 4.4.1
>
>         Attachments: 0001-PHOENIX-2018-Implement-math-build-in-function-SQRT_v3.patch
>
>
> # SQRT means square root.
> # Return type will be PDouble
> # OrderPreserving



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)