[GitHub] carbondata pull request #2852: [WIP]Column Schema objects are present in Dri...

[qiuchenjian-2]

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

    https://github.com/apache/carbondata/pull/2852#discussion_r228845893
 
    --- Diff: integration/spark2/src/main/scala/org/apache/spark/sql/hive/CarbonFileMetastore.scala ---
    @@ -510,6 +509,7 @@ class CarbonFileMetastore extends CarbonMetaStore {
             SegmentPropertiesAndSchemaHolder.getInstance().invalidate(absoluteTableIdentifier)
           }
         }
    +    removeTableFromMetadata(dbName, tableName)
    --- End diff --
   
    Moving outside can be a problem as according to above logic inside else block there is again an if check based on which this method is getting called. With current check that if check will not be used for decision making


---

[qiuchenjian-2]

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

    https://github.com/apache/carbondata/pull/2852#discussion_r228849744
 
    --- Diff: processing/src/main/java/org/apache/carbondata/processing/loading/sort/Sorter.java ---
    @@ -31,9 +32,10 @@
       /**
        * Initialize sorter with sort parameters.
        *
    +   * @param carbonTable
        * @param sortParameters
        */
    -  void initialize(SortParameters sortParameters);
    +  void initialize(CarbonTable carbonTable, SortParameters sortParameters);
    --- End diff --
   
    Do not modify the interface method to add CarbonTable. Instead hold the carbonTable object in SortParameters from which all classes extending Sorter interface can use carbonTable object


---

[qiuchenjian-2]

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

    https://github.com/apache/carbondata/pull/2852#discussion_r228850717
 
    --- Diff: processing/src/main/java/org/apache/carbondata/processing/loading/steps/DataWriterBatchProcessorStepImpl.java ---
    @@ -84,7 +84,7 @@ public DataWriterBatchProcessorStepImpl(CarbonDataLoadConfiguration configuratio
               .recordDictionaryValue2MdkAdd2FileTime(CarbonTablePath.DEPRECATED_PATITION_ID,
                   System.currentTimeMillis());
           int i = 0;
    -      String[] storeLocation = getStoreLocation(tableIdentifier);
    +      String[] storeLocation = getStoreLocation(configuration.getTableSpec().getCarbonTable());
    --- End diff --
   
    same comment as above


---

[qiuchenjian-2]

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

    https://github.com/apache/carbondata/pull/2852#discussion_r228850573
 
    --- Diff: processing/src/main/java/org/apache/carbondata/processing/loading/steps/CarbonRowDataWriterProcessorStepImpl.java ---
    @@ -161,7 +159,7 @@ public CarbonRowDataWriterProcessorStepImpl(CarbonDataLoadConfiguration configur
       }
     
       private void doExecute(Iterator<CarbonRowBatch> iterator, int iteratorIndex) throws IOException {
    -    String[] storeLocation = getStoreLocation(tableIdentifier);
    +    String[] storeLocation = getStoreLocation(this.configuration.getTableSpec().getCarbonTable());
    --- End diff --
   
    no need to pass the carbonTable from here, you can directly get in the calling method using Configuration object


---

[qiuchenjian-2]

Github user manishgupta88 commented on the issue:

    https://github.com/apache/carbondata/pull/2852
 
    Please add a Note for `addCarbonTable` method of `CarbonMetadata` class to say that the method should be used only in driver as clean up in executor is not handled  if this table is added to executor


---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

    https://github.com/apache/carbondata/pull/2852
 
    Build Success with Spark 2.1.0, Please check CI http://136.243.101.176:8080/job/ApacheCarbonPRBuilder2.1/1117/



---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

    https://github.com/apache/carbondata/pull/2852
 
    Build Success with Spark 2.2.1, Please check CI http://95.216.28.178:8080/job/ApacheCarbonPRBuilder1/1329/



---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

    https://github.com/apache/carbondata/pull/2852
 
    Build Success with Spark 2.3.1, Please check CI http://136.243.101.176:8080/job/carbondataprbuilder2.3/9381/



---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

    https://github.com/apache/carbondata/pull/2852
 
    Build Success with Spark 2.1.0, Please check CI http://136.243.101.176:8080/job/ApacheCarbonPRBuilder2.1/1137/



---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

    https://github.com/apache/carbondata/pull/2852
 
    Build Success with Spark 2.3.1, Please check CI http://136.243.101.176:8080/job/carbondataprbuilder2.3/9401/



---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

    https://github.com/apache/carbondata/pull/2852
 
    Build Success with Spark 2.2.1, Please check CI http://95.216.28.178:8080/job/ApacheCarbonPRBuilder1/1349/



---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

    https://github.com/apache/carbondata/pull/2852
 
    Build Success with Spark 2.1.0, Please check CI http://136.243.101.176:8080/job/ApacheCarbonPRBuilder2.1/1146/



---

[qiuchenjian-2]

Github user manishgupta88 commented on the issue:

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


---

[qiuchenjian-2]

Github user asfgit closed the pull request at:

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


---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

    https://github.com/apache/carbondata/pull/2852
 
    Build Failed with Spark 2.2.1, Please check CI http://95.216.28.178:8080/job/ApacheCarbonPRBuilder1/1358/



---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

    https://github.com/apache/carbondata/pull/2852
 
    Build Success with Spark 2.3.1, Please check CI http://136.243.101.176:8080/job/carbondataprbuilder2.3/9411/



---