[GitHub] carbondata pull request #1437: [CARBONDATA-1618] Fix issue of not support ta...

[qiuchenjian-2]

GitHub user chenerlu opened a pull request:

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

    [CARBONDATA-1618] Fix issue of not support table comment

    Background: Current carbon do not support table comment when create table.
    This PR will support table comment.

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

    $ git pull https://github.com/chenerlu/incubator-carbondata tablecomment

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

    https://github.com/apache/carbondata/pull/1437.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 #1437
   
----

----


---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

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



---

[qiuchenjian-2]

Github user ravipesala commented on the issue:

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



---

[qiuchenjian-2]

Github user chenerlu commented on the issue:

    https://github.com/apache/carbondata/pull/1437
 
    retest this please


---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

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



---

[qiuchenjian-2]

Github user ravipesala commented on the issue:

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



---

[qiuchenjian-2]

Github user chenerlu commented on the issue:

    https://github.com/apache/carbondata/pull/1437
 
    retest this please


---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

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



---

[qiuchenjian-2]

Github user chenliang613 commented on the issue:

    https://github.com/apache/carbondata/pull/1437
 
    @sounakr  please review it.


---

[qiuchenjian-2]

Github user ravipesala commented on the issue:

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



---

[qiuchenjian-2]

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

    https://github.com/apache/carbondata/pull/1437#discussion_r147350873
 
    --- Diff: integration/spark-common-test/src/test/scala/org/apache/carbondata/spark/testsuite/createTable/TestCreateTableWithTableComment.scala ---
    @@ -0,0 +1,74 @@
    +/*
    + * Licensed to the Apache Software Foundation (ASF) under one or more
    + * contributor license agreements.  See the NOTICE file distributed with
    + * this work for additional information regarding copyright ownership.
    + * The ASF licenses this file to You under the Apache License, Version 2.0
    + * (the "License"); you may not use this file except in compliance with
    + * the License.  You may obtain a copy of the License at
    + *
    + *    http://www.apache.org/licenses/LICENSE-2.0
    + *
    + * Unless required by applicable law or agreed to in writing, software
    + * distributed under the License is distributed on an "AS IS" BASIS,
    + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
    + * See the License for the specific language governing permissions and
    + * limitations under the License.
    + */
    +
    +package org.apache.carbondata.spark.testsuite.createTable
    +
    +import org.apache.spark.sql.test.util.QueryTest
    +import org.scalatest.BeforeAndAfterAll
    +
    +/**
    + * test functionality for create table with table comment
    + */
    +class TestCreateTableWithTableComment extends QueryTest with BeforeAndAfterAll {
    +
    +  override def beforeAll {
    +    sql("use default")
    +    sql("drop table if exists withTableComment")
    +    sql("drop table if exists withoutTableComment")
    +  }
    +
    +  test("test create table with table comment") {
    +    sql(
    +      s"""
    +         | create table withTableComment(
    +         | id int,
    +         | name string
    +         | )
    +         | comment "This table has table comment"
    +         | STORED BY 'carbondata'
    +       """.stripMargin
    +    )
    +
    +    val result = sql("describe formatted withTableComment")
    +
    +    checkExistence(result, true, "Comment:")
    +    checkExistence(result, true, "This table has table comment")
    +  }
    +
    +  test("test create table without table comment") {
    +    sql(
    +      s"""
    +         | create table withoutTableComment(
    +         | id int,
    +         | name string
    +         | )
    +         | STORED BY 'carbondata'
    +       """.stripMargin
    +    )
    +
    --- End diff --
   
    Can existing table will be able to add comment through Alter table command?


---

[qiuchenjian-2]

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

    https://github.com/apache/carbondata/pull/1437#discussion_r147354409
 
    --- Diff: integration/spark-common/src/main/scala/org/apache/spark/sql/catalyst/CarbonDDLSqlParser.scala ---
    @@ -287,7 +288,8 @@ abstract class CarbonDDLSqlParser extends AbstractCarbonSparkSQLParser {
           groupCols,
           Some(colProps),
           bucketFields: Option[BucketFields],
    -      partitionInfo)
    +      partitionInfo,
    +      comment)
    --- End diff --
   
    Please check if the all the callers for TableModel case class is covered the "comment" parameter or have handled it. For e.g. in def createTableInfoFromParam.


---

[qiuchenjian-2]

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

    https://github.com/apache/carbondata/pull/1437#discussion_r147354669
 
    --- Diff: integration/spark-common/src/main/scala/org/apache/spark/sql/catalyst/CarbonDDLSqlParser.scala ---
    @@ -247,7 +247,8 @@ abstract class CarbonDDLSqlParser extends AbstractCarbonSparkSQLParser {
           , tableName: String, fields: Seq[Field],
           partitionCols: Seq[PartitionerField],
           tableProperties: mutable.Map[String, String],
    -      bucketFields: Option[BucketFields], isAlterFlow: Boolean = false): TableModel = {
    +      bucketFields: Option[BucketFields], isAlterFlow: Boolean = false,
    +      comment: Option[String] = None): TableModel = {
    --- End diff --
   
    Better to rename it to tableComment in order to avoid confusion with Column Comment which will come in future.


---

[qiuchenjian-2]

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

    https://github.com/apache/carbondata/pull/1437#discussion_r147435775
 
    --- Diff: integration/spark-common/src/main/scala/org/apache/spark/sql/catalyst/CarbonDDLSqlParser.scala ---
    @@ -247,7 +247,8 @@ abstract class CarbonDDLSqlParser extends AbstractCarbonSparkSQLParser {
           , tableName: String, fields: Seq[Field],
           partitionCols: Seq[PartitionerField],
           tableProperties: mutable.Map[String, String],
    -      bucketFields: Option[BucketFields], isAlterFlow: Boolean = false): TableModel = {
    +      bucketFields: Option[BucketFields], isAlterFlow: Boolean = false,
    +      comment: Option[String] = None): TableModel = {
    --- End diff --
   
    Carbon already support column comment.


---

[qiuchenjian-2]

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

    https://github.com/apache/carbondata/pull/1437#discussion_r147436112
 
    --- Diff: integration/spark-common-test/src/test/scala/org/apache/carbondata/spark/testsuite/createTable/TestCreateTableWithTableComment.scala ---
    @@ -0,0 +1,74 @@
    +/*
    + * Licensed to the Apache Software Foundation (ASF) under one or more
    + * contributor license agreements.  See the NOTICE file distributed with
    + * this work for additional information regarding copyright ownership.
    + * The ASF licenses this file to You under the Apache License, Version 2.0
    + * (the "License"); you may not use this file except in compliance with
    + * the License.  You may obtain a copy of the License at
    + *
    + *    http://www.apache.org/licenses/LICENSE-2.0
    + *
    + * Unless required by applicable law or agreed to in writing, software
    + * distributed under the License is distributed on an "AS IS" BASIS,
    + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
    + * See the License for the specific language governing permissions and
    + * limitations under the License.
    + */
    +
    +package org.apache.carbondata.spark.testsuite.createTable
    +
    +import org.apache.spark.sql.test.util.QueryTest
    +import org.scalatest.BeforeAndAfterAll
    +
    +/**
    + * test functionality for create table with table comment
    + */
    +class TestCreateTableWithTableComment extends QueryTest with BeforeAndAfterAll {
    +
    +  override def beforeAll {
    +    sql("use default")
    +    sql("drop table if exists withTableComment")
    +    sql("drop table if exists withoutTableComment")
    +  }
    +
    +  test("test create table with table comment") {
    +    sql(
    +      s"""
    +         | create table withTableComment(
    +         | id int,
    +         | name string
    +         | )
    +         | comment "This table has table comment"
    +         | STORED BY 'carbondata'
    +       """.stripMargin
    +    )
    +
    +    val result = sql("describe formatted withTableComment")
    +
    +    checkExistence(result, true, "Comment:")
    +    checkExistence(result, true, "This table has table comment")
    +  }
    +
    +  test("test create table without table comment") {
    +    sql(
    +      s"""
    +         | create table withoutTableComment(
    +         | id int,
    +         | name string
    +         | )
    +         | STORED BY 'carbondata'
    +       """.stripMargin
    +    )
    +
    --- End diff --
   
    This PR not contains this functions.


---

[qiuchenjian-2]

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

    https://github.com/apache/carbondata/pull/1437#discussion_r147437929
 
    --- Diff: integration/spark-common/src/main/scala/org/apache/spark/sql/catalyst/CarbonDDLSqlParser.scala ---
    @@ -247,7 +247,8 @@ abstract class CarbonDDLSqlParser extends AbstractCarbonSparkSQLParser {
           , tableName: String, fields: Seq[Field],
           partitionCols: Seq[PartitionerField],
           tableProperties: mutable.Map[String, String],
    -      bucketFields: Option[BucketFields], isAlterFlow: Boolean = false): TableModel = {
    +      bucketFields: Option[BucketFields], isAlterFlow: Boolean = false,
    +      comment: Option[String] = None): TableModel = {
    --- End diff --
   
    Have renamed.


---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

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



---

[qiuchenjian-2]

Github user ravipesala commented on the issue:

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



---

[qiuchenjian-2]

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

    https://github.com/apache/carbondata/pull/1437#discussion_r147570132
 
    --- Diff: integration/spark-common-test/src/test/scala/org/apache/carbondata/spark/testsuite/createTable/TestCreateTableWithTableComment.scala ---
    @@ -0,0 +1,74 @@
    +/*
    + * Licensed to the Apache Software Foundation (ASF) under one or more
    + * contributor license agreements.  See the NOTICE file distributed with
    + * this work for additional information regarding copyright ownership.
    + * The ASF licenses this file to You under the Apache License, Version 2.0
    + * (the "License"); you may not use this file except in compliance with
    + * the License.  You may obtain a copy of the License at
    + *
    + *    http://www.apache.org/licenses/LICENSE-2.0
    + *
    + * Unless required by applicable law or agreed to in writing, software
    + * distributed under the License is distributed on an "AS IS" BASIS,
    + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
    + * See the License for the specific language governing permissions and
    + * limitations under the License.
    + */
    +
    +package org.apache.carbondata.spark.testsuite.createTable
    +
    +import org.apache.spark.sql.test.util.QueryTest
    +import org.scalatest.BeforeAndAfterAll
    +
    +/**
    + * test functionality for create table with table comment
    + */
    +class TestCreateTableWithTableComment extends QueryTest with BeforeAndAfterAll {
    +
    +  override def beforeAll {
    +    sql("use default")
    +    sql("drop table if exists withTableComment")
    +    sql("drop table if exists withoutTableComment")
    +  }
    +
    +  test("test create table with table comment") {
    +    sql(
    +      s"""
    +         | create table withTableComment(
    +         | id int,
    +         | name string
    +         | )
    +         | comment "This table has table comment"
    +         | STORED BY 'carbondata'
    +       """.stripMargin
    +    )
    +
    +    val result = sql("describe formatted withTableComment")
    +
    +    checkExistence(result, true, "Comment:")
    +    checkExistence(result, true, "This table has table comment")
    +  }
    +
    +  test("test create table without table comment") {
    +    sql(
    +      s"""
    +         | create table withoutTableComment(
    +         | id int,
    +         | name string
    +         | )
    +         | STORED BY 'carbondata'
    +       """.stripMargin
    +    )
    +
    +    val result = sql("describe formatted withoutTableComment")
    +
    +    checkExistence(result, true, "Comment:")
    --- End diff --
   
    Can you assert the string include "This table has table comment" also?


---

[qiuchenjian-2]

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

    https://github.com/apache/carbondata/pull/1437#discussion_r147736082
 
    --- Diff: integration/spark-common-test/src/test/scala/org/apache/carbondata/spark/testsuite/createTable/TestCreateTableWithTableComment.scala ---
    @@ -0,0 +1,74 @@
    +/*
    + * Licensed to the Apache Software Foundation (ASF) under one or more
    + * contributor license agreements.  See the NOTICE file distributed with
    + * this work for additional information regarding copyright ownership.
    + * The ASF licenses this file to You under the Apache License, Version 2.0
    + * (the "License"); you may not use this file except in compliance with
    + * the License.  You may obtain a copy of the License at
    + *
    + *    http://www.apache.org/licenses/LICENSE-2.0
    + *
    + * Unless required by applicable law or agreed to in writing, software
    + * distributed under the License is distributed on an "AS IS" BASIS,
    + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
    + * See the License for the specific language governing permissions and
    + * limitations under the License.
    + */
    +
    +package org.apache.carbondata.spark.testsuite.createTable
    +
    +import org.apache.spark.sql.test.util.QueryTest
    +import org.scalatest.BeforeAndAfterAll
    +
    +/**
    + * test functionality for create table with table comment
    + */
    +class TestCreateTableWithTableComment extends QueryTest with BeforeAndAfterAll {
    +
    +  override def beforeAll {
    +    sql("use default")
    +    sql("drop table if exists withTableComment")
    +    sql("drop table if exists withoutTableComment")
    +  }
    +
    +  test("test create table with table comment") {
    +    sql(
    +      s"""
    +         | create table withTableComment(
    +         | id int,
    +         | name string
    +         | )
    +         | comment "This table has table comment"
    +         | STORED BY 'carbondata'
    +       """.stripMargin
    +    )
    +
    +    val result = sql("describe formatted withTableComment")
    +
    +    checkExistence(result, true, "Comment:")
    +    checkExistence(result, true, "This table has table comment")
    +  }
    +
    +  test("test create table without table comment") {
    +    sql(
    +      s"""
    +         | create table withoutTableComment(
    +         | id int,
    +         | name string
    +         | )
    +         | STORED BY 'carbondata'
    +       """.stripMargin
    +    )
    +
    +    val result = sql("describe formatted withoutTableComment")
    +
    +    checkExistence(result, true, "Comment:")
    --- End diff --
   
    Done


---