Hive
  1. Hive
  2. HIVE-6327

A few mathematic functions don't take decimal input

    Details

    • Type: Improvement Improvement
    • Status: Resolved
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 0.11.0, 0.12.0
    • Fix Version/s: 0.13.0
    • Component/s: None
    • Labels:
      None
    • Release Note:
      Wiki updated.

      Description

      A few mathematical functions, such as sin() cos(), etc. don't take decimal as argument.

      hive> show tables;
      OK
      Time taken: 0.534 seconds
      hive> create table test(d decimal(5,2));
      OK
      Time taken: 0.351 seconds
      hive> select sin(d) from test;
      FAILED: SemanticException [Error 10014]: Line 1:7 Wrong arguments 'd': No matching method for class org.apache.hadoop.hive.ql.udf.UDFSin with (decimal(5,2)). Possible choices: _FUNC_(double)  
      

      HIVE-6246 covers only sign() function. The remaining ones, including sin, cos, tan, asin, acos, atan, exp, ln, log, log10, log2, radians, and sqrt. These are non-generic UDFs.

      1. HIVE-6327.1.patch
        22 kB
        Xuefu Zhang
      2. HIVE-6327.patch
        22 kB
        Xuefu Zhang

        Issue Links

          Activity

          Hide
          Jason Dere added a comment -

          Maybe the function signature matching for non-generic UDFs should allow decimal args to be converted to double for these UDFs? Would be a more general fix as opposed to having to patch each of these UDFs.

          Show
          Jason Dere added a comment - Maybe the function signature matching for non-generic UDFs should allow decimal args to be converted to double for these UDFs? Would be a more general fix as opposed to having to patch each of these UDFs.
          Hide
          Xuefu Zhang added a comment -

          Good thinking. I'm not sure what you have in mind, but my work is generic for those mathematical functions where decimal can be treated as double. In general though, this is not true: we cannot just treat decimal as double in every case. The other direction is not desirable either: we cannot implicitly convert double to decimal as hive type promotion hierarchy suggests.

          Show
          Xuefu Zhang added a comment - Good thinking. I'm not sure what you have in mind, but my work is generic for those mathematical functions where decimal can be treated as double. In general though, this is not true: we cannot just treat decimal as double in every case. The other direction is not desirable either: we cannot implicitly convert double to decimal as hive type promotion hierarchy suggests.
          Hide
          Jason Dere added a comment -

          In general though, this is not true: we cannot just treat decimal as double in every case.

          Is this true? I thought the point of FunctionRegistry.getMethodInternal() was to find best matching method signature given the arguments, while doing type conversion if necessary. I would think that double/decimal would be a good match for one another, if there isn't a signature that matches the param exactly.

          Show
          Jason Dere added a comment - In general though, this is not true: we cannot just treat decimal as double in every case. Is this true? I thought the point of FunctionRegistry.getMethodInternal() was to find best matching method signature given the arguments, while doing type conversion if necessary. I would think that double/decimal would be a good match for one another, if there isn't a signature that matches the param exactly.
          Hide
          Xuefu Zhang added a comment -

          I'm not talking about method matching. I'm talking about data type in general. Double data and decimal data are intrinsically different, and Hive shouldn't implicit convert from one to the other. An an example, let's say there is a UDF just returns the input and it only has "double evaluate(double input)" defined. For this, if user passes decimal input to the UDF, hoping to get its decimal number back. In this case, I'd argue that Hive should return an error rather than converting the input to a double and return it. I'd also contend the reverse is also true. GenericUDF overcomes the shortcomings of UDF, where everything becomes explicit.

          I thought the point of FunctionRegistry.getMethodInternal() was to find best matching method signature given the arguments, while doing type conversion if necessary.

          Yes, that's what hive is doing, but it doesn't mean in order to find a match, any conversion can be performed. And the conversion is implicit. I was arguing that conversion between double and decimal should be explicit. Thus, when finding a match, Hive shouldn't treat double and decimal exchangeable, regardless whether hive is actually doing it or not.

          I don't know what this debate is leading to, but Hive has the problem as described in the JIRA and I'm solving it. If you have any feedback, I'd be happy to see it once you actually see the solution.

          Show
          Xuefu Zhang added a comment - I'm not talking about method matching. I'm talking about data type in general. Double data and decimal data are intrinsically different, and Hive shouldn't implicit convert from one to the other. An an example, let's say there is a UDF just returns the input and it only has "double evaluate(double input)" defined. For this, if user passes decimal input to the UDF, hoping to get its decimal number back. In this case, I'd argue that Hive should return an error rather than converting the input to a double and return it. I'd also contend the reverse is also true. GenericUDF overcomes the shortcomings of UDF, where everything becomes explicit. I thought the point of FunctionRegistry.getMethodInternal() was to find best matching method signature given the arguments, while doing type conversion if necessary. Yes, that's what hive is doing, but it doesn't mean in order to find a match, any conversion can be performed. And the conversion is implicit. I was arguing that conversion between double and decimal should be explicit. Thus, when finding a match, Hive shouldn't treat double and decimal exchangeable, regardless whether hive is actually doing it or not. I don't know what this debate is leading to, but Hive has the problem as described in the JIRA and I'm solving it. If you have any feedback, I'd be happy to see it once you actually see the solution.
          Hide
          Hive QA added a comment -

          Overall: -1 at least one tests failed

          Here are the results of testing the latest attachment:
          https://issues.apache.org/jira/secure/attachment/12626373/HIVE-6327.patch

          ERROR: -1 due to 1 failed/errored test(s), 5006 tests executed
          Failed tests:

          org.apache.hadoop.hive.cli.TestMinimrCliDriver.testCliDriver_bucket_num_reducers
          

          Test results: http://bigtop01.cloudera.org:8080/job/PreCommit-HIVE-Build/1146/testReport
          Console output: http://bigtop01.cloudera.org:8080/job/PreCommit-HIVE-Build/1146/console

          Messages:

          Executing org.apache.hive.ptest.execution.PrepPhase
          Executing org.apache.hive.ptest.execution.ExecutionPhase
          Executing org.apache.hive.ptest.execution.ReportingPhase
          Tests exited with: TestsFailedException: 1 tests failed
          

          This message is automatically generated.

          ATTACHMENT ID: 12626373

          Show
          Hive QA added a comment - Overall : -1 at least one tests failed Here are the results of testing the latest attachment: https://issues.apache.org/jira/secure/attachment/12626373/HIVE-6327.patch ERROR: -1 due to 1 failed/errored test(s), 5006 tests executed Failed tests: org.apache.hadoop.hive.cli.TestMinimrCliDriver.testCliDriver_bucket_num_reducers Test results: http://bigtop01.cloudera.org:8080/job/PreCommit-HIVE-Build/1146/testReport Console output: http://bigtop01.cloudera.org:8080/job/PreCommit-HIVE-Build/1146/console Messages: Executing org.apache.hive.ptest.execution.PrepPhase Executing org.apache.hive.ptest.execution.ExecutionPhase Executing org.apache.hive.ptest.execution.ReportingPhase Tests exited with: TestsFailedException: 1 tests failed This message is automatically generated. ATTACHMENT ID: 12626373
          Hide
          Xuefu Zhang added a comment -

          The above failed test case is flaky, which is unrelated code changes in the patch. Patch is ready for review.

          Show
          Xuefu Zhang added a comment - The above failed test case is flaky, which is unrelated code changes in the patch. Patch is ready for review.
          Hide
          Xuefu Zhang added a comment -
          Show
          Xuefu Zhang added a comment - RB: https://reviews.apache.org/r/17661/
          Hide
          Jason Dere added a comment -

          +1

          Show
          Jason Dere added a comment - +1
          Hide
          Mohammad Kamrul Islam added a comment -

          Left few minor comments in RB.

          Show
          Mohammad Kamrul Islam added a comment - Left few minor comments in RB.
          Hide
          Xuefu Zhang added a comment -

          Patch #1 updated:

          1. removed a tab
          2. if condition, changed to >= 1.0

          Show
          Xuefu Zhang added a comment - Patch #1 updated: 1. removed a tab 2. if condition, changed to >= 1.0
          Hide
          Hive QA added a comment -

          Overall: +1 all checks pass

          Here are the results of testing the latest attachment:
          https://issues.apache.org/jira/secure/attachment/12626802/HIVE-6327.1.patch

          SUCCESS: +1 5010 tests passed

          Test results: http://bigtop01.cloudera.org:8080/job/PreCommit-HIVE-Build/1173/testReport
          Console output: http://bigtop01.cloudera.org:8080/job/PreCommit-HIVE-Build/1173/console

          Messages:

          Executing org.apache.hive.ptest.execution.PrepPhase
          Executing org.apache.hive.ptest.execution.ExecutionPhase
          Executing org.apache.hive.ptest.execution.ReportingPhase
          

          This message is automatically generated.

          ATTACHMENT ID: 12626802

          Show
          Hive QA added a comment - Overall : +1 all checks pass Here are the results of testing the latest attachment: https://issues.apache.org/jira/secure/attachment/12626802/HIVE-6327.1.patch SUCCESS: +1 5010 tests passed Test results: http://bigtop01.cloudera.org:8080/job/PreCommit-HIVE-Build/1173/testReport Console output: http://bigtop01.cloudera.org:8080/job/PreCommit-HIVE-Build/1173/console Messages: Executing org.apache.hive.ptest.execution.PrepPhase Executing org.apache.hive.ptest.execution.ExecutionPhase Executing org.apache.hive.ptest.execution.ReportingPhase This message is automatically generated. ATTACHMENT ID: 12626802
          Hide
          Xuefu Zhang added a comment -

          Patch committed to trunk. Thanks Jason and others for the review.

          Show
          Xuefu Zhang added a comment - Patch committed to trunk. Thanks Jason and others for the review.

            People

            • Assignee:
              Xuefu Zhang
              Reporter:
              Xuefu Zhang
            • Votes:
              0 Vote for this issue
              Watchers:
              3 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development