Details

    • Type: New Feature New Feature
    • Status: Resolved
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 0.13.0
    • Component/s: Types
    • Labels:
      None
    • Release Note:
      The new datatype (char) has been documented in Types and DDL wikidocs.

      Description

      Separate task for char type, since HIVE-4844 only adds varchar

      1. HIVE-5191.4.patch
        260 kB
        Jason Dere
      2. HIVE-5191.3.patch
        260 kB
        Jason Dere
      3. HIVE-5191.2.patch
        251 kB
        Jason Dere
      4. HIVE-5191.1.patch
        251 kB
        Jason Dere

        Issue Links

          Activity

          Hide
          Jason Dere added a comment -

          patch v1

          • string length enforced (like varchar)
          • string value is padded to full string length
          • trailing spaces not significant during comparison
          Show
          Jason Dere added a comment - patch v1 string length enforced (like varchar) string value is padded to full string length trailing spaces not significant during comparison
          Hide
          Jason Dere added a comment -
          Show
          Jason Dere added a comment - review at https://reviews.apache.org/r/14998/
          Hide
          Xuefu Zhang added a comment -

          Jason Dere Thanks for the patch. It looks very good. I haven't gone into details yet, but I had some high-level comments on the RB. Thanks.

          Show
          Xuefu Zhang added a comment - Jason Dere Thanks for the patch. It looks very good. I haven't gone into details yet, but I had some high-level comments on the RB. Thanks.
          Hide
          Jason Dere added a comment -

          Patch v2, changes based on Xuefu's comments

          Show
          Jason Dere added a comment - Patch v2, changes based on Xuefu's comments
          Hide
          Xuefu Zhang added a comment -

          Hi Jason Dere, I have some comments on rb on your latest patch. After I played with your patch, I felt that neither characterLength nor maxLength is necessary. The reason for no need of maxLength is that you will always have type info because the grammar prevents one from specifying a type of char w/o maxLength. This is different from HiveDecimal, where UDF can specifies a return type w/o precision/scale, for which some default values have to be assumed. Please let me know what you think. Thanks.

          Show
          Xuefu Zhang added a comment - Hi Jason Dere , I have some comments on rb on your latest patch. After I played with your patch, I felt that neither characterLength nor maxLength is necessary. The reason for no need of maxLength is that you will always have type info because the grammar prevents one from specifying a type of char w/o maxLength. This is different from HiveDecimal, where UDF can specifies a return type w/o precision/scale, for which some default values have to be assumed. Please let me know what you think. Thanks.
          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/12610910/HIVE-5191.2.patch

          SUCCESS: +1 4541 tests passed

          Test results: http://bigtop01.cloudera.org:8080/job/PreCommit-HIVE-Build/34/testReport
          Console output: http://bigtop01.cloudera.org:8080/job/PreCommit-HIVE-Build/34/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/12610910/HIVE-5191.2.patch SUCCESS: +1 4541 tests passed Test results: http://bigtop01.cloudera.org:8080/job/PreCommit-HIVE-Build/34/testReport Console output: http://bigtop01.cloudera.org:8080/job/PreCommit-HIVE-Build/34/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 -

          patch v3, more changes per Xuefu's review.

          Show
          Jason Dere added a comment - patch v3, more changes per Xuefu's review.
          Hide
          Xuefu Zhang added a comment -

          Jason Dere Thanks for making those changes. I left a minor question on RB. Other than that, the patch looks good to me.

          Show
          Xuefu Zhang added a comment - Jason Dere Thanks for making those changes. I left a minor question on RB. Other than that, the patch looks good to me.
          Hide
          Ashutosh Chauhan added a comment -

          Max length currently is 255. Seems like different DBs use different max sizes. Mysql : 255. Postgres: 1GB, MS SQL Server : 8K. Seems like a lot of variation. It will be good to check if standard says something about this. For us, picking higher value shouldn't matter, so in case standard is lax, we may want to pick higher value.

          Show
          Ashutosh Chauhan added a comment - Max length currently is 255. Seems like different DBs use different max sizes. Mysql : 255. Postgres: 1GB, MS SQL Server : 8K. Seems like a lot of variation. It will be good to check if standard says something about this. For us, picking higher value shouldn't matter, so in case standard is lax, we may want to pick higher value.
          Hide
          Jason Dere added a comment -

          Standard only says that it is implementation defined:

          35) Subclause 6.1, “<data type>”:
          i) The maximum lengths for character string types and binary string types are implementation-defined.

          The max varchar length was set based on MySQL's max length (32K), thought it made sense to do the same for char. We can make the char limit a bit bigger, but if users want very large string values then they probably should be using string columns. Any preferences on size?

          Show
          Jason Dere added a comment - Standard only says that it is implementation defined: 35) Subclause 6.1, “<data type>”: i) The maximum lengths for character string types and binary string types are implementation-defined. The max varchar length was set based on MySQL's max length (32K), thought it made sense to do the same for char. We can make the char limit a bit bigger, but if users want very large string values then they probably should be using string columns. Any preferences on size?
          Hide
          Xuefu Zhang added a comment -

          Personally I think 255 might be sufficient, and I'd like the idea of being inline with mysql. I haven't heard any complaint about the limit is too low for char type for any any database. Most usage of char is storing some fix-length symbols such as state abbreviation, zip code, ssn, etc. for those, 255 is probably good enough. In Hive, user can always opt for varchar if something bigger is needed. After all, char offers no performance adv over varchar, at least as of now.

          Show
          Xuefu Zhang added a comment - Personally I think 255 might be sufficient, and I'd like the idea of being inline with mysql. I haven't heard any complaint about the limit is too low for char type for any any database. Most usage of char is storing some fix-length symbols such as state abbreviation, zip code, ssn, etc. for those, 255 is probably good enough. In Hive, user can always opt for varchar if something bigger is needed. After all, char offers no performance adv over varchar, at least as of now.
          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/12611486/HIVE-5191.3.patch

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

          org.apache.hadoop.hive.cli.TestCliDriver.testCliDriver_char_1
          org.apache.hadoop.hive.cli.TestCliDriver.testCliDriver_char_join1
          org.apache.hadoop.hive.cli.TestCliDriver.testCliDriver_char_union1
          

          Test results: http://bigtop01.cloudera.org:8080/job/PreCommit-HIVE-Build/98/testReport
          Console output: http://bigtop01.cloudera.org:8080/job/PreCommit-HIVE-Build/98/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/12611486/HIVE-5191.3.patch ERROR: -1 due to 3 failed/errored test(s), 4572 tests executed Failed tests: org.apache.hadoop.hive.cli.TestCliDriver.testCliDriver_char_1 org.apache.hadoop.hive.cli.TestCliDriver.testCliDriver_char_join1 org.apache.hadoop.hive.cli.TestCliDriver.testCliDriver_char_union1 Test results: http://bigtop01.cloudera.org:8080/job/PreCommit-HIVE-Build/98/testReport Console output: http://bigtop01.cloudera.org:8080/job/PreCommit-HIVE-Build/98/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.
          Hide
          Jason Dere added a comment -

          patch v4: Xuefu feedback, update qfiles due to mavenization

          Show
          Jason Dere added a comment - patch v4: Xuefu feedback, update qfiles due to mavenization
          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/12611643/HIVE-5191.4.patch

          SUCCESS: +1 4572 tests passed

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

          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/12611643/HIVE-5191.4.patch SUCCESS: +1 4572 tests passed Test results: http://bigtop01.cloudera.org:8080/job/PreCommit-HIVE-Build/117/testReport Console output: http://bigtop01.cloudera.org:8080/job/PreCommit-HIVE-Build/117/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: 12611643
          Hide
          Jason Dere added a comment -

          Xuefu Zhang, you're now a committer! Given that you've spent some time reviewing this one, are there any other issues here, or does it have your approval to go in?

          Show
          Jason Dere added a comment - Xuefu Zhang , you're now a committer! Given that you've spent some time reviewing this one, are there any other issues here, or does it have your approval to go in?
          Hide
          Xuefu Zhang added a comment -

          Jason Dere Thanks for asking my opinion. I think the patch are in a really good shape. I posted a minor comment on RB for your to consider. Upon your response, I would be more than happy to +1 on this. Thank you for your contribution.

          Show
          Xuefu Zhang added a comment - Jason Dere Thanks for asking my opinion. I think the patch are in a really good shape. I posted a minor comment on RB for your to consider. Upon your response, I would be more than happy to +1 on this. Thank you for your contribution.
          Hide
          Xuefu Zhang added a comment -

          +1, patch look good to me.

          Show
          Xuefu Zhang added a comment - +1, patch look good to me.
          Hide
          Xuefu Zhang added a comment -

          Patch committed to trunk. Thanks to Jason for the contribution.

          Show
          Xuefu Zhang added a comment - Patch committed to trunk. Thanks to Jason for the contribution.
          Hide
          Lefty Leverenz added a comment -

          Lars Francke documented char in the Types doc and I added it to the primitive_type list for CREATE TABLE syntax:

          Show
          Lefty Leverenz added a comment - Lars Francke documented char in the Types doc and I added it to the primitive_type list for CREATE TABLE syntax: Types doc String Types Char DDL doc Create Table

            People

            • Assignee:
              Jason Dere
              Reporter:
              Jason Dere
            • Votes:
              0 Vote for this issue
              Watchers:
              5 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development