[GitHub] carbondata pull request #1432: [WIP][CARBONDATA-1608]Support Column Comment ...

[qiuchenjian-2]

GitHub user akashrn5 opened a pull request:

    https://github.com/apache/carbondata/pull/1432

    [WIP][CARBONDATA-1608]Support Column Comment for Create Table

    add column comment during carbon create table and when table is described if comment is not mentioned, default comment will be null
   
    Be sure to do all of the following to help us incorporate your contribution
    quickly and easily:
   
     - [*] Make sure the PR title is formatted like:
       `[CARBONDATA-<Jira issue #>] Description of pull request`
       
     - [*] Make sure to add PR description including
          -create carbon table with column comment is added in this PR, previously there was no option for column comment, now it has been added
    -for implementing this, the parser has been edited to get column comment and instead of checking its encoding technique to display in desc sql, we can just show the comment for that specific column if any, if there are no comment for the columns, the default value is null  
            - the root cause/problem statement
            - What is the implemented solution
   
     - [*] Any interfaces changed?
     NONE
     - [*] Any backward compatibility impacted?
     NONE
     - [*] Document update required?
    NONE
     - [*] Testing done
     
    test cases are added for this
            Please provide details on
            - Whether new unit test cases have been added or why no new tests are required?
            - How it is tested? Please attach test report.
            - Is it a performance related change? Please attach the performance test report.
            - Any additional information to help reviewers in testing this change.
             
     - [*] For large changes, please consider breaking it into sub-tasks under an umbrella JIRA.
                     
    ---


You can merge this pull request into a Git repository by running:

    $ git pull https://github.com/akashrn5/incubator-carbondata col_comment

Alternatively you can review and apply these changes as the patch at:

    https://github.com/apache/carbondata/pull/1432.patch

To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:

    This closes #1432
   
----
commit 5cc41bfc4f117a5a1d6ff6bdc67fcb7ba61d343d
Author: akashrn5 <[hidden email]>
Date:   2017-10-24T10:26:53Z

    add column comment during carbon create table and when table is described if comment is not mentioned, default will be null

----


---

[qiuchenjian-2]

Github user ravipesala commented on the issue:

    https://github.com/apache/carbondata/pull/1432
 
    SDV Build Fail , Please check CI http://144.76.159.231:8080/job/ApacheSDVTests/1300/



---

[qiuchenjian-2]

Github user ravipesala commented on the issue:

    https://github.com/apache/carbondata/pull/1432
 
    SDV Build Fail , Please check CI http://144.76.159.231:8080/job/ApacheSDVTests/1301/



---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

    https://github.com/apache/carbondata/pull/1432
 
    Build Failed  with Spark 2.1.0, Please check CI http://136.243.101.176:8080/job/ApacheCarbonPRBuilder1/664/



---

[qiuchenjian-2]

Github user ravipesala commented on the issue:

    https://github.com/apache/carbondata/pull/1432
 
    SDV Build Fail , Please check CI http://144.76.159.231:8080/job/ApacheSDVTests/1302/



---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

    https://github.com/apache/carbondata/pull/1432
 
    Build Failed  with Spark 2.1.0, Please check CI http://136.243.101.176:8080/job/ApacheCarbonPRBuilder1/665/



---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

    https://github.com/apache/carbondata/pull/1432
 
    Build Success with Spark 2.1.0, Please check CI http://136.243.101.176:8080/job/ApacheCarbonPRBuilder1/673/



---

[qiuchenjian-2]

Github user akashrn5 commented on the issue:

    https://github.com/apache/carbondata/pull/1432
 
    @manishgupta88 please review


---

[qiuchenjian-2]

Github user ravipesala commented on the issue:

    https://github.com/apache/carbondata/pull/1432
 
    SDV Build Success , Please check CI http://144.76.159.231:8080/job/ApacheSDVTests/1310/



---

[qiuchenjian-2]

Github user akashrn5 commented on the issue:

    https://github.com/apache/carbondata/pull/1432
 
    @jackylk please review


---

[qiuchenjian-2]

Github user akashrn5 commented on the issue:

    https://github.com/apache/carbondata/pull/1432
 
    @gvramana please review


---

[qiuchenjian-2]

Github user akashrn5 commented on the issue:

    https://github.com/apache/carbondata/pull/1432
 
    @ravipesala please review


---

[qiuchenjian-2]

