Hive
  1. Hive
  2. HIVE-2171

Allow custom serdes to set field comments

    Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 0.7.0
    • Fix Version/s: 0.8.0
    • Component/s: None
    • Labels:
      None
    • Hadoop Flags:
      Reviewed

      Description

      Currently, while serde implementations can set a field's name, they can't set its comment. These are set in the metastore utils to (from deserializer). For those serdes that can provide meaningful comments for a field, they should be propagated to the table description. These serde-provided comments could be prepended to "(from deserializer)" if others feel that's a meaningful distinction. This change involves updating StructField to support a (possibly null) comment field and then propagating this change out to the myriad places StructField is thrown around.

      1. HIVE-2171-2.patch
        36 kB
        Jakob Homan
      2. HIVE-2171.patch
        36 kB
        Jakob Homan

        Activity

        Hide
        Hudson added a comment -

        Integrated in Hive-trunk-h0.21 #892 (See https://builds.apache.org/job/Hive-trunk-h0.21/892/)
        HIVE-2171. Allow custom serdes to set field comments
        (Jakob Homan via jvs)

        jvs : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1157224
        Files :

        • /hive/trunk/metastore/src/java/org/apache/hadoop/hive/metastore/MetaStoreUtils.java
        • /hive/trunk/serde/src/test/org/apache/hadoop/hive/serde2/objectinspector/TestStandardObjectInspectors.java
        • /hive/trunk/serde/src/java/org/apache/hadoop/hive/serde2/objectinspector/ColumnarStructObjectInspector.java
        • /hive/trunk/serde/src/java/org/apache/hadoop/hive/serde2/objectinspector/UnionStructObjectInspector.java
        • /hive/trunk/serde/src/java/org/apache/hadoop/hive/serde2/lazy/objectinspector/LazySimpleStructObjectInspector.java
        • /hive/trunk/serde/src/java/org/apache/hadoop/hive/serde2/objectinspector/ReflectionStructObjectInspector.java
        • /hive/trunk/serde/src/java/org/apache/hadoop/hive/serde2/objectinspector/ObjectInspectorFactory.java
        • /hive/trunk/serde/src/test/org/apache/hadoop/hive/serde2/TestSerdeWithFieldComments.java
        • /hive/trunk/serde/src/java/org/apache/hadoop/hive/serde2/objectinspector/MetadataListStructObjectInspector.java
        • /hive/trunk/serde/src/java/org/apache/hadoop/hive/serde2/lazybinary/objectinspector/LazyBinaryObjectInspectorFactory.java
        • /hive/trunk/serde/src/java/org/apache/hadoop/hive/serde2/lazy/objectinspector/LazyObjectInspectorFactory.java
        • /hive/trunk/ivy/libraries.properties
        • /hive/trunk/serde/src/java/org/apache/hadoop/hive/serde2/lazybinary/objectinspector/LazyBinaryStructObjectInspector.java
        • /hive/trunk/serde/ivy.xml
        • /hive/trunk/serde/src/java/org/apache/hadoop/hive/serde2/objectinspector/StandardStructObjectInspector.java
        • /hive/trunk/serde/src/java/org/apache/hadoop/hive/serde2/objectinspector/StructField.java
        Show
        Hudson added a comment - Integrated in Hive-trunk-h0.21 #892 (See https://builds.apache.org/job/Hive-trunk-h0.21/892/ ) HIVE-2171 . Allow custom serdes to set field comments (Jakob Homan via jvs) jvs : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1157224 Files : /hive/trunk/metastore/src/java/org/apache/hadoop/hive/metastore/MetaStoreUtils.java /hive/trunk/serde/src/test/org/apache/hadoop/hive/serde2/objectinspector/TestStandardObjectInspectors.java /hive/trunk/serde/src/java/org/apache/hadoop/hive/serde2/objectinspector/ColumnarStructObjectInspector.java /hive/trunk/serde/src/java/org/apache/hadoop/hive/serde2/objectinspector/UnionStructObjectInspector.java /hive/trunk/serde/src/java/org/apache/hadoop/hive/serde2/lazy/objectinspector/LazySimpleStructObjectInspector.java /hive/trunk/serde/src/java/org/apache/hadoop/hive/serde2/objectinspector/ReflectionStructObjectInspector.java /hive/trunk/serde/src/java/org/apache/hadoop/hive/serde2/objectinspector/ObjectInspectorFactory.java /hive/trunk/serde/src/test/org/apache/hadoop/hive/serde2/TestSerdeWithFieldComments.java /hive/trunk/serde/src/java/org/apache/hadoop/hive/serde2/objectinspector/MetadataListStructObjectInspector.java /hive/trunk/serde/src/java/org/apache/hadoop/hive/serde2/lazybinary/objectinspector/LazyBinaryObjectInspectorFactory.java /hive/trunk/serde/src/java/org/apache/hadoop/hive/serde2/lazy/objectinspector/LazyObjectInspectorFactory.java /hive/trunk/ivy/libraries.properties /hive/trunk/serde/src/java/org/apache/hadoop/hive/serde2/lazybinary/objectinspector/LazyBinaryStructObjectInspector.java /hive/trunk/serde/ivy.xml /hive/trunk/serde/src/java/org/apache/hadoop/hive/serde2/objectinspector/StandardStructObjectInspector.java /hive/trunk/serde/src/java/org/apache/hadoop/hive/serde2/objectinspector/StructField.java
        Hide
        Jakob Homan added a comment -

        Quite likely. I'll investigate and open a JIRA. Thanks.

        Show
        Jakob Homan added a comment - Quite likely. I'll investigate and open a JIRA. Thanks.
        Hide
        John Sichi added a comment -

        BTW, I noticed that the mockito jar ends up in build/dist lib (similar to junit). Does that mean we also need to add a license notice for it?

        Show
        John Sichi added a comment - BTW, I noticed that the mockito jar ends up in build/dist lib (similar to junit). Does that mean we also need to add a license notice for it?
        Hide
        John Sichi added a comment -

        Committed. Thanks Jakob!

        Show
        John Sichi added a comment - Committed. Thanks Jakob!
        Hide
        John Sichi added a comment -

        +1. Will commit when tests pass.

        Show
        John Sichi added a comment - +1. Will commit when tests pass.
        Hide
        jiraposter@reviews.apache.org added a comment -

        -----------------------------------------------------------
        This is an automatically generated e-mail. To reply, visit:
        https://reviews.apache.org/r/790/
        -----------------------------------------------------------

        (Updated 2011-08-10 19:24:16.187507)

        Review request for hive and John Sichi.

        Changes
        -------

        updated patch based on review comments.

        Summary
        -------

        HIVE-2171: Allow custom serdes to set field comments

        This addresses bug HIVE-2171.
        https://issues.apache.org/jira/browse/HIVE-2171

        Diffs


        ivy/libraries.properties af856bd
        metastore/src/java/org/apache/hadoop/hive/metastore/MetaStoreUtils.java c1fa4e5
        serde/ivy.xml d6c836a
        serde/src/java/org/apache/hadoop/hive/serde2/lazy/objectinspector/LazyObjectInspectorFactory.java 4850601
        serde/src/java/org/apache/hadoop/hive/serde2/lazy/objectinspector/LazySimpleStructObjectInspector.java e2fa9db
        serde/src/java/org/apache/hadoop/hive/serde2/lazybinary/objectinspector/LazyBinaryObjectInspectorFactory.java 2947e49
        serde/src/java/org/apache/hadoop/hive/serde2/lazybinary/objectinspector/LazyBinaryStructObjectInspector.java 3d5408f
        serde/src/java/org/apache/hadoop/hive/serde2/objectinspector/ColumnarStructObjectInspector.java 881c3c1
        serde/src/java/org/apache/hadoop/hive/serde2/objectinspector/MetadataListStructObjectInspector.java bd42a0c
        serde/src/java/org/apache/hadoop/hive/serde2/objectinspector/ObjectInspectorFactory.java 0c8cc42
        serde/src/java/org/apache/hadoop/hive/serde2/objectinspector/ReflectionStructObjectInspector.java 4a934c5
        serde/src/java/org/apache/hadoop/hive/serde2/objectinspector/StandardStructObjectInspector.java 3b26e45
        serde/src/java/org/apache/hadoop/hive/serde2/objectinspector/StructField.java 62c3017
        serde/src/java/org/apache/hadoop/hive/serde2/objectinspector/UnionStructObjectInspector.java 76ff736
        serde/src/test/org/apache/hadoop/hive/serde2/TestSerdeWithFieldComments.java PRE-CREATION
        serde/src/test/org/apache/hadoop/hive/serde2/objectinspector/TestStandardObjectInspectors.java f139ea5

        Diff: https://reviews.apache.org/r/790/diff

        Testing
        -------

        New unit test and refactor existing unit test.

        Thanks,

        Jakob

        Show
        jiraposter@reviews.apache.org added a comment - ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/790/ ----------------------------------------------------------- (Updated 2011-08-10 19:24:16.187507) Review request for hive and John Sichi. Changes ------- updated patch based on review comments. Summary ------- HIVE-2171 : Allow custom serdes to set field comments This addresses bug HIVE-2171 . https://issues.apache.org/jira/browse/HIVE-2171 Diffs ivy/libraries.properties af856bd metastore/src/java/org/apache/hadoop/hive/metastore/MetaStoreUtils.java c1fa4e5 serde/ivy.xml d6c836a serde/src/java/org/apache/hadoop/hive/serde2/lazy/objectinspector/LazyObjectInspectorFactory.java 4850601 serde/src/java/org/apache/hadoop/hive/serde2/lazy/objectinspector/LazySimpleStructObjectInspector.java e2fa9db serde/src/java/org/apache/hadoop/hive/serde2/lazybinary/objectinspector/LazyBinaryObjectInspectorFactory.java 2947e49 serde/src/java/org/apache/hadoop/hive/serde2/lazybinary/objectinspector/LazyBinaryStructObjectInspector.java 3d5408f serde/src/java/org/apache/hadoop/hive/serde2/objectinspector/ColumnarStructObjectInspector.java 881c3c1 serde/src/java/org/apache/hadoop/hive/serde2/objectinspector/MetadataListStructObjectInspector.java bd42a0c serde/src/java/org/apache/hadoop/hive/serde2/objectinspector/ObjectInspectorFactory.java 0c8cc42 serde/src/java/org/apache/hadoop/hive/serde2/objectinspector/ReflectionStructObjectInspector.java 4a934c5 serde/src/java/org/apache/hadoop/hive/serde2/objectinspector/StandardStructObjectInspector.java 3b26e45 serde/src/java/org/apache/hadoop/hive/serde2/objectinspector/StructField.java 62c3017 serde/src/java/org/apache/hadoop/hive/serde2/objectinspector/UnionStructObjectInspector.java 76ff736 serde/src/test/org/apache/hadoop/hive/serde2/TestSerdeWithFieldComments.java PRE-CREATION serde/src/test/org/apache/hadoop/hive/serde2/objectinspector/TestStandardObjectInspectors.java f139ea5 Diff: https://reviews.apache.org/r/790/diff Testing ------- New unit test and refactor existing unit test. Thanks, Jakob
        Hide
        Jakob Homan added a comment -

        I did do the intersection and didn't see anything. I've got tests going, but don't expect anything new as the review comments were about style rather than logic. The was relatively simple, so I'm not overly concerned about new problems introduced in the interim. Updated RB; some of the changes it shows for patch v1 to v2 are spurious and introduced by the trunk drift. Thanks.

        Show
        Jakob Homan added a comment - I did do the intersection and didn't see anything. I've got tests going, but don't expect anything new as the review comments were about style rather than logic. The was relatively simple, so I'm not overly concerned about new problems introduced in the interim. Updated RB; some of the changes it shows for patch v1 to v2 are spurious and introduced by the trunk drift. Thanks.
        Hide
        jiraposter@reviews.apache.org added a comment -

        -----------------------------------------------------------
        This is an automatically generated e-mail. To reply, visit:
        https://reviews.apache.org/r/790/
        -----------------------------------------------------------

        (Updated 2011-08-04 00:12:21.542815)

        Review request for hive.

        Changes
        -------

        Updated patch based on review comments.

        Summary
        -------

        HIVE-2171: Allow custom serdes to set field comments

        This addresses bug HIVE-2171.
        https://issues.apache.org/jira/browse/HIVE-2171

        Diffs (updated)


        ivy/libraries.properties af856bd
        metastore/src/java/org/apache/hadoop/hive/metastore/MetaStoreUtils.java c1fa4e5
        serde/ivy.xml d6c836a
        serde/src/java/org/apache/hadoop/hive/serde2/lazy/objectinspector/LazyObjectInspectorFactory.java 4850601
        serde/src/java/org/apache/hadoop/hive/serde2/lazy/objectinspector/LazySimpleStructObjectInspector.java e2fa9db
        serde/src/java/org/apache/hadoop/hive/serde2/lazybinary/objectinspector/LazyBinaryObjectInspectorFactory.java 2947e49
        serde/src/java/org/apache/hadoop/hive/serde2/lazybinary/objectinspector/LazyBinaryStructObjectInspector.java 3d5408f
        serde/src/java/org/apache/hadoop/hive/serde2/objectinspector/ColumnarStructObjectInspector.java 881c3c1
        serde/src/java/org/apache/hadoop/hive/serde2/objectinspector/MetadataListStructObjectInspector.java bd42a0c
        serde/src/java/org/apache/hadoop/hive/serde2/objectinspector/ObjectInspectorFactory.java 0c8cc42
        serde/src/java/org/apache/hadoop/hive/serde2/objectinspector/ReflectionStructObjectInspector.java 4a934c5
        serde/src/java/org/apache/hadoop/hive/serde2/objectinspector/StandardStructObjectInspector.java 3b26e45
        serde/src/java/org/apache/hadoop/hive/serde2/objectinspector/StructField.java 62c3017
        serde/src/java/org/apache/hadoop/hive/serde2/objectinspector/UnionStructObjectInspector.java 76ff736
        serde/src/test/org/apache/hadoop/hive/serde2/TestSerdeWithFieldComments.java PRE-CREATION
        serde/src/test/org/apache/hadoop/hive/serde2/objectinspector/TestStandardObjectInspectors.java f139ea5

        Diff: https://reviews.apache.org/r/790/diff

        Testing
        -------

        New unit test and refactor existing unit test.

        Thanks,

        Jakob

        Show
        jiraposter@reviews.apache.org added a comment - ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/790/ ----------------------------------------------------------- (Updated 2011-08-04 00:12:21.542815) Review request for hive. Changes ------- Updated patch based on review comments. Summary ------- HIVE-2171 : Allow custom serdes to set field comments This addresses bug HIVE-2171 . https://issues.apache.org/jira/browse/HIVE-2171 Diffs (updated) ivy/libraries.properties af856bd metastore/src/java/org/apache/hadoop/hive/metastore/MetaStoreUtils.java c1fa4e5 serde/ivy.xml d6c836a serde/src/java/org/apache/hadoop/hive/serde2/lazy/objectinspector/LazyObjectInspectorFactory.java 4850601 serde/src/java/org/apache/hadoop/hive/serde2/lazy/objectinspector/LazySimpleStructObjectInspector.java e2fa9db serde/src/java/org/apache/hadoop/hive/serde2/lazybinary/objectinspector/LazyBinaryObjectInspectorFactory.java 2947e49 serde/src/java/org/apache/hadoop/hive/serde2/lazybinary/objectinspector/LazyBinaryStructObjectInspector.java 3d5408f serde/src/java/org/apache/hadoop/hive/serde2/objectinspector/ColumnarStructObjectInspector.java 881c3c1 serde/src/java/org/apache/hadoop/hive/serde2/objectinspector/MetadataListStructObjectInspector.java bd42a0c serde/src/java/org/apache/hadoop/hive/serde2/objectinspector/ObjectInspectorFactory.java 0c8cc42 serde/src/java/org/apache/hadoop/hive/serde2/objectinspector/ReflectionStructObjectInspector.java 4a934c5 serde/src/java/org/apache/hadoop/hive/serde2/objectinspector/StandardStructObjectInspector.java 3b26e45 serde/src/java/org/apache/hadoop/hive/serde2/objectinspector/StructField.java 62c3017 serde/src/java/org/apache/hadoop/hive/serde2/objectinspector/UnionStructObjectInspector.java 76ff736 serde/src/test/org/apache/hadoop/hive/serde2/TestSerdeWithFieldComments.java PRE-CREATION serde/src/test/org/apache/hadoop/hive/serde2/objectinspector/TestStandardObjectInspectors.java f139ea5 Diff: https://reviews.apache.org/r/790/diff Testing ------- New unit test and refactor existing unit test. Thanks, Jakob
        Hide
        John Sichi added a comment -
        • Yes, please update RB
        • checkstyle: yeah, I think what people do is intersect the list of files they changed with the report output
        • test suite: yes, committers run the full suite and reject if anything fails, but it's best if authors do too (takes hours these days, so kick it off before you go to bed)

        (And yes, we have ideas for improving all three of these inefficiencies, but so far no owners.)

        Show
        John Sichi added a comment - Yes, please update RB checkstyle: yeah, I think what people do is intersect the list of files they changed with the report output test suite: yes, committers run the full suite and reject if anything fails, but it's best if authors do too (takes hours these days, so kick it off before you go to bed) (And yes, we have ideas for improving all three of these inefficiencies, but so far no owners.)
        Hide
        Jakob Homan added a comment -

        Updated patch.

        • rebased against trunk
        • added braces around all new if statements
        • checked for checkstyle errors, didn't see any, although it's difficult to be sure since there is a baseline of 4,789 violations in trunk.

        Do I need to open a new RB for this updated patch? Do the committers run the full test suite? I see lots of comments like "will commit if tests pass." Does this mean that in Hive it's up to the committers to do the testing? Thanks.

        Show
        Jakob Homan added a comment - Updated patch. rebased against trunk added braces around all new if statements checked for checkstyle errors, didn't see any, although it's difficult to be sure since there is a baseline of 4,789 violations in trunk. Do I need to open a new RB for this updated patch? Do the committers run the full test suite? I see lots of comments like "will commit if tests pass." Does this mean that in Hive it's up to the committers to do the testing? Thanks.
        Hide
        jiraposter@reviews.apache.org added a comment -

        -----------------------------------------------------------
        This is an automatically generated e-mail. To reply, visit:
        https://reviews.apache.org/r/790/#review1274
        -----------------------------------------------------------

        serde/src/java/org/apache/hadoop/hive/serde2/lazy/objectinspector/LazyObjectInspectorFactory.java
        <https://reviews.apache.org/r/790/#comment2908>

        Use curly brackets for all if statements (even when the body only has one line). Use ant checkstyle to find more of these.

        • John

        On 2011-05-26 20:16:20, Jakob Homan wrote:

        -----------------------------------------------------------

        This is an automatically generated e-mail. To reply, visit:

        https://reviews.apache.org/r/790/

        -----------------------------------------------------------

        (Updated 2011-05-26 20:16:20)

        Review request for hive.

        Summary

        -------

        HIVE-2171: Allow custom serdes to set field comments

        This addresses bug HIVE-2171.

        https://issues.apache.org/jira/browse/HIVE-2171

        Diffs

        -----

        ivy/libraries.properties af856bd

        metastore/src/java/org/apache/hadoop/hive/metastore/MetaStoreUtils.java c1fa4e5

        serde/ivy.xml d6c836a

        serde/src/java/org/apache/hadoop/hive/serde2/lazy/objectinspector/LazyObjectInspectorFactory.java 4850601

        serde/src/java/org/apache/hadoop/hive/serde2/lazy/objectinspector/LazySimpleStructObjectInspector.java e2fa9db

        serde/src/java/org/apache/hadoop/hive/serde2/lazybinary/objectinspector/LazyBinaryObjectInspectorFactory.java 2947e49

        serde/src/java/org/apache/hadoop/hive/serde2/lazybinary/objectinspector/LazyBinaryStructObjectInspector.java 3d5408f

        serde/src/java/org/apache/hadoop/hive/serde2/objectinspector/ColumnarStructObjectInspector.java 66f4f8d

        serde/src/java/org/apache/hadoop/hive/serde2/objectinspector/MetadataListStructObjectInspector.java bd42a0c

        serde/src/java/org/apache/hadoop/hive/serde2/objectinspector/ObjectInspectorFactory.java 90561a1

        serde/src/java/org/apache/hadoop/hive/serde2/objectinspector/ReflectionStructObjectInspector.java 4a934c5

        serde/src/java/org/apache/hadoop/hive/serde2/objectinspector/StandardStructObjectInspector.java 3b26e45

        serde/src/java/org/apache/hadoop/hive/serde2/objectinspector/StructField.java 62c3017

        serde/src/java/org/apache/hadoop/hive/serde2/objectinspector/UnionStructObjectInspector.java 76ff736

        serde/src/test/org/apache/hadoop/hive/serde2/TestSerdeWithFieldComments.java PRE-CREATION

        serde/src/test/org/apache/hadoop/hive/serde2/objectinspector/TestStandardObjectInspectors.java f139ea5

        Diff: https://reviews.apache.org/r/790/diff

        Testing

        -------

        New unit test and refactor existing unit test.

        Thanks,

        Jakob

        Show
        jiraposter@reviews.apache.org added a comment - ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/790/#review1274 ----------------------------------------------------------- serde/src/java/org/apache/hadoop/hive/serde2/lazy/objectinspector/LazyObjectInspectorFactory.java < https://reviews.apache.org/r/790/#comment2908 > Use curly brackets for all if statements (even when the body only has one line). Use ant checkstyle to find more of these. John On 2011-05-26 20:16:20, Jakob Homan wrote: ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/790/ ----------------------------------------------------------- (Updated 2011-05-26 20:16:20) Review request for hive. Summary ------- HIVE-2171 : Allow custom serdes to set field comments This addresses bug HIVE-2171 . https://issues.apache.org/jira/browse/HIVE-2171 Diffs ----- ivy/libraries.properties af856bd metastore/src/java/org/apache/hadoop/hive/metastore/MetaStoreUtils.java c1fa4e5 serde/ivy.xml d6c836a serde/src/java/org/apache/hadoop/hive/serde2/lazy/objectinspector/LazyObjectInspectorFactory.java 4850601 serde/src/java/org/apache/hadoop/hive/serde2/lazy/objectinspector/LazySimpleStructObjectInspector.java e2fa9db serde/src/java/org/apache/hadoop/hive/serde2/lazybinary/objectinspector/LazyBinaryObjectInspectorFactory.java 2947e49 serde/src/java/org/apache/hadoop/hive/serde2/lazybinary/objectinspector/LazyBinaryStructObjectInspector.java 3d5408f serde/src/java/org/apache/hadoop/hive/serde2/objectinspector/ColumnarStructObjectInspector.java 66f4f8d serde/src/java/org/apache/hadoop/hive/serde2/objectinspector/MetadataListStructObjectInspector.java bd42a0c serde/src/java/org/apache/hadoop/hive/serde2/objectinspector/ObjectInspectorFactory.java 90561a1 serde/src/java/org/apache/hadoop/hive/serde2/objectinspector/ReflectionStructObjectInspector.java 4a934c5 serde/src/java/org/apache/hadoop/hive/serde2/objectinspector/StandardStructObjectInspector.java 3b26e45 serde/src/java/org/apache/hadoop/hive/serde2/objectinspector/StructField.java 62c3017 serde/src/java/org/apache/hadoop/hive/serde2/objectinspector/UnionStructObjectInspector.java 76ff736 serde/src/test/org/apache/hadoop/hive/serde2/TestSerdeWithFieldComments.java PRE-CREATION serde/src/test/org/apache/hadoop/hive/serde2/objectinspector/TestStandardObjectInspectors.java f139ea5 Diff: https://reviews.apache.org/r/790/diff Testing ------- New unit test and refactor existing unit test. Thanks, Jakob
        Hide
        John Sichi added a comment -

        I tried applying the patch against trunk for testing, but hit some conflicts...can you rebase?

        Also, @Yongqiang: do you have any more comments on this one?

        Show
        John Sichi added a comment - I tried applying the patch against trunk for testing, but hit some conflicts...can you rebase? Also, @Yongqiang: do you have any more comments on this one?
        Hide
        John Sichi added a comment -

        I'll take a look.

        Show
        John Sichi added a comment - I'll take a look.
        Hide
        Jakob Homan added a comment -

        Is there anything else that needs to be done to get a code review on this patch?

        Show
        Jakob Homan added a comment - Is there anything else that needs to be done to get a code review on this patch?
        Hide
        Jakob Homan added a comment -

        And can we modify the desc table commands to show comments from StorageDescriptor, instead of putting a "from deserializer"?

        I opened HIVE-2189 for this issue.

        Show
        Jakob Homan added a comment - And can we modify the desc table commands to show comments from StorageDescriptor, instead of putting a "from deserializer"? I opened HIVE-2189 for this issue.
        Hide
        Jakob Homan added a comment -

        hive already stores each column's comment in StorageDescriptor. Can we get the comments from there instead of putting it to StructField?

        Hive stores column information for native serdes (see SerDeUtils.java:99). For non-native serdes, it queries the serde's object inspector, which is what this patch is dealing with.

        Show
        Jakob Homan added a comment - hive already stores each column's comment in StorageDescriptor. Can we get the comments from there instead of putting it to StructField? Hive stores column information for native serdes (see SerDeUtils.java:99 ). For non-native serdes, it queries the serde's object inspector, which is what this patch is dealing with.
        Hide
        He Yongqiang added a comment -

        hive already stores each column's comment in StorageDescriptor. Can we get the comments from there instead of putting it to StructField?

        And can we modify the desc table commands to show comments from StorageDescriptor, instead of putting a "from deserializer"?

        Show
        He Yongqiang added a comment - hive already stores each column's comment in StorageDescriptor. Can we get the comments from there instead of putting it to StructField? And can we modify the desc table commands to show comments from StorageDescriptor, instead of putting a "from deserializer"?
        Hide
        Jakob Homan added a comment -

        Anything else necessary to get a review on this patch?

        Show
        Jakob Homan added a comment - Anything else necessary to get a review on this patch?
        Hide
        jiraposter@reviews.apache.org added a comment -

        -----------------------------------------------------------
        This is an automatically generated e-mail. To reply, visit:
        https://reviews.apache.org/r/790/
        -----------------------------------------------------------

        Review request for hive.

        Summary
        -------

        HIVE-2171: Allow custom serdes to set field comments

        This addresses bug HIVE-2171.
        https://issues.apache.org/jira/browse/HIVE-2171

        Diffs


        ivy/libraries.properties af856bd
        metastore/src/java/org/apache/hadoop/hive/metastore/MetaStoreUtils.java c1fa4e5
        serde/ivy.xml d6c836a
        serde/src/java/org/apache/hadoop/hive/serde2/lazy/objectinspector/LazyObjectInspectorFactory.java 4850601
        serde/src/java/org/apache/hadoop/hive/serde2/lazy/objectinspector/LazySimpleStructObjectInspector.java e2fa9db
        serde/src/java/org/apache/hadoop/hive/serde2/lazybinary/objectinspector/LazyBinaryObjectInspectorFactory.java 2947e49
        serde/src/java/org/apache/hadoop/hive/serde2/lazybinary/objectinspector/LazyBinaryStructObjectInspector.java 3d5408f
        serde/src/java/org/apache/hadoop/hive/serde2/objectinspector/ColumnarStructObjectInspector.java 66f4f8d
        serde/src/java/org/apache/hadoop/hive/serde2/objectinspector/MetadataListStructObjectInspector.java bd42a0c
        serde/src/java/org/apache/hadoop/hive/serde2/objectinspector/ObjectInspectorFactory.java 90561a1
        serde/src/java/org/apache/hadoop/hive/serde2/objectinspector/ReflectionStructObjectInspector.java 4a934c5
        serde/src/java/org/apache/hadoop/hive/serde2/objectinspector/StandardStructObjectInspector.java 3b26e45
        serde/src/java/org/apache/hadoop/hive/serde2/objectinspector/StructField.java 62c3017
        serde/src/java/org/apache/hadoop/hive/serde2/objectinspector/UnionStructObjectInspector.java 76ff736
        serde/src/test/org/apache/hadoop/hive/serde2/TestSerdeWithFieldComments.java PRE-CREATION
        serde/src/test/org/apache/hadoop/hive/serde2/objectinspector/TestStandardObjectInspectors.java f139ea5

        Diff: https://reviews.apache.org/r/790/diff

        Testing
        -------

        New unit test and refactor existing unit test.

        Thanks,

        Jakob

        Show
        jiraposter@reviews.apache.org added a comment - ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/790/ ----------------------------------------------------------- Review request for hive. Summary ------- HIVE-2171 : Allow custom serdes to set field comments This addresses bug HIVE-2171 . https://issues.apache.org/jira/browse/HIVE-2171 Diffs ivy/libraries.properties af856bd metastore/src/java/org/apache/hadoop/hive/metastore/MetaStoreUtils.java c1fa4e5 serde/ivy.xml d6c836a serde/src/java/org/apache/hadoop/hive/serde2/lazy/objectinspector/LazyObjectInspectorFactory.java 4850601 serde/src/java/org/apache/hadoop/hive/serde2/lazy/objectinspector/LazySimpleStructObjectInspector.java e2fa9db serde/src/java/org/apache/hadoop/hive/serde2/lazybinary/objectinspector/LazyBinaryObjectInspectorFactory.java 2947e49 serde/src/java/org/apache/hadoop/hive/serde2/lazybinary/objectinspector/LazyBinaryStructObjectInspector.java 3d5408f serde/src/java/org/apache/hadoop/hive/serde2/objectinspector/ColumnarStructObjectInspector.java 66f4f8d serde/src/java/org/apache/hadoop/hive/serde2/objectinspector/MetadataListStructObjectInspector.java bd42a0c serde/src/java/org/apache/hadoop/hive/serde2/objectinspector/ObjectInspectorFactory.java 90561a1 serde/src/java/org/apache/hadoop/hive/serde2/objectinspector/ReflectionStructObjectInspector.java 4a934c5 serde/src/java/org/apache/hadoop/hive/serde2/objectinspector/StandardStructObjectInspector.java 3b26e45 serde/src/java/org/apache/hadoop/hive/serde2/objectinspector/StructField.java 62c3017 serde/src/java/org/apache/hadoop/hive/serde2/objectinspector/UnionStructObjectInspector.java 76ff736 serde/src/test/org/apache/hadoop/hive/serde2/TestSerdeWithFieldComments.java PRE-CREATION serde/src/test/org/apache/hadoop/hive/serde2/objectinspector/TestStandardObjectInspectors.java f139ea5 Diff: https://reviews.apache.org/r/790/diff Testing ------- New unit test and refactor existing unit test. Thanks, Jakob
        Hide
        Jakob Homan added a comment -

        Patch:

        • Adds comment field to StructField interface and implements reasonable versions to each of its implementations.
        • Adds overloaded versions of each of the struct-based ObjectInspector factories to allow the comments to be set.
        • Adjusts MetastoreUtils to check if the comment of the field is null, if so, maintains previous behavior, else uses the comment.
        • Adds new unit test for MetastoreUtils. For this, mockito was added as a dependency. Right now it looks like Hive's Ivy conf isn't set up to only include some jars in the package. If this patch goes in, I'll open another jira to make sure the mockito and other test-related jars aren't included in jars they don't need to be.
        • Refactors the TestStandardObjectInspectors test to test both with and without comments.

        After this patch, a serde that wants to specify comments can and have them show up in the table description. For example, with a table kst created by an implementation of SerDe, that has an example for each type (the comments are all separate, they're all just boring: this is field BLAH) can now set the field comments:

        hive> describe kst;
        OK
        string1 string  this field is string1
        string2 string  this field is string2
        int1    int     this field is int1
        boolean1        boolean this field is boolean1
        long1   bigint  this field is long1
        float1  float   this field is float1
        double1 double  this field is double1
        inner_record1   struct<int_in_inner_record1:int,string_in_inner_record1:string> this field is inner_record1
        enum1   string  this field is enum1
        array1  array<string>   this field is array1
        map1    map<string,string>      this field is map1
        union1  uniontype<float,boolean,string> this field is union1
        fixed1  array<tinyint>  this field is fixed1
        null1   void    this field is null1
        unionnullint    int     this field is UnionNullInt
        bytes1  array<tinyint>  this field is bytes1
        ds      string
        Time taken: 0.286 seconds

        One thing I noticed is that these field comments on structs should extended to substructures, and does with this new patch for custom serdes:

        hive> describe kst.inner_record1;
        OK
        int_in_inner_record1    int     this field is int_in_inner_record1
        string_in_inner_record1 string  this field is string_in_inner_record1
        Time taken: 0.113 seconds

        However, this doesn't work correctly with built-in serdes:

        hive> create table test_table(a STRUCT<z:string COMMENT 'comment for z',x:int> COMMENT 'comment for a');
        OK
        Time taken: 2.565 seconds
        hive> describe test_table;
        OK
        a	struct<z:string,x:int>	comment for a
        Time taken: 0.139 seconds
        hive> describe test_table.a;
        OK
        z	string	from deserializer
        x	int	from deserializer
        Time taken: 0.096 seconds
        hive> describe test_table.a.z;
        OK
        z	string	from deserializer
        Time taken: 0.089 seconds
        hive>

        The comment for field z is lost, replaced by the boilerplate text "from deserializer" and can't be retrieved from the CLI. I'll open a JIRA for this.

        This is my first Hive patch, so please check to see if I missed anything.

        Show
        Jakob Homan added a comment - Patch: Adds comment field to StructField interface and implements reasonable versions to each of its implementations. Adds overloaded versions of each of the struct-based ObjectInspector factories to allow the comments to be set. Adjusts MetastoreUtils to check if the comment of the field is null, if so, maintains previous behavior, else uses the comment. Adds new unit test for MetastoreUtils. For this, mockito was added as a dependency. Right now it looks like Hive's Ivy conf isn't set up to only include some jars in the package. If this patch goes in, I'll open another jira to make sure the mockito and other test-related jars aren't included in jars they don't need to be. Refactors the TestStandardObjectInspectors test to test both with and without comments. After this patch, a serde that wants to specify comments can and have them show up in the table description. For example, with a table kst created by an implementation of SerDe, that has an example for each type (the comments are all separate, they're all just boring: this is field BLAH) can now set the field comments: hive> describe kst; OK string1 string this field is string1 string2 string this field is string2 int1 int this field is int1 boolean1 boolean this field is boolean1 long1 bigint this field is long1 float1 float this field is float1 double1 double this field is double1 inner_record1 struct<int_in_inner_record1:int,string_in_inner_record1:string> this field is inner_record1 enum1 string this field is enum1 array1 array<string> this field is array1 map1 map<string,string> this field is map1 union1 uniontype<float,boolean,string> this field is union1 fixed1 array<tinyint> this field is fixed1 null1 void this field is null1 unionnullint int this field is UnionNullInt bytes1 array<tinyint> this field is bytes1 ds string Time taken: 0.286 seconds One thing I noticed is that these field comments on structs should extended to substructures, and does with this new patch for custom serdes: hive> describe kst.inner_record1; OK int_in_inner_record1 int this field is int_in_inner_record1 string_in_inner_record1 string this field is string_in_inner_record1 Time taken: 0.113 seconds However, this doesn't work correctly with built-in serdes: hive> create table test_table(a STRUCT<z:string COMMENT 'comment for z',x:int> COMMENT 'comment for a'); OK Time taken: 2.565 seconds hive> describe test_table; OK a struct<z:string,x:int> comment for a Time taken: 0.139 seconds hive> describe test_table.a; OK z string from deserializer x int from deserializer Time taken: 0.096 seconds hive> describe test_table.a.z; OK z string from deserializer Time taken: 0.089 seconds hive> The comment for field z is lost, replaced by the boilerplate text "from deserializer" and can't be retrieved from the CLI. I'll open a JIRA for this. This is my first Hive patch, so please check to see if I missed anything.

          People

          • Assignee:
            Jakob Homan
            Reporter:
            Jakob Homan
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development