[GitHub] carbondata pull request #2491: [CARBONDATA-2700][BloomDataMap] Block droppin...

[qiuchenjian-2]

GitHub user Sssan520 opened a pull request:

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

    [CARBONDATA-2700][BloomDataMap] Block dropping index columns for bloomfilter index datamap

    1.Block create bloomfilter datamap index on column which its datatype is complex type;
    2.Block create bloomfilter datamap index on local_dictionary column;
    3.Block change datatype for bloomfilter index datamap;
    4.Block dropping index columns for bloomfilter index datamap
   
    Be sure to do all of the following checklist to help us incorporate
    your contribution quickly and easily:
   
     - [ ] Any interfaces changed?
     add a parameter "targets" for method "canAllow" of "CarbonTable" class
   
     - [ ] Any backward compatibility impacted?  No
     
     - [ ] Document update required? No
   
     - [ ] Testing done NA
            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.
           
     - [ ] 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/Sssan520/carbondata bloomfilter

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

    https://github.com/apache/carbondata/pull/2491.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 #2491
   
----
commit 3d6597f4c51faefa191a1c068fa1b1c8be4d9f67
Author: lianganping 00251374 <lianganping@...>
Date:   2018-07-11T12:17:09Z

    1.Block create bloomfilter datamap index on column which its datatype is complex;2.Block create bloomfilter datamap index on local_dictionary column;3.Block change datatype for bloomfilter index datamap;4.Block dropping index columns for bloomfilter index datamap

----


---

[qiuchenjian-2]

Github user ravipesala commented on the issue:

    https://github.com/apache/carbondata/pull/2491
 
    Can one of the admins verify this patch?


---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

    https://github.com/apache/carbondata/pull/2491
 
    Can one of the admins verify this patch?


---

[qiuchenjian-2]

Github user xuchuanyin commented on the issue:

    https://github.com/apache/carbondata/pull/2491
 
    better to change the title of the PR


---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

    https://github.com/apache/carbondata/pull/2491
 
    Can one of the admins verify this patch?


---

[qiuchenjian-2]

Github user Sssan520 commented on the issue:

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


---

[qiuchenjian-2]

Github user Sssan520 commented on the issue:

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


---

[qiuchenjian-2]

Github user Sssan520 closed the pull request at:

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


---

[qiuchenjian-2]

GitHub user Sssan520 reopened a pull request:

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

    [CARBONDATA-2700][CARBONDATA-2730][CARBONDATA-2732][BloomDataMap] block some oprerations of bloomfilter datamap

    1.Block create bloomfilter datamap index on column which its datatype is complex type;
    2.Block create bloomfilter datamap index on local_dictionary column;
    3.Block change datatype for bloomfilter index datamap;
    4.Block dropping index columns for bloomfilter index datamap
   
    Be sure to do all of the following checklist to help us incorporate
    your contribution quickly and easily:
   
     - [ ] Any interfaces changed?
     add a parameter "targets" for method "canAllow" of "CarbonTable" class
   
     - [ ] Any backward compatibility impacted?  No
     
     - [ ] Document update required? No
   
     - [ ] Testing done NA
            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.
           
     - [ ] 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/Sssan520/carbondata bloomfilter

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

    https://github.com/apache/carbondata/pull/2491.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 #2491
   
----
commit 3d6597f4c51faefa191a1c068fa1b1c8be4d9f67
Author: lianganping 00251374 <lianganping@...>
Date:   2018-07-11T12:17:09Z

    1.Block create bloomfilter datamap index on column which its datatype is complex;2.Block create bloomfilter datamap index on local_dictionary column;3.Block change datatype for bloomfilter index datamap;4.Block dropping index columns for bloomfilter index datamap

----


---

[qiuchenjian-2]

Github user Sssan520 closed the pull request at:

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


---

[qiuchenjian-2]

GitHub user Sssan520 reopened a pull request:

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

    [CARBONDATA-2698][CARBONDATA-2700][CARBONDATA-2730][CARBONDATA-2732][BloomDataMap] block some oprerations of bloomfilter datamap

    1.Block create bloomfilter datamap index on column which its datatype is complex type;
    2.Block create bloomfilter datamap index on local_dictionary column;
    3.Block change datatype for bloomfilter index datamap;
    4.Block dropping index columns for bloomfilter index datamap
   
    Be sure to do all of the following checklist to help us incorporate
    your contribution quickly and easily:
   
     - [ ] Any interfaces changed?
     add a parameter "targets" for method "canAllow" of "CarbonTable" class
   
     - [ ] Any backward compatibility impacted?  No
     
     - [ ] Document update required? No
   
     - [ ] Testing done NA
            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.
           
     - [ ] 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/Sssan520/carbondata bloomfilter

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

    https://github.com/apache/carbondata/pull/2491.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 #2491
   
----
commit 3d6597f4c51faefa191a1c068fa1b1c8be4d9f67
Author: lianganping 00251374 <lianganping@...>
Date:   2018-07-11T12:17:09Z

    1.Block create bloomfilter datamap index on column which its datatype is complex;2.Block create bloomfilter datamap index on local_dictionary column;3.Block change datatype for bloomfilter index datamap;4.Block dropping index columns for bloomfilter index datamap

----


---