Github user gvramana commented on a diff in the pull request:

    https://github.com/apache/carbondata/pull/1432#discussion_r150860743
 
    --- Diff: integration/spark2/src/main/scala/org/apache/spark/sql/parser/CarbonSpark2SqlParser.scala ---
    @@ -424,11 +424,20 @@ class CarbonSpark2SqlParser extends CarbonDDLSqlParser {
     
       def getFields(schema: Seq[StructField]): Seq[Field] = {
         schema.map { col =>
    -      val x = if (col.dataType.catalogString == "float") {
    -        '`' + col.name + '`' + " double"
    -      }
    -      else {
    -        '`' + col.name + '`' + ' ' + col.dataType.catalogString
    +      val colComment = col.getComment()
    +      val x = if (colComment.isDefined) {
    +        val columnComment = " comment \"" + colComment.get + "\""
    --- End diff --
   
    No need to repeat logic, just add columnComment as empty string if no colComment


---

[qiuchenjian-2]

Github user gvramana commented on a diff in the pull request:

    https://github.com/apache/carbondata/pull/1432#discussion_r150862967
 
    --- Diff: integration/spark2/src/test/scala/org/apache/carbondata/spark/testsuite/booleantype/BooleanDataTypesInsertTest.scala ---
    @@ -945,4 +948,41 @@ class BooleanDataTypesInsertTest extends QueryTest with BeforeAndAfterEach with
         )
       }
     
    +  test("Inserting table with bad records, and SORT_COLUMNS is boolean column") {
    +    CarbonProperties.getInstance().addProperty(CarbonCommonConstants.ENABLE_AUTO_LOAD_MERGE, "true")
    --- End diff --
   
    How is this test related to comments?


---

[qiuchenjian-2]

Github user gvramana commented on a diff in the pull request:

    https://github.com/apache/carbondata/pull/1432#discussion_r150863854
 
    --- Diff: integration/spark2/src/main/scala/org/apache/spark/sql/execution/command/CarbonDescribeFormattedCommand.scala ---
    @@ -65,6 +65,7 @@ private[sql] case class CarbonDescribeFormattedCommand(
         val dims = relation.metaData.dims.map(x => x.toLowerCase)
         var results: Seq[(String, String, String)] = child.schema.fields.map { field =>
           val fieldName = field.name.toLowerCase
    +      val colComment = field.getComment().getOrElse("null")
    --- End diff --
   
    Also needs to make sure that "desc table" also works. As spark/Hive supports accordingly, need to make it consistant


---

[qiuchenjian-2]

Github user akashrn5 commented on a diff in the pull request:

    https://github.com/apache/carbondata/pull/1432#discussion_r150883724
 
    --- Diff: integration/spark2/src/test/scala/org/apache/carbondata/spark/testsuite/booleantype/BooleanDataTypesInsertTest.scala ---
    @@ -945,4 +948,41 @@ class BooleanDataTypesInsertTest extends QueryTest with BeforeAndAfterEach with
         )
       }
     
    +  test("Inserting table with bad records, and SORT_COLUMNS is boolean column") {
    +    CarbonProperties.getInstance().addProperty(CarbonCommonConstants.ENABLE_AUTO_LOAD_MERGE, "true")
    --- End diff --
   
    this test is not related to col comment, this test is just added in this PR which is for boolean datatype


---

[qiuchenjian-2]

Github user akashrn5 commented on a diff in the pull request:

    https://github.com/apache/carbondata/pull/1432#discussion_r151345725
 
    --- Diff: integration/spark2/src/main/scala/org/apache/spark/sql/parser/CarbonSpark2SqlParser.scala ---
    @@ -424,11 +424,20 @@ class CarbonSpark2SqlParser extends CarbonDDLSqlParser {
     
       def getFields(schema: Seq[StructField]): Seq[Field] = {
         schema.map { col =>
    -      val x = if (col.dataType.catalogString == "float") {
    -        '`' + col.name + '`' + " double"
    -      }
    -      else {
    -        '`' + col.name + '`' + ' ' + col.dataType.catalogString
    +      val colComment = col.getComment()
    +      val x = if (colComment.isDefined) {
    +        val columnComment = " comment \"" + colComment.get + "\""
    --- End diff --
   
    handled, please review


---

[qiuchenjian-2]

Github user akashrn5 commented on a diff in the pull request:

    https://github.com/apache/carbondata/pull/1432#discussion_r151345870
 
    --- Diff: integration/spark2/src/main/scala/org/apache/spark/sql/execution/command/CarbonDescribeFormattedCommand.scala ---
    @@ -65,6 +65,7 @@ private[sql] case class CarbonDescribeFormattedCommand(
         val dims = relation.metaData.dims.map(x => x.toLowerCase)
         var results: Seq[(String, String, String)] = child.schema.fields.map { field =>
           val fieldName = field.name.toLowerCase
    +      val colComment = field.getComment().getOrElse("null")
    --- End diff --
   
    verified, it is in sync with hive, if comment is not there, by default it will be null


---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

    https://github.com/apache/carbondata/pull/1432
 
    Build Failed  with Spark 2.1.0, Please check CI http://136.243.101.176:8080/job/ApacheCarbonPRBuilder1/1171/



---

[qiuchenjian-2]

Github user ravipesala commented on the issue:

    https://github.com/apache/carbondata/pull/1432
 
    SDV Build Success , Please check CI http://144.76.159.231:8080/job/ApacheSDVTests/1748/



---