Mailing-List: contact pig-dev-help@hadoop.apache.org; run by ezmlm
Precedence: bulk
Reply-To: pig-dev@hadoop.apache.org
Message-ID: <223955454.1249485794779.JavaMail.jira@brutus>
Date: Wed, 5 Aug 2009 08:23:14 -0700 (PDT)
From: "Dmitriy V. Ryaboy (JIRA)" <jira@apache.org>
To: pig-dev@hadoop.apache.org
Subject: [jira] Commented: (PIG-893) support cast of chararray to other
 simple types
In-Reply-To: <1110471862.1248197294829.JavaMail.jira@brutus>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: 7bit


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

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

Jeff,
Thanks for the contribution!
Just a few comments:

0) could you name your patch files *.patch? That makes them easier to review, as the proper highlighting mode is chosen.

1) Other class names in the utils package imply that the class name for this should be CastUtils

2) Spacing in POCast.java is a bit messed up. Please make sure all spacing is to project conventions

3) In TestSchema -- Numberic isn't a word, you mean "Numeric" (no "b")

4) I am not sure about naming the methods chararrayToxxxx . Since they take String as an argument, being in Java-land, I think it would be more straightforward to say stringToxxx .

5) Implementation of the casts -- you call str.toBytes(), and hand off to bytesToXXX  method. That method, in turn, converts bytes back into a string, and proceeds to do the conversion. That seems like redundant work. Wouldn't it be better to have stringToXXX peform the conversion, and have bytesToXXX covert to string, then call the stringToXXX method?

6)  TestCharArray2Numeric.java -- the convention is to spell out "To" instead of using the number 2

7)  The tests in TestCharArray2Numeric look very similar to each other. Could you pull out the common functionality so the code is only repeated once?  
About the tests themselves:  Since you are just testing conversions, this can be a straightforward unit test -- make a few strings, assert that they convert to the expected value. Hit the edge cases (overflows, special cases for parsing, etc).  We don't need to spin up a whole Pig query.

8) I don't like testing random values, as this creates tests that might sometimes pass, and sometimes not. Recommend using known data for reproducible test results.

9) You extracted functionality from Utf8StorageConverter by duplicating the code; I would prefer to see Utf8StorageConverter modified to hand off conversions to CastUtils


> support cast of chararray to other simple types
> -----------------------------------------------
>
>                 Key: PIG-893
>                 URL: https://issues.apache.org/jira/browse/PIG-893
>             Project: Pig
>          Issue Type: New Feature
>    Affects Versions: 0.4.0
>            Reporter: Thejas M Nair
>            Assignee: Jeff Zhang
>             Fix For: 0.4.0
>
>         Attachments: Pig_893_Patch.txt
>
>
> Pig should support casting of chararray to integer,long,float,double,bytearray. If the conversion fails for reasons such as overflow, cast should return null and log a warning.

-- 
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.