[CARBONDATA-2624] Added validations for complex dataType columns in create table command for Local Dictionary Support #2390

praveenmeenakshi56 · 2018-06-20T22:09:16Z

What is proposed in this PR

Added Validations for Complex DataType command in create table command.
Added Unit Test cases for the same

What changes were made

If Duplicate columns exist, the column names were not displayed in the error message
Considered to check for duplicates if extra space was the difference between column names for LOCAL_DICTIONARY_INCLUDE and LOCAL_DICTIONARY_EXCLUDE
Added validation to check if no child column is of string dataType in create table command when the complex dataType column was specified in LOCA_DICTIONARY_INCLUDE
If Duplicate columns were specified in LOCAL_DICTONARY_INCLUDE/EXCLUDE and in DICTONARY_INCLUDE, the column names were not displayed in the error message

Be sure to do all of the following checklist to help us incorporate
your contribution quickly and easily:

Any interfaces changed?
NA
Any backward compatibility impacted?
NA
Document update required?
will be updated in another PR
Testing done
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.
Unit Test cases tested and added in this PR
For large changes, please consider breaking it into sub-tasks under an umbrella JIRA.

CarbonDataQA · 2018-06-20T23:34:32Z

Build Success with Spark 2.1.0, Please check CI http://136.243.101.176:8080/job/ApacheCarbonPRBuilder1/6429/

CarbonDataQA · 2018-06-20T23:45:24Z

Build Failed with Spark 2.2.1, Please check CI http://88.99.58.216:8080/job/ApacheCarbonPRBuilder/5260/

kumarvishal09 · 2018-06-21T02:25:15Z

retest this please

CarbonDataQA · 2018-06-21T03:21:04Z

Build Success with Spark 2.1.0, Please check CI http://136.243.101.176:8080/job/ApacheCarbonPRBuilder1/6432/

CarbonDataQA · 2018-06-21T03:35:36Z

Build Failed with Spark 2.2.1, Please check CI http://88.99.58.216:8080/job/ApacheCarbonPRBuilder/5263/

ravipesala · 2018-06-21T03:51:05Z

SDV Build Success , Please check CI http://144.76.159.231:8080/job/ApacheSDVTests/5366/

ravipesala · 2018-06-21T05:23:10Z

SDV Build Success , Please check CI http://144.76.159.231:8080/job/ApacheSDVTests/5367/

akashrn5 · 2018-06-21T06:00:58Z

