Pig
  1. Pig
  2. PIG-3569

SUM function for BigDecimal and BigInteger

    Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Critical Critical
    • Resolution: Fixed
    • Affects Version/s: 0.12.0
    • Fix Version/s: 0.13.0
    • Component/s: internal-udfs
    • Labels:
      None
    • Patch Info:
      Patch Available
    • Hadoop Flags:
      Reviewed
    • Release Note:
      Attached PIG_3569.patch was tested with latest from trunk.

      Description

      SUM function doesn't support newly added BigDecimal data type. I've a patch to make SUM function work for BigDecimal

      1. PIG_3569_v2.patch
        25 kB
        Hariharasudhan Chinnan

        Activity

        Hide
        Hariharasudhan Chinnan added a comment -

        Patch for getting sum function to work with BigDecimal

        Show
        Hariharasudhan Chinnan added a comment - Patch for getting sum function to work with BigDecimal
        Hide
        Rohini Palaniswamy added a comment -

        Added Hari to the contributors list and assigning the jira to him. Reopening jira for reviewing.

        Hariharasudhan Chinnan,
        Welcome to the pig community. While contributing, you need to make the status as Patch Available after you put up a patch. One of the committers will review the patch and after all review comments are addressed will mark it Resolved after committing the patch to svn. Please take a look at https://cwiki.apache.org/confluence/display/PIG/HowToContribute which has all the relevant information and process documented. Feel free to mail the dev list if you have any questions.

        Show
        Rohini Palaniswamy added a comment - Added Hari to the contributors list and assigning the jira to him. Reopening jira for reviewing. Hariharasudhan Chinnan , Welcome to the pig community. While contributing, you need to make the status as Patch Available after you put up a patch. One of the committers will review the patch and after all review comments are addressed will mark it Resolved after committing the patch to svn. Please take a look at https://cwiki.apache.org/confluence/display/PIG/HowToContribute which has all the relevant information and process documented. Feel free to mail the dev list if you have any questions.
        Hide
        Rohini Palaniswamy added a comment -

        Instead of AlgebraicBigDecimalSumBase, can you create a AlgebraicBigDecimalMathBase similar to AlgebraicFloatMathBase, so that it supports MIN and MAX as well. Also can you add code for BigInteger as well?

        Show
        Rohini Palaniswamy added a comment - Instead of AlgebraicBigDecimalSumBase, can you create a AlgebraicBigDecimalMathBase similar to AlgebraicFloatMathBase, so that it supports MIN and MAX as well. Also can you add code for BigInteger as well?
        Hide
        Hariharasudhan Chinnan added a comment -

        For MIN and MAX works because the seed for MIN Double and MAX Double could be clearly defined as double.positive_infinity and double.negative_infinity. For BigDecimal, no such boundaries could be defined. More framework level changes have to be implemented first to get MAX and MIN working.

        Show
        Hariharasudhan Chinnan added a comment - For MIN and MAX works because the seed for MIN Double and MAX Double could be clearly defined as double.positive_infinity and double.negative_infinity. For BigDecimal, no such boundaries could be defined. More framework level changes have to be implemented first to get MAX and MIN working.
        Hide
        Rohini Palaniswamy added a comment -

        Thanks for the clarification. We can address that in a different jira.

        Few comments:
        1) //Memory Management? - That is not required. You can remove setting values to null as it is not a big object and java GC should handle that easily.
        2) BigDecimal d = ((BigDecimal) n).add(BigDecimal.ZERO);
        Why do you have to add zero? Isn't typecast just enough?
        3) AlgebraicBigDecimalSumBase - Can you please rename to AlgebraicBigDecimalMathBase and also correct the javadoc to reflect that, so that we can use this class to add MIN and MAX in a different jira in future.
        4) Please make a copy of that code for BigInteger as well.
        5) Please update testSUM* testcases in TestBuiltin.java for BigInteger and BigDecimal.

        Show
        Rohini Palaniswamy added a comment - Thanks for the clarification. We can address that in a different jira. Few comments: 1) //Memory Management? - That is not required. You can remove setting values to null as it is not a big object and java GC should handle that easily. 2) BigDecimal d = ((BigDecimal) n).add(BigDecimal.ZERO); Why do you have to add zero? Isn't typecast just enough? 3) AlgebraicBigDecimalSumBase - Can you please rename to AlgebraicBigDecimalMathBase and also correct the javadoc to reflect that, so that we can use this class to add MIN and MAX in a different jira in future. 4) Please make a copy of that code for BigInteger as well. 5) Please update testSUM* testcases in TestBuiltin.java for BigInteger and BigDecimal.
        Hide
        Hariharasudhan Chinnan added a comment -

        Incorporated feedback from Rohini. Added BigInteger files for SUM function. Added test cases to TestBuiltin.java. All tests execute successfully.

        Show
        Hariharasudhan Chinnan added a comment - Incorporated feedback from Rohini. Added BigInteger files for SUM function. Added test cases to TestBuiltin.java. All tests execute successfully.
        Hide
        Rohini Palaniswamy added a comment -

        +1. Committed to trunk (0.13). Thank you Hari.

        Show
        Rohini Palaniswamy added a comment - +1. Committed to trunk (0.13). Thank you Hari.
        Hide
        Hariharasudhan Chinnan added a comment -

        Thank you Rohini for your feedback and for accepting my patch. I'm all exited on my first ever patch to any open source project getting accepted. You folks are a welcoming community. I look forward to contributing more in the future.

        Show
        Hariharasudhan Chinnan added a comment - Thank you Rohini for your feedback and for accepting my patch. I'm all exited on my first ever patch to any open source project getting accepted. You folks are a welcoming community. I look forward to contributing more in the future.

          People

          • Assignee:
            Hariharasudhan Chinnan
            Reporter:
            Hariharasudhan Chinnan
          • Votes:
            0 Vote for this issue
            Watchers:
            3 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development