[GitHub] carbondata pull request #1489: [CARBONDATA-1576] Support DataMap drop

[qiuchenjian-2]

GitHub user ravipesala opened a pull request:

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

    [CARBONDATA-1576] Support DataMap drop

    Be sure to do all of the following checklist to help us incorporate
    your contribution quickly and easily:
   
     Added drop datamap sql parser to support dropping datamap from table.
     ```
    DROP DATAMAP IF EXISTS datamapname ON TABLE tablename
     ```
    Added restriction on dropping child table if user tries to drop the datamap table directly.
   
     - [X] Any interfaces changed?
       NO
     
     - [X] Any backward compatibility impacted?
        NO
     
     - [X] Document update required?
       Yes, syntax needs to be added to document
   
     - [X] Testing done
           Added  tests
           
     - [X] 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/ravipesala/incubator-carbondata datamap-drop

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

    https://github.com/apache/carbondata/pull/1489.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 #1489
   
----
commit a4d6f6d827bdc4c3cd2fe877b69eb703f3801cc9
Author: kumarvishal <[hidden email]>
Date:   2017-10-15T12:35:55Z

    [CARBONDATA-1517]- Pre Aggregate Create Table Support
   
    Support CTAS in carbon and support creating aggregation tables using CTAS and update aggregation table information to main table schema.
   
    This closes #1433

commit b26e1273d2cacb16d5ad26cb71f279e6c3914a9a
Author: ravipesala <[hidden email]>
Date:   2017-11-10T10:48:17Z

    Added create datamap parser and saved to schema file

commit 7b82bf9787b9becc697a826105675afd31230438
Author: ravipesala <[hidden email]>
Date:   2017-11-12T06:57:47Z

    Added parser for drop datamap and handled events

----


---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

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



---

[qiuchenjian-2]

Github user ravipesala commented on the issue:

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



---