...pache/carbondata/spark/testsuite/localdictionary/LocalDictionarySupportCreateTableTest.scala

      sql(
        """
          | CREATE TABLE local1(id int, name string, city string, age int)
          | STORED BY 'org.apache.carbondata.format'
          | tblproperties('local_dictionary_enable'='true','local_dictionary_include'='name,city',
-          | 'local_dictionary_exclude'='name')
+          | 'local_dictionary_exclude'='city')


better to change exiting test case only for case sensitive check

akashrn5 · 2018-06-21T06:03:30Z

...pache/carbondata/spark/testsuite/localdictionary/LocalDictionarySupportCreateTableTest.scala

+      case Some(row) => assert(row.get(1).toString.contains("name,st"))
+    }
+  }
+


add one more test case, where two complex columns are there and one is only with int datatype child columns

akashrn5 · 2018-06-21T06:04:13Z

integration/spark-common/src/main/scala/org/apache/spark/sql/catalyst/CarbonDDLSqlParser.scala

+        }
+      }
+    }
+


remove empty line here and add a proper comment

akashrn5 · 2018-06-21T06:04:33Z

integration/spark-common/src/main/scala/org/apache/spark/sql/catalyst/CarbonDDLSqlParser.scala

+        .exists(x => x.column.equalsIgnoreCase(dictColm) && x.children.isDefined &&
+                     null != x.children.get &&
+                     !validateChildColumns(x))) {
+        val errMsg = "No child column is string dataType column in local_dictionary_include."


error message is not correct, give a proper error message

akashrn5 · 2018-06-21T06:13:08Z

integration/spark-common/src/main/scala/org/apache/spark/sql/catalyst/CarbonDDLSqlParser.scala

    // check if the same column is present in both dictionary include and local dictionary columns
    // configuration
    if (tableProperties.get(CarbonCommonConstants.DICTIONARY_INCLUDE).isDefined) {
+      val dictExcludeCoumns = tableProperties.get(CarbonCommonConstants.LOCAL_DICTIONARY_EXCLUDE)


remove this check, because if same column is contained in dictioanry include, local dictionary include, local dictionary exclude, any two will be validated and error will be thrown, better not to check all the three properties

CarbonDataQA · 2018-06-21T06:45:02Z

Build Failed with Spark 2.1.0, Please check CI http://136.243.101.176:8080/job/ApacheCarbonPRBuilder1/6435/

praveenmeenakshi56 · 2018-06-21T06:45:56Z

retest this please

kunal642 · 2018-06-21T06:52:55Z

integration/spark-common/src/main/scala/org/apache/spark/sql/catalyst/CarbonDDLSqlParser.scala

+   * this method validates whether any of the child columns of complex dataType column
+   * is of type string.
+   */
+  def validateChildColumns(field: Field): Boolean = {


can you change the logic to return true as soon as you find any column with string datatype. No need to check other columns.

CarbonDataQA · 2018-06-21T06:53:42Z

Build Success with Spark 2.2.1, Please check CI http://88.99.58.216:8080/job/ApacheCarbonPRBuilder/5266/

kunal642 · 2018-06-21T06:57:02Z

...pache/carbondata/spark/testsuite/localdictionary/LocalDictionarySupportCreateTableTest.scala

+        "Child columns do not exist for the specified complex dataType column(s) in local_dictionary_include."))
+  }
+
+  test(


i think local_dictionary_include _001 has already covered this scenario. Please remove all duplicate test cases.

kunal642 · 2018-06-21T07:04:45Z

integration/spark-common/src/main/scala/org/apache/spark/sql/catalyst/CarbonDDLSqlParser.scala

+        .exists(x => x.column.equalsIgnoreCase(dictColm) && x.children.isDefined &&
+                     null != x.children.get &&
+                     !validateChildColumns(x))) {
+        val errMsg = "Child columns do not exist for the specified complex dataType column(s) in local_dictionary_include."


Throw proper error message.

CarbonDataQA · 2018-06-21T07:47:33Z

Build Failed with Spark 2.1.0, Please check CI http://136.243.101.176:8080/job/ApacheCarbonPRBuilder1/6439/

CarbonDataQA · 2018-06-21T08:17:11Z

Build Failed with Spark 2.2.1, Please check CI http://88.99.58.216:8080/job/ApacheCarbonPRBuilder/5270/

CarbonDataQA · 2018-06-21T09:53:19Z

Build Success with Spark 2.1.0, Please check CI http://136.243.101.176:8080/job/ApacheCarbonPRBuilder1/6443/

ravipesala · 2018-06-21T10:02:54Z

SDV Build Success , Please check CI http://144.76.159.231:8080/job/ApacheSDVTests/5370/

CarbonDataQA · 2018-06-21T10:51:45Z

Build Failed with Spark 2.2.1, Please check CI http://88.99.58.216:8080/job/ApacheCarbonPRBuilder/5277/

…mns for Local Dictionary Support

akashrn5 · 2018-06-21T12:05:42Z

retest this please

akashrn5 · 2018-06-21T12:05:50Z

retest sdv please

CarbonDataQA · 2018-06-21T12:17:53Z

Build Success with Spark 2.1.0, Please check CI http://136.243.101.176:8080/job/ApacheCarbonPRBuilder1/6446/

ravipesala · 2018-06-21T12:23:13Z

retest this please

akashrn5 · 2018-06-21T12:57:15Z

LGTM

CarbonDataQA · 2018-06-21T14:19:58Z

Build Success with Spark 2.1.0, Please check CI http://136.243.101.176:8080/job/ApacheCarbonPRBuilder1/6449/

ravipesala · 2018-06-21T14:37:32Z

SDV Build Fail , Please check CI http://144.76.159.231:8080/job/ApacheSDVTests/5373/

praveenmeenakshi56 · 2018-06-21T14:39:46Z

retest sdv please

CarbonDataQA · 2018-06-21T14:47:13Z

Build Success with Spark 2.2.1, Please check CI http://88.99.58.216:8080/job/ApacheCarbonPRBuilder/5280/

ravipesala · 2018-06-21T17:59:52Z

SDV Build Success , Please check CI http://144.76.159.231:8080/job/ApacheSDVTests/5376/

kunal642 · 2018-06-21T19:04:17Z

LGTM

…reate table command for Local Dictionary Support 1. If Duplicate columns exist, the column names were not displayed in the error message 2. Considered to check for duplicates if extra space was the difference between column names for LOCAL_DICTIONARY_INCLUDE and LOCAL_DICTIONARY_EXCLUDE 3. Added validation to check if no child column is of string dataType in create table command when the complex dataType column was specified in LOCA_DICTIONARY_INCLUDE 4. If Duplicate columns were specified in LOCAL_DICTONARY_INCLUDE/EXCLUDE and in DICTONARY_INCLUDE, the column names were not displayed in the error message This closes #2390

praveenmeenakshi56 force-pushed the local_dict1 branch from ee3eeaf to 91f7e16 Compare June 20, 2018 22:12

praveenmeenakshi56 force-pushed the local_dict1 branch from 91f7e16 to 2025e76 Compare June 21, 2018 05:45

akashrn5 reviewed Jun 21, 2018

View reviewed changes

praveenmeenakshi56 force-pushed the local_dict1 branch from 2025e76 to 063c230 Compare June 21, 2018 06:38

kunal642 reviewed Jun 21, 2018

View reviewed changes

praveenmeenakshi56 force-pushed the local_dict1 branch 2 times, most recently from 72e3731 to 5ad6e87 Compare June 21, 2018 08:38

Added validations for create table command with complex dataType colu…

ffd308b

…mns for Local Dictionary Support

praveenmeenakshi56 force-pushed the local_dict1 branch from 5ad6e87 to ffd308b Compare June 21, 2018 11:43

asfgit closed this in d5e86db Jun 22, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[CARBONDATA-2624] Added validations for complex dataType columns in create table command for Local Dictionary Support #2390

[CARBONDATA-2624] Added validations for complex dataType columns in create table command for Local Dictionary Support #2390

praveenmeenakshi56 commented Jun 20, 2018 •

edited

CarbonDataQA commented Jun 20, 2018

CarbonDataQA commented Jun 20, 2018

kumarvishal09 commented Jun 21, 2018

CarbonDataQA commented Jun 21, 2018

CarbonDataQA commented Jun 21, 2018

ravipesala commented Jun 21, 2018

ravipesala commented Jun 21, 2018

akashrn5 Jun 21, 2018

akashrn5 Jun 21, 2018

akashrn5 Jun 21, 2018

akashrn5 Jun 21, 2018

akashrn5 Jun 21, 2018

CarbonDataQA commented Jun 21, 2018

praveenmeenakshi56 commented Jun 21, 2018

kunal642 Jun 21, 2018

CarbonDataQA commented Jun 21, 2018

kunal642 Jun 21, 2018

kunal642 Jun 21, 2018

CarbonDataQA commented Jun 21, 2018

CarbonDataQA commented Jun 21, 2018

CarbonDataQA commented Jun 21, 2018

ravipesala commented Jun 21, 2018

CarbonDataQA commented Jun 21, 2018

akashrn5 commented Jun 21, 2018

akashrn5 commented Jun 21, 2018

CarbonDataQA commented Jun 21, 2018

ravipesala commented Jun 21, 2018

akashrn5 commented Jun 21, 2018

CarbonDataQA commented Jun 21, 2018

ravipesala commented Jun 21, 2018

praveenmeenakshi56 commented Jun 21, 2018

CarbonDataQA commented Jun 21, 2018

ravipesala commented Jun 21, 2018

kunal642 commented Jun 21, 2018

[CARBONDATA-2624] Added validations for complex dataType columns in create table command for Local Dictionary Support #2390

[CARBONDATA-2624] Added validations for complex dataType columns in create table command for Local Dictionary Support #2390

Conversation

praveenmeenakshi56 commented Jun 20, 2018 • edited

What is proposed in this PR

What changes were made

CarbonDataQA commented Jun 20, 2018

CarbonDataQA commented Jun 20, 2018

kumarvishal09 commented Jun 21, 2018

CarbonDataQA commented Jun 21, 2018

CarbonDataQA commented Jun 21, 2018

ravipesala commented Jun 21, 2018

ravipesala commented Jun 21, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CarbonDataQA commented Jun 21, 2018

praveenmeenakshi56 commented Jun 21, 2018

Choose a reason for hiding this comment

CarbonDataQA commented Jun 21, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CarbonDataQA commented Jun 21, 2018

CarbonDataQA commented Jun 21, 2018

CarbonDataQA commented Jun 21, 2018

ravipesala commented Jun 21, 2018

CarbonDataQA commented Jun 21, 2018

akashrn5 commented Jun 21, 2018

akashrn5 commented Jun 21, 2018

CarbonDataQA commented Jun 21, 2018

ravipesala commented Jun 21, 2018

akashrn5 commented Jun 21, 2018

CarbonDataQA commented Jun 21, 2018

ravipesala commented Jun 21, 2018

praveenmeenakshi56 commented Jun 21, 2018

CarbonDataQA commented Jun 21, 2018

ravipesala commented Jun 21, 2018

kunal642 commented Jun 21, 2018

praveenmeenakshi56 commented Jun 20, 2018 •

edited