[GitHub] carbondata pull request #1374: [CARBONDATA-1491] Dictionary_exclude columns ...

[qiuchenjian-2]

GitHub user dhatchayani opened a pull request:

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

    [CARBONDATA-1491] Dictionary_exclude columns are not going into no_dictionary flow

    (1) **DICTIONARY_EXCLUDE** columns are not considered as no_dictionary columns - while parsing we are not setting nodictionary columns.
    (2) For reconstructing defaultValue for newly added no dictionary measure column, logic is changed, as the previous logic can throw IllegalArgumentException for wrong length.
    (3) Test cases are added for the same.

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

    $ git pull https://github.com/dhatchayani/incubator-carbondata add_dic_excl_measure

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

    https://github.com/apache/carbondata/pull/1374.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 #1374
   
----
commit 4a812e70f3f2aa023e1fbf509f287766a14d8521
Author: dhatchayani <[hidden email]>
Date:   2017-09-20T06:33:18Z

    [CARBONDATA-1491] Dictionary_exclude columns are not going into no_dictionary flow

----


---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

    https://github.com/apache/carbondata/pull/1374
 
    Build Success with Spark 1.6, Please check CI http://88.99.58.216:8080/job/ApacheCarbonPRBuilder/116/



---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

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



---

[qiuchenjian-2]

Github user ravipesala commented on the issue:

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



---

[qiuchenjian-2]

Github user dhatchayani commented on the issue:

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


---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

    https://github.com/apache/carbondata/pull/1374
 
    Build Success with Spark 1.6, Please check CI http://88.99.58.216:8080/job/ApacheCarbonPRBuilder/117/



---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

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



---

[qiuchenjian-2]

Github user ravipesala commented on the issue:

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



---

