[GitHub] carbondata pull request #1508: [CARBONDATA-1738] Block direct insert/load on...

[qiuchenjian-2]

GitHub user kunal642 opened a pull request:

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

    [CARBONDATA-1738] Block direct insert/load on pre-aggregate table

    Block load/insert on pre-aggregate table
   
     - [X] Any interfaces changed?
     No
     - [X] Any backward compatibility impacted?
     No
     - [X] Document update required?
    No
     - [X] Testing done
           Added test case
     - [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/kunal642/carbondata block_load_on_preagg

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

    https://github.com/apache/carbondata/pull/1508.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 #1508
   
----
commit 9d48d35a93116ac53951ad6b9a3dc6e110075d36
Author: kunal642 <[hidden email]>
Date:   2017-11-16T12:28:50Z

    blocked direct insert/load on pre-aggregate table

----


---

[qiuchenjian-2]

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

    https://github.com/apache/carbondata/pull/1508#discussion_r151407441
 
    --- Diff: integration/spark2/src/main/scala/org/apache/spark/sql/CarbonSession.scala ---
    @@ -212,6 +212,18 @@ object CarbonSession {
         ThreadLocalSessionInfo.setCarbonSessionInfo(currentThreadSessionInfo)
       }
     
    +  def threadUnset(key: String): Unit = {
    +    var currentThreadSessionInfo = ThreadLocalSessionInfo.getCarbonSessionInfo
    +    if (currentThreadSessionInfo == null) {
    --- End diff --
   
    If null then not required to unset


---

[qiuchenjian-2]

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

    https://github.com/apache/carbondata/pull/1508#discussion_r151408491
 
    --- Diff: integration/spark2/src/main/scala/org/apache/spark/sql/execution/command/preaaggregate/PreAggregateListeners.scala ---
    @@ -51,11 +51,39 @@ object LoadPostAggregateListener extends OperationEventListener {
             val childDatabaseName = dataMapSchema.getRelationIdentifier.getDatabaseName
             val selectQuery = dataMapSchema.getProperties.get("CHILD_SELECT QUERY")
             sparkSession.sql(s"insert into $childDatabaseName.$childTableName $selectQuery")
    +        CarbonSession.threadUnset(CarbonCommonConstants.CARBON_INPUT_SEGMENTS +
    --- End diff --
   
    add this unset to finally


---

[qiuchenjian-2]

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

    https://github.com/apache/carbondata/pull/1508#discussion_r151410067
 
    --- Diff: integration/spark2/src/main/scala/org/apache/spark/sql/execution/command/preaaggregate/PreAggregateListeners.scala ---
    @@ -51,11 +51,39 @@ object LoadPostAggregateListener extends OperationEventListener {
             val childDatabaseName = dataMapSchema.getRelationIdentifier.getDatabaseName
             val selectQuery = dataMapSchema.getProperties.get("CHILD_SELECT QUERY")
             sparkSession.sql(s"insert into $childDatabaseName.$childTableName $selectQuery")
    +        CarbonSession.threadUnset(CarbonCommonConstants.CARBON_INPUT_SEGMENTS +
    +                                  carbonLoadModel.getDatabaseName + "." +
    +                                  carbonLoadModel.getTableName)
    +        CarbonSession.threadUnset(CarbonCommonConstants.VALIDATE_CARBON_INPUT_SEGMENTS +
    +                                  carbonLoadModel.getDatabaseName + "." +
    +                                  carbonLoadModel.getTableName)
           }
         }
       }
     }
     
    +object LoadPreAggregateTablePreListener extends OperationEventListener {
    +  /**
    +   * Called on a specified event occurrence
    +   *
    +   * @param event
    +   * @param operationContext
    +   */
    +  override def onEvent(event: Event, operationContext: OperationContext): Unit = {
    +    val loadEvent = event.asInstanceOf[LoadTablePreExecutionEvent]
    +    val carbonLoadModel = loadEvent.carbonLoadModel
    +    val table = carbonLoadModel.getCarbonDataLoadSchema.getCarbonTable
    +    val carbonSessionInfo = ThreadLocalSessionInfo.getCarbonSessionInfo
    +    val isInternalLoadCall = carbonSessionInfo.getSessionParams.getAll.keySet().asScala
    --- End diff --
   
    Use specifc parameter instead of using this VALIDATE_CARBON_INPUT_SEGMENTS


---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

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



---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

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



---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

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



---

[qiuchenjian-2]

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

    https://github.com/apache/carbondata/pull/1508#discussion_r151637632
 
    --- Diff: core/src/main/java/org/apache/carbondata/core/constants/CarbonCommonConstants.java ---
    @@ -67,6 +67,12 @@
       public static final String VALIDATE_CARBON_INPUT_SEGMENTS = "validate.carbon.input.segments.";
     
       /**
    +   * Whether load/insert command is fired internally or by the user.
    +   * Used to block load/insert on pre-aggregate if fired by user
    +   */
    +  public static final String IS_INTERNAL_LOAD_CALL = "is.internal.load.call";
    --- End diff --
   
    Seems no testcase for this option. And the option name should start with `carbon`


---

[qiuchenjian-2]

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

    https://github.com/apache/carbondata/pull/1508#discussion_r151638473
 
    --- Diff: core/src/main/java/org/apache/carbondata/core/constants/CarbonCommonConstants.java ---
    @@ -67,6 +67,12 @@
       public static final String VALIDATE_CARBON_INPUT_SEGMENTS = "validate.carbon.input.segments.";
     
       /**
    +   * Whether load/insert command is fired internally or by the user.
    +   * Used to block load/insert on pre-aggregate if fired by user
    +   */
    +  public static final String IS_INTERNAL_LOAD_CALL = "is.internal.load.call";
    --- End diff --
   
    This option/property will not be exposed to the user. It will be set by the post load listener to know whether the load is fired by the user or is it an internal call.
   
    Test case is added in TestPreAggregateLoad


---

[qiuchenjian-2]

Github user kunal642 commented on the issue:

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


---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

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



---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

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



---

[qiuchenjian-2]

Github user kunal642 commented on the issue:

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


---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

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



---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

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



---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

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



---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

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



---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

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



---

[qiuchenjian-2]

Github user ravipesala commented on the issue:

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



---

[qiuchenjian-2]

Github user kunal642 commented on the issue:

    https://github.com/apache/carbondata/pull/1508
 
    @ravipesala Please review


---