[qiuchenjian-2]

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

    https://github.com/apache/carbondata/pull/2491#discussion_r201977605
 
    --- Diff: datamap/bloom/src/main/java/org/apache/carbondata/datamap/bloom/BloomCoarseGrainDataMapFactory.java ---
    @@ -380,6 +380,49 @@ public void deleteDatamapData() {
         return false;
       }
     
    +  @Override public boolean isOperationBlock(TableOperation operation, Object... targets) {
    --- End diff --
   
    1. move 'override' to the previous line


---

[qiuchenjian-2]

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

    https://github.com/apache/carbondata/pull/2491#discussion_r201977727
 
    --- Diff: datamap/bloom/src/main/java/org/apache/carbondata/datamap/bloom/BloomCoarseGrainDataMapFactory.java ---
    @@ -380,6 +380,49 @@ public void deleteDatamapData() {
         return false;
       }
     
    +  @Override public boolean isOperationBlock(TableOperation operation, Object... targets) {
    --- End diff --
   
    2. better to optimize the name of method to 'isOperationBlocked'


---

[qiuchenjian-2]

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

    https://github.com/apache/carbondata/pull/2491#discussion_r201997642
 
    --- Diff: core/src/main/java/org/apache/carbondata/core/metadata/schema/table/CarbonTable.java ---
    @@ -1071,6 +1072,10 @@ public boolean canAllow(CarbonTable carbonTable, TableOperation operation) {
               if (factoryClass.willBecomeStale(operation)) {
                 return false;
               }
    +          //to check something like columns by the operation
    --- End diff --
   
    space after "//"
   
    optimize the comment to "check whether the operation is blocked for datamap"


---

[qiuchenjian-2]

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

    https://github.com/apache/carbondata/pull/2491#discussion_r201978444
 
    --- Diff: core/src/main/java/org/apache/carbondata/core/datamap/dev/DataMapFactory.java ---
    @@ -144,4 +144,17 @@ public void validate() throws MalformedDataMapCommandException {
         }
       }
     
    +  /**
    +   * to check something like columns by the operation
    --- End diff --
   
    better to optimize the description to:
   
    whether to block operation on corresponding table or column. For example, bloomfilter datamap will block changing datatype for bloomindex column. By default it will not block any operation.


---

[qiuchenjian-2]

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

    https://github.com/apache/carbondata/pull/2491#discussion_r201997452
 
    --- Diff: core/src/main/java/org/apache/carbondata/core/metadata/schema/table/CarbonTable.java ---
    @@ -1058,9 +1058,10 @@ public void setTransactionalTable(boolean transactionalTable) {
        * if this operation makes datamap stale it is not allowed
        * @param carbonTable
        * @param operation
    +   * @param targets
    --- End diff --
   
    add description for it


---

[qiuchenjian-2]

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

    https://github.com/apache/carbondata/pull/2491#discussion_r201998117
 
    --- Diff: datamap/bloom/src/main/java/org/apache/carbondata/datamap/bloom/BloomCoarseGrainDataMapFactory.java ---
    @@ -380,6 +380,49 @@ public void deleteDatamapData() {
         return false;
       }
     
    +  @Override public boolean isOperationBlock(TableOperation operation, Object... targets) {
    +    switch (operation) {
    +      case ALTER_DROP: {
    +        try {
    +          //alter table drop columns
    --- End diff --
   
    add space after "//"


---

[qiuchenjian-2]

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

    https://github.com/apache/carbondata/pull/2491#discussion_r201998306
 
    --- Diff: datamap/bloom/src/main/java/org/apache/carbondata/datamap/bloom/BloomCoarseGrainDataMapFactory.java ---
    @@ -380,6 +380,49 @@ public void deleteDatamapData() {
         return false;
       }
     
    +  @Override public boolean isOperationBlock(TableOperation operation, Object... targets) {
    +    switch (operation) {
    +      case ALTER_DROP: {
    +        try {
    +          //alter table drop columns
    +          //will be blocked if the columns in bloomfilter datamap
    --- End diff --
   
    This comment is not needed.
   
    The same with below switch branch


---

[qiuchenjian-2]

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

    https://github.com/apache/carbondata/pull/2491#discussion_r201999539
 
    --- Diff: datamap/bloom/src/main/java/org/apache/carbondata/datamap/bloom/BloomCoarseGrainDataMapFactory.java ---
    @@ -380,6 +380,49 @@ public void deleteDatamapData() {
         return false;
       }
     
    +  @Override public boolean isOperationBlock(TableOperation operation, Object... targets) {
    +    switch (operation) {
    +      case ALTER_DROP: {
    +        try {
    +          //alter table drop columns
    +          //will be blocked if the columns in bloomfilter datamap
    +          List<String> columnsToDrop = (List<String>) targets[0];
    +          List<String> indexedColumnNames = dataMapMeta.getIndexedColumnNames();
    +          for (String indexedcolumn : indexedColumnNames) {
    +            for (String column : columnsToDrop) {
    +              if (column.equalsIgnoreCase(indexedcolumn)) {
    +                return true;
    +              }
    +            }
    +          }
    +          return false;
    +        } catch (ClassCastException | NullPointerException ex) {
    --- End diff --
   
    better to ignore these exceptions and let it throw out if it really happens.
   
    The same with the below switch branch


---

[qiuchenjian-2]

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

    https://github.com/apache/carbondata/pull/2491#discussion_r202000444
 
    --- Diff: integration/spark2/src/main/scala/org/apache/spark/sql/execution/command/schema/CarbonAlterTableDataTypeChangeCommand.scala ---
    @@ -55,7 +55,10 @@ private[sql] case class CarbonAlterTableDataTypeChangeCommand(
             .validateTableAndAcquireLock(dbName, tableName, locksToBeAcquired)(sparkSession)
           val metastore = CarbonEnv.getInstance(sparkSession).carbonMetastore
           carbonTable = CarbonEnv.getCarbonTable(Some(dbName), tableName)(sparkSession)
    -      if (!carbonTable.canAllow(carbonTable, TableOperation.ALTER_CHANGE_DATATYPE)) {
    +      if (!carbonTable
    --- End diff --
   
    please optimize the indent


---