Hive
  1. Hive
  2. HIVE-5355

JDBC support for decimal precision/scale

    Details

    • Type: Task Task
    • Status: Resolved
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 0.10.0
    • Fix Version/s: 0.13.0
    • Labels:
      None

      Description

      A subtask of HIVE-3976.

      1. HIVE-5355.3.patch
        15 kB
        Xuefu Zhang
      2. HIVE-5355.2.patch
        15 kB
        Xuefu Zhang
      3. HIVE-5355.1.patch
        15 kB
        Xuefu Zhang
      4. HIVE-5355.patch
        14 kB
        Xuefu Zhang

        Issue Links

          Activity

          Prasad Mujumdar made changes -
          Link This issue breaks HIVE-5780 [ HIVE-5780 ]
          Hide
          Xuefu Zhang added a comment -

          Prasad Mujumdar, Thanks for pointing this out. Jason Dere Also pointed this to me. The entry in IDL file I added got lost somewhere during merge and rebase. Since I manually modified the generated file, the build happened to be okay. Please feel free add the entry in your patch, or log a separate JIRA for this. I can review it.

          Show
          Xuefu Zhang added a comment - Prasad Mujumdar , Thanks for pointing this out. Jason Dere Also pointed this to me. The entry in IDL file I added got lost somewhere during merge and rebase. Since I manually modified the generated file, the build happened to be okay. Please feel free add the entry in your patch, or log a separate JIRA for this. I can review it.
          Hide
          Jason Dere added a comment -

          I believe this patch ended up modifying the generated code rather than generating the code using the thrift definition. I've corrected this in my patch for HIVE-5683.

          Show
          Jason Dere added a comment - I believe this patch ended up modifying the generated code rather than generating the code using the thrift definition. I've corrected this in my patch for HIVE-5683 .
          Hide
          Prasad Mujumdar added a comment -

          Looks like the TCLIService.thrift was not correctly merged. The TProtocolVersion.HIVE_CLI_SERVICE_PROTOCOL_V4 is used in the IDL but the declaration is missing. It's there in the generated code so it builds, but when you try to update the IDL, it throws an error.

          Show
          Prasad Mujumdar added a comment - Looks like the TCLIService.thrift was not correctly merged. The TProtocolVersion.HIVE_CLI_SERVICE_PROTOCOL_V4 is used in the IDL but the declaration is missing. It's there in the generated code so it builds, but when you try to update the IDL, it throws an error.
          Brock Noland made changes -
          Status Patch Available [ 10002 ] Resolved [ 5 ]
          Resolution Fixed [ 1 ]
          Hide
          Brock Noland added a comment -

          Thank you for the contribution Xuefu! I have committed this to trunk!

          Show
          Brock Noland added a comment - Thank you for the contribution Xuefu! I have committed this to trunk!
          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/12612328/HIVE-5355.3.patch

          SUCCESS: +1 4551 tests passed

          Test results: http://bigtop01.cloudera.org:8080/job/PreCommit-HIVE-Build/150/testReport
          Console output: http://bigtop01.cloudera.org:8080/job/PreCommit-HIVE-Build/150/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: 12612328

          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/12612328/HIVE-5355.3.patch SUCCESS: +1 4551 tests passed Test results: http://bigtop01.cloudera.org:8080/job/PreCommit-HIVE-Build/150/testReport Console output: http://bigtop01.cloudera.org:8080/job/PreCommit-HIVE-Build/150/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: 12612328
          Xuefu Zhang made changes -
          Status Open [ 1 ] Patch Available [ 10002 ]
          Xuefu Zhang made changes -
          Attachment HIVE-5355.3.patch [ 12612328 ]
          Hide
          Xuefu Zhang added a comment -

          Patch #3 rebased with latest trunk.

          Show
          Xuefu Zhang added a comment - Patch #3 rebased with latest trunk.
          Brock Noland made changes -
          Status Patch Available [ 10002 ] Open [ 1 ]
          Hide
          Brock Noland added a comment -

          Cancelling patch for now since the patch doesn't apply.

          Show
          Brock Noland added a comment - Cancelling patch for now since the patch doesn't apply.
          Hide
          Brock Noland added a comment -

          This patch doesn't apply any longer can you rebase?

          Show
          Brock Noland added a comment - This patch doesn't apply any longer can you rebase?
          Hide
          Brock Noland added a comment -

          +1

          Show
          Brock Noland added a comment - +1
          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/12610593/HIVE-5355.2.patch

          SUCCESS: +1 4512 tests passed

          Test results: https://builds.apache.org/job/PreCommit-HIVE-Build/1276/testReport
          Console output: https://builds.apache.org/job/PreCommit-HIVE-Build/1276/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.

          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/12610593/HIVE-5355.2.patch SUCCESS: +1 4512 tests passed Test results: https://builds.apache.org/job/PreCommit-HIVE-Build/1276/testReport Console output: https://builds.apache.org/job/PreCommit-HIVE-Build/1276/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.
          Hide
          Jason Dere added a comment -

          oops, disregard my comment, those changes are calling columnPrecision() which is doing the null checks.

          Show
          Jason Dere added a comment - oops, disregard my comment, those changes are calling columnPrecision() which is doing the null checks.
          Hide
          Jason Dere added a comment -

          Hey Xuefu, for these changes:

               case Types.VARCHAR:
          -      if (columnAttributes != null) {
          -        return columnAttributes.precision;
          -      }
          -      return Integer.MAX_VALUE; // hive has no max limit for strings
          +      return columnPrecision(columnType, columnAttributes);
          

          String columns end up getting translated to JDBC varchar type. So changing this would result in a NPE if the user tries to call getColumnDisplaySize() for string columns, because I think the columnAttributes are null for non-qualified Hive types.
          Not sure if you have to worry about a similar issue for decimal if you are connecting to a pre-HIVE-3976 Hive server, are newer clients meant to be backward compatible with older servers?

          Show
          Jason Dere added a comment - Hey Xuefu, for these changes: case Types.VARCHAR: - if (columnAttributes != null) { - return columnAttributes.precision; - } - return Integer.MAX_VALUE; // hive has no max limit for strings + return columnPrecision(columnType, columnAttributes); String columns end up getting translated to JDBC varchar type. So changing this would result in a NPE if the user tries to call getColumnDisplaySize() for string columns, because I think the columnAttributes are null for non-qualified Hive types. Not sure if you have to worry about a similar issue for decimal if you are connecting to a pre- HIVE-3976 Hive server, are newer clients meant to be backward compatible with older servers?
          Xuefu Zhang made changes -
          Status Open [ 1 ] Patch Available [ 10002 ]
          Xuefu Zhang made changes -
          Attachment HIVE-5355.2.patch [ 12610593 ]
          Hide
          Xuefu Zhang added a comment -

          Patch #2 fixed the test code. Manually running the test passed.

          Show
          Xuefu Zhang added a comment - Patch #2 fixed the test code. Manually running the test passed.
          Hide
          Brock Noland added a comment -

          FWIW, the patch does this:

              try {
                stmt.executeQuery("select from " + dataTypeTableName);
               } catch (SQLException e) {
                 assertEquals("42000", e.getSQLState());
               }
              fail("SQLException is expected"); 
          

          and it should do:

              try {
                stmt.executeQuery("select from " + dataTypeTableName);
                fail("SQLException is expected"); 
               } catch (SQLException e) {
                 assertEquals("42000", e.getSQLState());
               }
          
          Show
          Brock Noland added a comment - FWIW, the patch does this: try { stmt.executeQuery("select from " + dataTypeTableName); } catch (SQLException e) { assertEquals("42000", e.getSQLState()); } fail("SQLException is expected"); and it should do: try { stmt.executeQuery("select from " + dataTypeTableName); fail("SQLException is expected"); } catch (SQLException e) { assertEquals("42000", e.getSQLState()); }
          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/12610501/HIVE-5355.1.patch

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

          org.apache.hive.jdbc.TestJdbcDriver2.testErrorDiag0
          org.apache.hive.jdbc.TestJdbcDriver2.testErrorDiag1
          org.apache.hive.jdbc.TestJdbcDriver2.testErrorDiag2
          

          Test results: https://builds.apache.org/job/PreCommit-HIVE-Build/1263/testReport
          Console output: https://builds.apache.org/job/PreCommit-HIVE-Build/1263/console

          Messages:

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

          This message is automatically generated.

          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/12610501/HIVE-5355.1.patch ERROR: -1 due to 3 failed/errored test(s), 4504 tests executed Failed tests: org.apache.hive.jdbc.TestJdbcDriver2.testErrorDiag0 org.apache.hive.jdbc.TestJdbcDriver2.testErrorDiag1 org.apache.hive.jdbc.TestJdbcDriver2.testErrorDiag2 Test results: https://builds.apache.org/job/PreCommit-HIVE-Build/1263/testReport Console output: https://builds.apache.org/job/PreCommit-HIVE-Build/1263/console Messages: Executing org.apache.hive.ptest.execution.PrepPhase Executing org.apache.hive.ptest.execution.ExecutionPhase Executing org.apache.hive.ptest.execution.ReportingPhase Tests failed with: TestsFailedException: 3 tests failed This message is automatically generated.
          Xuefu Zhang made changes -
          Status Patch Available [ 10002 ] Open [ 1 ]
          Hide
          Xuefu Zhang added a comment -

          Okay. Let me take a new look. I may need to put a "return" in the catch clause. Cancelling the patch.

          Show
          Xuefu Zhang added a comment - Okay. Let me take a new look. I may need to put a "return" in the catch clause. Cancelling the patch.
          Hide
          Brock Noland added a comment -

          I might be mis reading but it looks like the fails were added to the wrong location.

          Show
          Brock Noland added a comment - I might be mis reading but it looks like the fails were added to the wrong location.
          Xuefu Zhang made changes -
          Status Open [ 1 ] Patch Available [ 10002 ]
          Xuefu Zhang made changes -
          Attachment HIVE-5355.1.patch [ 12610501 ]
          Hide
          Xuefu Zhang added a comment -

          Patch #1 rebased with latest trunk, and also addressed Brock's concern above.

          Show
          Xuefu Zhang added a comment - Patch #1 rebased with latest trunk, and also addressed Brock's concern above.
          Hide
          Xuefu Zhang added a comment -

          Thanks, Brock. That's a valid point, though the problem is not introduced by this patch. The patch will be updated with that nevertheless.

          Show
          Xuefu Zhang added a comment - Thanks, Brock. That's a valid point, though the problem is not introduced by this patch. The patch will be updated with that nevertheless.
          Hide
          Brock Noland added a comment -

          There should be a fail() after the stmt.exec() below in case no exception is thrown.

               // verify syntax error
               try {
          -      ResultSet res = stmt.executeQuery("select from " + dataTypeTableName);
          +      stmt.executeQuery("select from " + dataTypeTableName);
               } catch (SQLException e) {
                 assertEquals("42000", e.getSQLState());
               }
          
          Show
          Brock Noland added a comment - There should be a fail() after the stmt.exec() below in case no exception is thrown. // verify syntax error try { - ResultSet res = stmt.executeQuery("select from " + dataTypeTableName); + stmt.executeQuery("select from " + dataTypeTableName); } catch (SQLException e) { assertEquals("42000", e.getSQLState()); }
          Xuefu Zhang made changes -
          Link This issue depends upon HIVE-3976 [ HIVE-3976 ]
          Xuefu Zhang made changes -
          Attachment HIVE-5355.patch [ 12609250 ]
          Xuefu Zhang made changes -
          Field Original Value New Value
          Link This issue is part of HIVE-3976 [ HIVE-3976 ]
          Xuefu Zhang created issue -

            People

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

              Dates

              • Created:
                Updated:
                Resolved:

                Development