[GitHub] carbondata pull request #2436: [CARBONDATA-2682]fix create table with long_s...

[qiuchenjian-2]

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

    https://github.com/apache/carbondata/pull/2436#discussion_r201542897
 
    --- Diff: integration/spark-common-test/src/test/scala/org/apache/carbondata/spark/testsuite/longstring/VarcharDataTypesBasicTestCase.scala ---
    @@ -133,6 +133,64 @@ class VarcharDataTypesBasicTestCase extends QueryTest with BeforeAndAfterEach wi
         assert(exceptionCaught.getMessage.contains("its data type is not string"))
       }
     
    +  test("long string columns cannot contain duplicate columns") {
    +    val exceptionCaught = intercept[Exception] {
    +      sql(
    +        s"""
    +           | CREATE TABLE if not exists $longStringTable(
    +           | id INT, name STRING, description STRING, address STRING, note STRING
    +           | ) STORED BY 'carbondata'
    +           | TBLPROPERTIES('LONG_STRING_COLUMNS'='address, note, Note')
    +           |""".
    +          stripMargin)
    +    }
    +    assert(exceptionCaught.getMessage.contains("long_string_columns"))
    +    assert(exceptionCaught.getMessage.contains("Duplicate columns are not allowed"))
    +  }
    +
    +  test("long_string_columns: column does not exist in table ") {
    +    val exceptionCaught = intercept[Exception] {
    +      sql(
    +        s"""
    +           | CREATE TABLE if not exists $longStringTable(
    +           | id INT, name STRING, description STRING, address STRING, note STRING
    +           | ) STORED BY 'carbondata'
    +           | TBLPROPERTIES('LONG_STRING_COLUMNS'='address, note, NoteS')
    +           |""".
    +          stripMargin)
    +    }
    +    assert(exceptionCaught.getMessage.contains("long_string_columns"))
    +    assert(exceptionCaught.getMessage.contains("does not exist in table"))
    +  }
    +
    +  test("long_string_columns: columns cannot exist in patitions columns") {
    +    val exceptionCaught = intercept[Exception] {
    --- End diff --
   
    ok


---

[qiuchenjian-2]

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

    https://github.com/apache/carbondata/pull/2436#discussion_r201542905
 
    --- Diff: integration/spark-common-test/src/test/scala/org/apache/carbondata/spark/testsuite/longstring/VarcharDataTypesBasicTestCase.scala ---
    @@ -133,6 +133,64 @@ class VarcharDataTypesBasicTestCase extends QueryTest with BeforeAndAfterEach wi
         assert(exceptionCaught.getMessage.contains("its data type is not string"))
       }
     
    +  test("long string columns cannot contain duplicate columns") {
    +    val exceptionCaught = intercept[Exception] {
    +      sql(
    +        s"""
    +           | CREATE TABLE if not exists $longStringTable(
    +           | id INT, name STRING, description STRING, address STRING, note STRING
    +           | ) STORED BY 'carbondata'
    +           | TBLPROPERTIES('LONG_STRING_COLUMNS'='address, note, Note')
    +           |""".
    +          stripMargin)
    +    }
    +    assert(exceptionCaught.getMessage.contains("long_string_columns"))
    +    assert(exceptionCaught.getMessage.contains("Duplicate columns are not allowed"))
    +  }
    +
    +  test("long_string_columns: column does not exist in table ") {
    +    val exceptionCaught = intercept[Exception] {
    +      sql(
    +        s"""
    +           | CREATE TABLE if not exists $longStringTable(
    +           | id INT, name STRING, description STRING, address STRING, note STRING
    +           | ) STORED BY 'carbondata'
    +           | TBLPROPERTIES('LONG_STRING_COLUMNS'='address, note, NoteS')
    +           |""".
    +          stripMargin)
    +    }
    +    assert(exceptionCaught.getMessage.contains("long_string_columns"))
    +    assert(exceptionCaught.getMessage.contains("does not exist in table"))
    +  }
    +
    +  test("long_string_columns: columns cannot exist in patitions columns") {
    +    val exceptionCaught = intercept[Exception] {
    +      sql(
    +        s"""
    +           | CREATE TABLE if not exists $longStringTable(
    +           | id INT, name STRING, description STRING, address STRING
    +           | ) partitioned by (note string) STORED BY 'carbondata'
    +           | TBLPROPERTIES('LONG_STRING_COLUMNS'='note')
    +           |""".
    +          stripMargin)
    +    }
    +    assert(exceptionCaught.getMessage.contains("both in partition and long_string_columns"))
    +  }
    +
    +  test("long_string_columns: columns cannot exist in no_inverted_index columns") {
    +    val exceptionCaught = intercept[Exception] {
    --- End diff --
   
    ok


---

[qiuchenjian-2]

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

    https://github.com/apache/carbondata/pull/2436#discussion_r201542948
 
    --- Diff: integration/spark-common/src/main/scala/org/apache/spark/sql/catalyst/CarbonDDLSqlParser.scala ---
    @@ -391,6 +391,24 @@ abstract class CarbonDDLSqlParser extends AbstractCarbonSparkSQLParser {
             tableProperties.get(CarbonCommonConstants.CACHE_LEVEL).get,
             tableProperties)
         }
    +    // validate long_string_columns from tale properties
    +    var longStringColumns = varcharColumns.map(column => column.toUpperCase)
    --- End diff --
   
    ok


---

[qiuchenjian-2]

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

    https://github.com/apache/carbondata/pull/2436#discussion_r201544064
 
    --- Diff: integration/spark-common/src/main/scala/org/apache/spark/sql/catalyst/CarbonDDLSqlParser.scala ---
    @@ -486,6 +504,60 @@ abstract class CarbonDDLSqlParser extends AbstractCarbonSparkSQLParser {
         }
       }
     
    +  /**
    +   * This method validates the long string columns
    +   *
    +   * @param fields
    --- End diff --
   
    ok


---

[qiuchenjian-2]

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

    https://github.com/apache/carbondata/pull/2436#discussion_r201544083
 
    --- Diff: integration/spark-common/src/main/scala/org/apache/spark/sql/catalyst/CarbonDDLSqlParser.scala ---
    @@ -486,6 +504,60 @@ abstract class CarbonDDLSqlParser extends AbstractCarbonSparkSQLParser {
         }
       }
     
    +  /**
    +   * This method validates the long string columns
    --- End diff --
   
    ok


---

[qiuchenjian-2]

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

    https://github.com/apache/carbondata/pull/2436#discussion_r201545482
 
    --- Diff: integration/spark-common/src/main/scala/org/apache/spark/sql/catalyst/CarbonDDLSqlParser.scala ---
    @@ -486,6 +504,60 @@ abstract class CarbonDDLSqlParser extends AbstractCarbonSparkSQLParser {
         }
       }
     
    +  /**
    +   * This method validates the long string columns
    +   *
    +   * @param fields
    +   * @param varcharCols
    +   * @return
    +   */
    +  private def validateLongStringColumns(fields: Seq[Field],
    +      varcharCols: Seq[String]): Unit = {
    +    var longStringColumnsMap: Map[String, Field] = Map[String, Field]()
    +    fields.foreach(field =>
    +      longStringColumnsMap.put(field.column.toUpperCase, field)
    +    )
    +    var dataTypeErr: Set[String] = Set[String]()
    +    var duplicateColumnErr: Map[String, Int] = Map[String, Int]()
    +    var nullColumnErr: Set[String] = Set[String]()
    +    var tmpStr: String = ""
    +    varcharCols.foreach {
    +      column =>
    +        tmpStr = column.toUpperCase
    +        duplicateColumnErr.get(tmpStr) match {
    +          case None => duplicateColumnErr.put(tmpStr, 1)
    +          case Some(count) => duplicateColumnErr.put(tmpStr, count + 1)
    +        }
    +        longStringColumnsMap.get(tmpStr) match {
    +          case None => nullColumnErr += column
    +          case Some(field) => if (!DataTypes.STRING.getName.equalsIgnoreCase(field.dataType.get)) {
    +            dataTypeErr += column
    +          }
    +        }
    +    }
    +    if (!nullColumnErr.isEmpty) {
    +      val errMsg = "long_string_columns:" +
    --- End diff --
   
    ok


---

[qiuchenjian-2]

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

    https://github.com/apache/carbondata/pull/2436#discussion_r202009599
 
    --- Diff: integration/spark-common/src/main/scala/org/apache/spark/sql/catalyst/CarbonDDLSqlParser.scala ---
    @@ -486,6 +504,60 @@ abstract class CarbonDDLSqlParser extends AbstractCarbonSparkSQLParser {
         }
       }
     
    +  /**
    +   * This method validates the long string columns
    +   *
    +   * @param fields
    +   * @param varcharCols
    +   * @return
    +   */
    +  private def validateLongStringColumns(fields: Seq[Field],
    +      varcharCols: Seq[String]): Unit = {
    +    var longStringColumnsMap: Map[String, Field] = Map[String, Field]()
    +    fields.foreach(field =>
    +      longStringColumnsMap.put(field.column.toUpperCase, field)
    +    )
    +    var dataTypeErr: Set[String] = Set[String]()
    +    var duplicateColumnErr: Map[String, Int] = Map[String, Int]()
    +    var nullColumnErr: Set[String] = Set[String]()
    +    var tmpStr: String = ""
    +    varcharCols.foreach {
    +      column =>
    +        tmpStr = column.toUpperCase
    +        duplicateColumnErr.get(tmpStr) match {
    +          case None => duplicateColumnErr.put(tmpStr, 1)
    +          case Some(count) => duplicateColumnErr.put(tmpStr, count + 1)
    +        }
    +        longStringColumnsMap.get(tmpStr) match {
    +          case None => nullColumnErr += column
    +          case Some(field) => if (!DataTypes.STRING.getName.equalsIgnoreCase(field.dataType.get)) {
    +            dataTypeErr += column
    +          }
    +        }
    +    }
    +    if (!nullColumnErr.isEmpty) {
    +      val errMsg = "long_string_columns:" +
    +                   nullColumnErr.mkString(",") +
    +                   " does not exist in table. Please check create table statement."
    +      throw new MalformedCarbonCommandException(errMsg)
    +    }
    +
    +    var duplicateColumns = duplicateColumnErr.filter(kv => kv._2 != 1).keySet
    --- End diff --
   
    to check if exists duplicate  columns in long_string_columns property


---

[qiuchenjian-2]

Github user Sssan520 commented on the issue:

    https://github.com/apache/carbondata/pull/2436
 
    all code view comments has been handled.


---

[qiuchenjian-2]

Github user Sssan520 commented on the issue:

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


---

[qiuchenjian-2]

Github user Sssan520 commented on the issue:

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



---

[qiuchenjian-2]

Github user Sssan520 commented on the issue:

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


---

[qiuchenjian-2]

Github user Sssan520 commented on the issue:

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


---

[qiuchenjian-2]

Github user Sssan520 closed the pull request at:

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


---