[
https://issues.apache.org/jira/browse/PHOENIX-1705?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=14364506#comment-14364506
]
James Taylor commented on PHOENIX-1705:
---------------------------------------
Looking good, [~Dumindux]. Would you mind reviewing, [~samarthjain]? Here's
some minor feedback:
- Add local variables for getting the PDataType rather than repeating these
long expressions here:
{code}
+ public ArrayAppendFunction(List<Expression> children) throws
TypeMismatchException {
+
+ super(children);
+
if(!children.get(1).getDataType().equals(PDataType.fromTypeId(children.get(0).getDataType().getSqlType()
+ - PDataType.ARRAY_TYPE_BASE))){
{code}
- Remove any System.out.println from code.
- Move this code from evaluate to the top of the method implementation - no
need to do a bunch of work beforehand if you're just going to return anyway:
{code}
+ if (!arrayExpr.evaluate(tuple, ptr)) {
+ return false;
+ } else if (ptr.getLength() == 0) {
+ return true;
+ }
{code}
- Not sure I understand this code from evaluate. What's that for?
{code}
+ if (baseType.isFixedWidth())
+ {
+ int
arrayElementLength=getMaxLength()==null?baseType.getByteSize():getMaxLength();
+
+ if(ptr.getLength()>arrayElementLength)
+ {
+ return false;
+ }
+
+ }
{code}
- Take a look at the SignFunctionTest added by [~shuxi0ng] for a good way to
get test coverage by directly instantiating your built-in function. These tests
will run much faster than end2end tests, as we don't need a mini cluster to be
spun up.
- Make sure to rebase your patch on the latest, as ExpressionType has changed
with some new built-in function implementations.
- The test I referred to before would test when the offsets we store for a
variable length array can no longer fit in a short. You'd want to build up a
big array and then call append on it at the point where this would occur. Doing
this in a lower level unit test (i.e. not end2end) would make the most sense,
to test your PArrayDataType.appendItemToArray() function directly. It's the
logic that determines the useInt local variable in that method.
> implement ARRAY_APPEND built in function
> ----------------------------------------
>
> Key: PHOENIX-1705
> URL: https://issues.apache.org/jira/browse/PHOENIX-1705
> Project: Phoenix
> Issue Type: Sub-task
> Reporter: Dumindu Buddhika
> Assignee: Dumindu Buddhika
> Attachments:
> PHOENIX-1705_implement_ARRAY_APPEND_built_in_function.patch,
> PHOENIX-1705_implement_ARRAY_APPEND_built_in_function.patch,
> PHOENIX-1705_implement_ARRAY_APPEND_built_in_function1.patch,
> PHOENIX-1705_implement_ARRAY_APPEND_built_in_function2.patch,
> PHOENIX-1705_implement_ARRAY_APPEND_built_in_function3.patch
>
>
--
This message was sent by Atlassian JIRA
(v6.3.4#6332)
