[
https://issues.apache.org/jira/browse/PHOENIX-1705?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=14492220#comment-14492220
]
ramkrishna.s.vasudevan commented on PHOENIX-1705:
-------------------------------------------------
I did go through the patch completely and used this to remind myself as what
all was done to support Arrays.
The test cases looks great. The different exception scenarios handled are good.
Few comments in the PArrayDataType.appendItemToArray() code.
{code}
int newOffset = offsetArrayPosition - 2 * Bytes.SIZEOF_BYTE;
{code}
Remove this and move the
{code}
int newElementPosition = offsetArrayPosition - 2 * Bytes.SIZEOF_BYTE;
{code}
instead of that. Both are same.
{code}
Arrays.fill(newArray, newElementPosition + elementLength,
newOffsetArrayPosition,
QueryConstants.SEPARATOR_BYTE);
{code}
Should we do this above. Why to fill? Incase of a bigger array like in test
case testArrayAppendFunction4() you will fill it unnecessarily right?
{code}
Bytes.putInt(newArray, newOffsetArrayPosition +
offsetArrayLength
+ Bytes.SIZEOF_INT, newOffsetArrayPosition);
Bytes.putInt(newArray, newOffsetArrayPosition +
offsetArrayLength + 2
* Bytes.SIZEOF_INT, arrayLength);
Bytes.putByte(newArray, newOffsetArrayPosition +
offsetArrayLength + 3
* Bytes.SIZEOF_INT, arrayBytes[offset + length - 1]);
{code}
The above piece of code in useInt block can be converted to a private method so
that the similar piece of code in else block could use it but just pass
Bytes.SIZE_OFSHORT or Bytes.SIZE_OFINT based on it.
For the convertToInt case
{code}
int[] offsetArray = new int[Math.abs(arrayLength) - 1];
for (int arrayIndex = 0; arrayIndex < offsetArray.length;
arrayIndex++) {
int currOffset =
getOffset(arrayBytes, arrayIndex, true,
offsetArrayPosition);
offsetArray[arrayIndex] = currOffset;
}
int off = newOffsetArrayPosition;
for (int pos : offsetArray) {
Bytes.putInt(newArray, off, pos);
off += Bytes.SIZEOF_INT;
}
{code}
I think we can just have one loop right? Iterate thro all the elements in the
original offset array and then move those elements to the new Offset array as
Ints? You need not have two loops i think where you store to one temp array and
again move to the actual array.
I check out all the test cases looks find. The change to Wherecompiler where
the pad() is changed - do we have test case for that here?
I think if the above comments are addressed and I think James is also fine with
the other changes we can commit this. Thanks [~Dumindux].
> 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_function10.patch,
> PHOENIX-1705_implement_ARRAY_APPEND_built_in_function11.patch,
> PHOENIX-1705_implement_ARRAY_APPEND_built_in_function12.patch,
> PHOENIX-1705_implement_ARRAY_APPEND_built_in_function2.patch,
> PHOENIX-1705_implement_ARRAY_APPEND_built_in_function3.patch,
> PHOENIX-1705_implement_ARRAY_APPEND_built_in_function4.patch,
> PHOENIX-1705_implement_ARRAY_APPEND_built_in_function5.patch,
> PHOENIX-1705_implement_ARRAY_APPEND_built_in_function6.patch,
> PHOENIX-1705_implement_ARRAY_APPEND_built_in_function7.patch,
> PHOENIX-1705_implement_ARRAY_APPEND_built_in_function8.patch,
> PHOENIX-1705_implement_ARRAY_APPEND_built_in_function9.patch
>
>
--
This message was sent by Atlassian JIRA
(v6.3.4#6332)
