[Pig-dev] [jira] Created: (PIG-1563) SUBSTRING function is broken

[ https://issues.apache.org/jira/browse/PIG-1563?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12902195#action_12902195 ]

Dmitriy V. Ryaboy commented on PIG-1563:
----------------------------------------

Interesting -- this is a casting issue. If you specify name:chararray, it works. The log should be full of class cast exceptions..
Two ways to fix -- take out the classCast catch clause, or (better) write a BinSubstring EvalFunc that can do the conversions appropriately.

[ https://issues.apache.org/jira/browse/PIG-1563?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12902201#action_12902201 ]

Olga Natkovich commented on PIG-1563:
-------------------------------------

I think you just need to add the arg mapping function and the pig will insert the casts.

[ https://issues.apache.org/jira/browse/PIG-1563?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12902211#action_12902211 ]

Olga Natkovich commented on PIG-1563:
-------------------------------------

The same needs to be done (and we need unit tests) for the following string manipulation functions:

INDEXOF
LAST_INDEX_OF
REPLACE
SPLIT
TRIM

[ https://issues.apache.org/jira/browse/PIG-1563?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12902308#action_12902308 ]

Dmitriy V. Ryaboy commented on PIG-1563:
----------------------------------------

I wrote a unit test and it fails (due to classcastexceptions) even after I add

{code}
@Override
public List<FuncSpec> getArgToFuncMapping() {
List<FuncSpec> funcList = new ArrayList<FuncSpec>();
funcList.add(new FuncSpec(this.getClass().getName(),
new Schema(Lists.newArrayList(new Schema.FieldSchema(null, DataType.CHARARRAY),
new Schema.FieldSchema(null, DataType.INTEGER),
new Schema.FieldSchema(null, DataType.INTEGER)))));

return funcList;
}
{code}

to builtin.SUBSTRING

[ https://issues.apache.org/jira/browse/PIG-1563?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]

Dmitriy V. Ryaboy reassigned PIG-1563:
--------------------------------------

Assignee: Dmitriy V. Ryaboy (was: Yan Zhou)

[ https://issues.apache.org/jira/browse/PIG-1563?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]

Dmitriy V. Ryaboy updated PIG-1563:
-----------------------------------

Status: Patch Available (was: Open)
Affects Version/s: 0.8.0

[ https://issues.apache.org/jira/browse/PIG-1563?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]

Dmitriy V. Ryaboy updated PIG-1563:
-----------------------------------

Attachment: PIG_1563.patch

Updated the functions with the appropriate argToFunc mappings, added tests.
Also tried on current build of trunk to make sure all the up- and down- casting magic works, and it does.

Note the new methods in test.Utils and pig.impl.util.Utils -- pretty handy.

[ https://issues.apache.org/jira/browse/PIG-1563?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12903546#action_12903546 ]

Olga Natkovich commented on PIG-1563:
-------------------------------------

I am looking into this to see if I can make it work without double wrapping. So far I got the easy case of trim to work. Will update the JIRA once I have more results

[ https://issues.apache.org/jira/browse/PIG-1563?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12903578#action_12903578 ]

Olga Natkovich commented on PIG-1563:
-------------------------------------

I was able to make it successfully working (without wrapping) for the functions that have fixed number of arguments:

LAST_INDEX_OF
REPLACE
TRIM

I don't believe there is currently a way to make it work with variable number of args (even if the number of combinations is fixed.) Moreover, if we add the mapping table in this case, it breaks the case of typed data which is bad. This is the case with the remaining functions - INDEXOF and SPLIT.

So my suggestion is only to fix the first set of function and delay the rest to 0.9 when we fix the mapping code.

Dmitry and others, are you ok with this? If so, I can update the patch to reflect this.

[ https://issues.apache.org/jira/browse/PIG-1563?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12903636#action_12903636 ]

Dmitriy V. Ryaboy commented on PIG-1563:
----------------------------------------

Sounds good. Should we just merge in the amazon contrib for some of these?

[ https://issues.apache.org/jira/browse/PIG-1563?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12903640#action_12903640 ]

Olga Natkovich commented on PIG-1563:
-------------------------------------

which JIRA is that?

I will just get this in - I think that's all I have time today but I can look at the other one as well next week

[ https://issues.apache.org/jira/browse/PIG-1563?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12903644#action_12903644 ]

Dmitriy V. Ryaboy commented on PIG-1563:
----------------------------------------

Olga, the amazon contrib is PIG-1565

[ https://issues.apache.org/jira/browse/PIG-1563?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]

Olga Natkovich updated PIG-1563:
--------------------------------

Attachment: PIG_1563_v2.patch

[ https://issues.apache.org/jira/browse/PIG-1563?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12903744#action_12903744 ]

Olga Natkovich commented on PIG-1563:
-------------------------------------

Uploaded new patch which does the following:

(1) Adds mapping function for functions with fixed number of arguments: SUBSTRING, LAST_INDEX_OF, REPLACE,TRIM
(2) Left the rest of the functions alone which means that until 0.9 they will only work on typed data. CONCAT is in the same category
(3) Re-used applicable tests that Dmitry create, thanks!
(3) Added a couple of e2e tests to make sure that we test the mapping function as well

Please, review.

We will keep the open till we address (2) in 0.9.

[ https://issues.apache.org/jira/browse/PIG-1563?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12903753#action_12903753 ]

Dmitriy V. Ryaboy commented on PIG-1563:
----------------------------------------

+1

question/comment -- any reason you discarded the new buildSimpleFuncSpec I wrote in the first iteration of this patch? I think it simplifies the code:

{code}
funcList.add(Utils.buildSimpleFuncSpec(
this.getClass().getName(), DataType.CHARARRAY, DataType.CHARARRAY));
{code}

vs
{code}
Schema s = new Schema();
s.add(new Schema.FieldSchema(null, DataType.CHARARRAY));
s.add(new Schema.FieldSchema(null, DataType.CHARARRAY));
funcList.add(new FuncSpec(this.getClass().getName(), s));
{code}

[ https://issues.apache.org/jira/browse/PIG-1563?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12904450#action_12904450 ]

Olga Natkovich commented on PIG-1563:
-------------------------------------

Dmitry, thanks for the review. I did not discard your function - it was part of the patch. I did not change the code to use it just because I already finished testing the changes and did not have time to redo the code.

I am fixing some javadoc and release audit failures and will commit the code shortly.

[ https://issues.apache.org/jira/browse/PIG-1563?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]

Olga Natkovich updated PIG-1563:
--------------------------------

Summary: Some string functions don't work with bytearray arguments (was: SUBSTRING function is broken)

[ https://issues.apache.org/jira/browse/PIG-1563?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12904462#action_12904462 ]

Olga Natkovich commented on PIG-1563:
-------------------------------------

+1 overall.
[exec]
[exec] +1 @author. The patch does not contain any @author tags.
[exec]
[exec] +1 tests included. The patch appears to include 13 new or modified tests.
[exec]
[exec] +1 javadoc. The javadoc tool did not generate any warning messages.
[exec]
[exec] +1 javac. The applied patch does not increase the total number of javac compiler warnings.
[exec]
[exec] +1 findbugs. The patch does not introduce any new Findbugs warnings.
[exec]
[exec] +1 release audit. The applied patch does not increase the total number of release audit warnings.
[exec]

[ https://issues.apache.org/jira/browse/PIG-1563?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12904467#action_12904467 ]

Olga Natkovich commented on PIG-1563:
-------------------------------------

I made one additional change and renamed SPLIT into STRSPLIT to avoid conflict with SPLIT operator

[ https://issues.apache.org/jira/browse/PIG-1563?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]

Olga Natkovich updated PIG-1563:
--------------------------------

Attachment: PIG_1563_v3.patch

latest patch

[ https://issues.apache.org/jira/browse/PIG-1563?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]

Olga Natkovich updated PIG-1563:
--------------------------------

Status: Resolved (was: Patch Available)
Resolution: Fixed

patch committed. Thanks Dmitry for the help and review