[qiuchenjian-2]

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

    https://github.com/apache/carbondata/pull/1489#discussion_r150408117
 
    --- Diff: core/src/main/java/org/apache/carbondata/core/datamap/DataMapStoreManager.java ---
    @@ -144,23 +144,40 @@ private TableDataMap getTableDataMap(String dataMapName,
        * Clear the datamap/datamaps of a table from memory
        * @param identifier Table identifier
        */
    -  public void clearDataMap(AbsoluteTableIdentifier identifier) {
    +  public void clearDataMaps(AbsoluteTableIdentifier identifier) {
         List<TableDataMap> tableDataMaps = allDataMaps.get(identifier.uniqueName());
         segmentRefreshMap.remove(identifier.uniqueName());
         if (tableDataMaps != null) {
    -      int i = 0;
           for (TableDataMap tableDataMap: tableDataMaps) {
             if (tableDataMap != null) {
               tableDataMap.clear();
               break;
             }
    -        i++;
           }
           allDataMaps.remove(identifier.uniqueName());
         }
       }
     
       /**
    +   * Clear the datamap/datamaps of a table from memory
    +   * @param identifier Table identifier
    +   */
    +  public void clearDataMap(AbsoluteTableIdentifier identifier, String dataMapName) {
    --- End diff --
   
    contains bug in removing element


---

[qiuchenjian-2]

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

    https://github.com/apache/carbondata/pull/1489#discussion_r150408185
 
    --- Diff: format/src/main/thrift/schema.thrift ---
    @@ -187,15 +187,17 @@ struct ParentColumnTableRelation {
     }
     
     struct DataMapSchema  {
    +    // DataMap name
    +    1: required string dataMapName;
    --- End diff --
   
    will the change in sequence affect the previous version?


---

[qiuchenjian-2]

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

    https://github.com/apache/carbondata/pull/1489#discussion_r150411968
 
    --- Diff: format/src/main/thrift/schema.thrift ---
    @@ -187,15 +187,17 @@ struct ParentColumnTableRelation {
     }
     
     struct DataMapSchema  {
    +    // DataMap name
    +    1: required string dataMapName;
    --- End diff --
   
    NO, we are adding it in this version only. This `DataMapSchema` does not exist in old version


---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

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



---

[qiuchenjian-2]

Github user ravipesala commented on the issue:

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



---

[qiuchenjian-2]

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

    https://github.com/apache/carbondata/pull/1489#discussion_r150414426
 
    --- Diff: core/src/main/java/org/apache/carbondata/core/datamap/DataMapStoreManager.java ---
    @@ -144,23 +144,40 @@ private TableDataMap getTableDataMap(String dataMapName,
        * Clear the datamap/datamaps of a table from memory
        * @param identifier Table identifier
        */
    -  public void clearDataMap(AbsoluteTableIdentifier identifier) {
    +  public void clearDataMaps(AbsoluteTableIdentifier identifier) {
         List<TableDataMap> tableDataMaps = allDataMaps.get(identifier.uniqueName());
         segmentRefreshMap.remove(identifier.uniqueName());
         if (tableDataMaps != null) {
    -      int i = 0;
           for (TableDataMap tableDataMap: tableDataMaps) {
             if (tableDataMap != null) {
               tableDataMap.clear();
               break;
             }
    -        i++;
           }
           allDataMaps.remove(identifier.uniqueName());
         }
       }
     
       /**
    +   * Clear the datamap/datamaps of a table from memory
    +   * @param identifier Table identifier
    +   */
    +  public void clearDataMap(AbsoluteTableIdentifier identifier, String dataMapName) {
    --- End diff --
   
    what bug?


---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

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



---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

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



---

[qiuchenjian-2]

Github user ravipesala commented on the issue:

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


---

[qiuchenjian-2]

Github user ravipesala closed the pull request at:

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


---

[qiuchenjian-2]

GitHub user ravipesala reopened a pull request:

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

    [CARBONDATA-1576][PREAGG][DATAMAP] Support DataMap drop

    Be sure to do all of the following checklist to help us incorporate
    your contribution quickly and easily:
   
     Added drop datamap sql parser to support dropping datamap from table.
     ```
    DROP DATAMAP IF EXISTS datamapname ON TABLE tablename
     ```
    Added restriction on dropping child table if user tries to drop the datamap table directly.
   
    This PR is dependent on PR https://github.com/apache/carbondata/pull/1481
   
     - [X] Any interfaces changed?
       NO
     
     - [X] Any backward compatibility impacted?
        NO
     
     - [X] Document update required?
       Yes, syntax needs to be added to document
   
     - [X] Testing done
           Added  tests
           
     - [X] 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/ravipesala/incubator-carbondata datamap-drop

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

    https://github.com/apache/carbondata/pull/1489.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 #1489
   
----
commit cac54d9a1578796cf1d90abacab069a0413a49a0
Author: ravipesala <[hidden email]>
Date:   2017-11-12T11:38:09Z

    Added create datamap parser and saved to schema file

commit 9562faab3273c350336653f7eb0e27536377207b
Author: ravipesala <[hidden email]>
Date:   2017-11-12T06:57:47Z

    Added parser for drop datamap and handled events

----


---

[qiuchenjian-2]

Github user ravipesala commented on the issue:

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



---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

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



---

[qiuchenjian-2]

Github user ravipesala commented on the issue:

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



---

[qiuchenjian-2]

Github user ravipesala commented on the issue:

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



---

[qiuchenjian-2]

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

    https://github.com/apache/carbondata/pull/1489#discussion_r150433577
 
    --- Diff: core/src/main/java/org/apache/carbondata/core/datamap/DataMapStoreManager.java ---
    @@ -144,23 +144,40 @@ private TableDataMap getTableDataMap(String dataMapName,
        * Clear the datamap/datamaps of a table from memory
        * @param identifier Table identifier
        */
    -  public void clearDataMap(AbsoluteTableIdentifier identifier) {
    +  public void clearDataMaps(AbsoluteTableIdentifier identifier) {
         List<TableDataMap> tableDataMaps = allDataMaps.get(identifier.uniqueName());
         segmentRefreshMap.remove(identifier.uniqueName());
         if (tableDataMaps != null) {
    -      int i = 0;
           for (TableDataMap tableDataMap: tableDataMaps) {
             if (tableDataMap != null) {
               tableDataMap.clear();
               break;
             }
    -        i++;
           }
           allDataMaps.remove(identifier.uniqueName());
         }
       }
     
       /**
    +   * Clear the datamap/datamaps of a table from memory
    +   * @param identifier Table identifier
    +   */
    +  public void clearDataMap(AbsoluteTableIdentifier identifier, String dataMapName) {
    --- End diff --
   
    sorry, didn't notice the `break` before. That's fine~


---

[qiuchenjian-2]

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

    https://github.com/apache/carbondata/pull/1489#discussion_r150503820
 
    --- Diff: integration/spark-common-test/src/test/scala/org/apache/carbondata/integration/spark/testsuite/preaggregate/TestPreAggregateDrop.scala ---
    @@ -42,14 +42,14 @@ class TestPreAggregateDrop extends QueryTest with BeforeAndAfterAll {
           "create datamap preagg1 on table maintable using 'preaggregate' as select" +
           " a,sum(b) from maintable group by a")
         sql(
    -      "create datamap preagg2 on table maintable using 'preaggregate'  as select" +
    +      "create datamap preagg2 on table maintable using 'preaggregate' as select" +
           " a,sum(c) from maintable group by a")
    -    sql("drop table if exists preagg2")
    +    sql("drop datamap if exists preagg2 on table maintable")
         val showTables = sql("show tables")
         checkExistence(showTables, false, "preagg2")
         checkExistence(showTables, true, "preagg1")
       }
    -  
    +
       test("drop main table and check if preaggreagte is deleted") {
    --- End diff --
   
    please add a testcase to drop datamap which is not exist


---