[qiuchenjian-2]

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

    https://github.com/apache/carbondata/pull/1374#discussion_r139945003
 
    --- Diff: integration/spark-common/src/main/scala/org/apache/spark/sql/catalyst/CarbonDDLSqlParser.scala ---
    @@ -617,7 +617,13 @@ abstract class CarbonDDLSqlParser extends AbstractCarbonSparkSQLParser {
         // by default consider all String cols as dims and if any dictionary include isn't present then
         // add it to noDictionaryDims list. consider all dictionary excludes/include cols as dims
         fields.foreach { field =>
    -      if (dictIncludeCols.exists(x => x.equalsIgnoreCase(field.column))) {
    +      if (dictExcludeCols.exists(x => x.equalsIgnoreCase(field.column))) {
    +        val dataType = DataTypeUtil.getDataType(field.dataType.get.toUpperCase())
    +        if (dataType != DataType.DATE) {
    --- End diff --
   
    how about timestamp?


---

[qiuchenjian-2]

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

    https://github.com/apache/carbondata/pull/1374#discussion_r139946254
 
    --- Diff: integration/spark-common/src/main/scala/org/apache/spark/sql/catalyst/CarbonDDLSqlParser.scala ---
    @@ -617,7 +617,13 @@ abstract class CarbonDDLSqlParser extends AbstractCarbonSparkSQLParser {
         // by default consider all String cols as dims and if any dictionary include isn't present then
         // add it to noDictionaryDims list. consider all dictionary excludes/include cols as dims
         fields.foreach { field =>
    -      if (dictIncludeCols.exists(x => x.equalsIgnoreCase(field.column))) {
    +      if (dictExcludeCols.exists(x => x.equalsIgnoreCase(field.column))) {
    +        val dataType = DataTypeUtil.getDataType(field.dataType.get.toUpperCase())
    +        if (dataType != DataType.DATE) {
    --- End diff --
   
    timestamp can be excluded, as now we are supporting timestamp in both include and exclude


---

[qiuchenjian-2]

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

    https://github.com/apache/carbondata/pull/1374#discussion_r139947636
 
    --- Diff: integration/spark-common/src/main/scala/org/apache/spark/sql/catalyst/CarbonDDLSqlParser.scala ---
    @@ -617,7 +617,13 @@ abstract class CarbonDDLSqlParser extends AbstractCarbonSparkSQLParser {
         // by default consider all String cols as dims and if any dictionary include isn't present then
         // add it to noDictionaryDims list. consider all dictionary excludes/include cols as dims
         fields.foreach { field =>
    -      if (dictIncludeCols.exists(x => x.equalsIgnoreCase(field.column))) {
    +      if (dictExcludeCols.exists(x => x.equalsIgnoreCase(field.column))) {
    +        val dataType = DataTypeUtil.getDataType(field.dataType.get.toUpperCase())
    +        if (dataType != DataType.DATE) {
    --- End diff --
   
    Ok, how about double,decimal and float?


---

[qiuchenjian-2]

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

    https://github.com/apache/carbondata/pull/1374#discussion_r139958907
 
    --- Diff: integration/spark-common/src/main/scala/org/apache/spark/sql/catalyst/CarbonDDLSqlParser.scala ---
    @@ -617,7 +617,13 @@ abstract class CarbonDDLSqlParser extends AbstractCarbonSparkSQLParser {
         // by default consider all String cols as dims and if any dictionary include isn't present then
         // add it to noDictionaryDims list. consider all dictionary excludes/include cols as dims
         fields.foreach { field =>
    -      if (dictIncludeCols.exists(x => x.equalsIgnoreCase(field.column))) {
    +      if (dictExcludeCols.exists(x => x.equalsIgnoreCase(field.column))) {
    +        val dataType = DataTypeUtil.getDataType(field.dataType.get.toUpperCase())
    +        if (dataType != DataType.DATE) {
    --- End diff --
   
    double decimal and float will throw exception as unsupported datatype, may be we can add date also to that list


---

[qiuchenjian-2]

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

    https://github.com/apache/carbondata/pull/1374#discussion_r139970827
 
    --- Diff: integration/spark-common/src/main/scala/org/apache/spark/sql/catalyst/CarbonDDLSqlParser.scala ---
    @@ -617,7 +617,13 @@ abstract class CarbonDDLSqlParser extends AbstractCarbonSparkSQLParser {
         // by default consider all String cols as dims and if any dictionary include isn't present then
         // add it to noDictionaryDims list. consider all dictionary excludes/include cols as dims
         fields.foreach { field =>
    -      if (dictIncludeCols.exists(x => x.equalsIgnoreCase(field.column))) {
    +      if (dictExcludeCols.exists(x => x.equalsIgnoreCase(field.column))) {
    +        val dataType = DataTypeUtil.getDataType(field.dataType.get.toUpperCase())
    +        if (dataType != DataType.DATE) {
    --- End diff --
   
    yes, please add


---

[qiuchenjian-2]

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

    https://github.com/apache/carbondata/pull/1374#discussion_r140146423
 
    --- Diff: integration/spark-common/src/main/scala/org/apache/spark/sql/catalyst/CarbonDDLSqlParser.scala ---
    @@ -617,7 +617,13 @@ abstract class CarbonDDLSqlParser extends AbstractCarbonSparkSQLParser {
         // by default consider all String cols as dims and if any dictionary include isn't present then
         // add it to noDictionaryDims list. consider all dictionary excludes/include cols as dims
         fields.foreach { field =>
    -      if (dictIncludeCols.exists(x => x.equalsIgnoreCase(field.column))) {
    +      if (dictExcludeCols.exists(x => x.equalsIgnoreCase(field.column))) {
    +        val dataType = DataTypeUtil.getDataType(field.dataType.get.toUpperCase())
    +        if (dataType != DataType.DATE) {
    --- End diff --
   
    we are already having a whitelist for dictionary_exclude that will filter out date. so this check can be removed


---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

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



---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

    https://github.com/apache/carbondata/pull/1374
 
    Build Success with Spark 1.6, Please check CI http://88.99.58.216:8080/job/ApacheCarbonPRBuilder/127/



---

[qiuchenjian-2]

Github user ravipesala commented on the issue:

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



---

[qiuchenjian-2]

Github user ravipesala commented on the issue:

    https://github.com/apache/carbondata/pull/1374
 
    LGTM


---

[qiuchenjian-2]

Github user asfgit closed the pull request at:

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


---