[GitHub] carbondata pull request #2678: [WIP] Multi user support for SDK on S3

[qiuchenjian-2]

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

    https://github.com/apache/carbondata/pull/2678#discussion_r216130682
 
    --- Diff: core/src/main/java/org/apache/carbondata/core/indexstore/blockletindex/SegmentIndexFileStore.java ---
    @@ -76,20 +77,29 @@
        */
       private Map<String, List<String>> carbonMergeFileToIndexFilesMap;
     
    +  private Configuration configuration;
    +
       public SegmentIndexFileStore() {
         carbonIndexMap = new HashMap<>();
         carbonIndexMapWithFullPath = new TreeMap<>();
         carbonMergeFileToIndexFilesMap = new HashMap<>();
       }
     
    +  public SegmentIndexFileStore(Configuration configuration) {
    +    carbonIndexMap = new HashMap<>();
    +    carbonIndexMapWithFullPath = new TreeMap<>();
    +    carbonMergeFileToIndexFilesMap = new HashMap<>();
    +    this.configuration = configuration;
    +  }
    +
       /**
        * Read all index files and keep the cache in it.
        *
        * @param segmentPath
        * @throws IOException
        */
       public void readAllIIndexOfSegment(String segmentPath) throws IOException {
    -    CarbonFile[] carbonIndexFiles = getCarbonIndexFiles(segmentPath);
    +    CarbonFile[] carbonIndexFiles = getCarbonIndexFiles(segmentPath, configuration);
    --- End diff --
   
    If Constructor with Configuration is not used, it will throw NPE in this method call..
   
    I have observed below stacktrace in a test. Please take care
   
    java.lang.NullPointerException
    at org.apache.hadoop.fs.FileSystem.get(FileSystem.java:378)
    at org.apache.hadoop.fs.Path.getFileSystem(Path.java:295)
    at org.apache.carbondata.core.datastore.filesystem.AbstractDFSCarbonFile.getDataInputStream(AbstractDFSCarbonFile.java:336)
    at org.apache.carbondata.core.datastore.filesystem.AbstractDFSCarbonFile.getDataInputStream(AbstractDFSCarbonFile.java:302)
    at org.apache.carbondata.core.datastore.impl.FileFactory.getDataInputStream(FileFactory.java:125)
    at org.apache.carbondata.core.datastore.impl.FileFactory.getDataInputStream(FileFactory.java:116)
    at org.apache.carbondata.core.indexstore.blockletindex.SegmentIndexFileStore.readIndexFile(SegmentIndexFileStore.java:298)
    at org.apache.carbondata.core.indexstore.blockletindex.SegmentIndexFileStore.readAllIIndexOfSegment(SegmentIndexFileStore.java:156)
    at org.apache.carbondata.core.metadata.SegmentFileStore.readIndexFiles(SegmentFileStore.java:497)
    at org.apache.carbondata.core.metadata.SegmentFileStore.readIndexFiles(SegmentFileStore.java:474)
    at org.apache.carbondata.core.util.CarbonUtil.getDataSizeAndIndexSize(CarbonUtil.java:2624)
    at org.apache.carbondata.core.util.CarbonUtil.getDataSizeAndIndexSize(CarbonUtil.java:2675)
    at org.apache.carbondata.processing.util.CarbonLoaderUtil.addDataIndexSizeIntoMetaEntry(CarbonLoaderUtil.java:1124)
    at org.apache.carbondata.spark.rdd.CarbonDataRDDFactory$.updateTableStatus(CarbonDataRDDFactory.scala:934)
    at org.apache.carbondata.spark.rdd.CarbonDataRDDFactory$.loadCarbonData(CarbonDataRDDFactory.scala:539)
    at org.apache.spark.sql.execution.command.management.CarbonLoadDataCommand.loadData(CarbonLoadDataCommand.scala:594)
    at org.apache.spark.sql.execution.command.management.CarbonLoadDataCommand.processData(CarbonLoadDataCommand.scala:319)
    at org.apache.spark.sql.execution.command.management.CarbonInsertIntoCommand.processData(CarbonInsertIntoCommand.scala:85)


---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

    https://github.com/apache/carbondata/pull/2678
 
    Build Failed  with Spark 2.1.0, Please check CI http://136.243.101.176:8080/job/ApacheCarbonPRBuilder2.1/181/



---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

    https://github.com/apache/carbondata/pull/2678
 
    Build Failed  with Spark 2.3.1, Please check CI http://136.243.101.176:8080/job/ApacheCarbonPRBuilder2.3/8420/



---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

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



---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

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



---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

    https://github.com/apache/carbondata/pull/2678
 
    Build Failed  with Spark 2.3.1, Please check CI http://136.243.101.176:8080/job/ApacheCarbonPRBuilder2.3/8421/



---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

    https://github.com/apache/carbondata/pull/2678
 
    Build Failed  with Spark 2.1.0, Please check CI http://136.243.101.176:8080/job/ApacheCarbonPRBuilder2.1/182/



---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

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



---

[qiuchenjian-2]

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

    https://github.com/apache/carbondata/pull/2678#discussion_r216198665
 
    --- Diff: core/src/main/java/org/apache/carbondata/core/indexstore/blockletindex/SegmentIndexFileStore.java ---
    @@ -76,20 +77,29 @@
        */
       private Map<String, List<String>> carbonMergeFileToIndexFilesMap;
     
    +  private Configuration configuration;
    +
       public SegmentIndexFileStore() {
         carbonIndexMap = new HashMap<>();
         carbonIndexMapWithFullPath = new TreeMap<>();
         carbonMergeFileToIndexFilesMap = new HashMap<>();
       }
     
    +  public SegmentIndexFileStore(Configuration configuration) {
    +    carbonIndexMap = new HashMap<>();
    +    carbonIndexMapWithFullPath = new TreeMap<>();
    +    carbonMergeFileToIndexFilesMap = new HashMap<>();
    +    this.configuration = configuration;
    +  }
    +
       /**
        * Read all index files and keep the cache in it.
        *
        * @param segmentPath
        * @throws IOException
        */
       public void readAllIIndexOfSegment(String segmentPath) throws IOException {
    -    CarbonFile[] carbonIndexFiles = getCarbonIndexFiles(segmentPath);
    +    CarbonFile[] carbonIndexFiles = getCarbonIndexFiles(segmentPath, configuration);
    --- End diff --
   
    Fixed


---

[qiuchenjian-2]

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

    https://github.com/apache/carbondata/pull/2678#discussion_r216198687
 
    --- Diff: core/src/main/java/org/apache/carbondata/core/datamap/dev/DataMap.java ---
    @@ -35,7 +37,8 @@
       /**
        * It is called to load the data map to memory or to initialize it.
        */
    -  void init(DataMapModel dataMapModel) throws MemoryException, IOException;
    +  void init(DataMapModel dataMapModel, Configuration configuration)
    --- End diff --
   
    done


---

[qiuchenjian-2]

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

    https://github.com/apache/carbondata/pull/2678#discussion_r216198700
 
    --- Diff: core/src/main/java/org/apache/carbondata/core/indexstore/blockletindex/SegmentIndexFileStore.java ---
    @@ -354,6 +364,22 @@ private MergedBlockIndex readMergeBlockIndex(ThriftReader thriftReader) throws I
         });
       }
     
    +  /**
    +   * List all the index files of the segment.
    +   *
    +   * @param segmentPath
    +   * @return
    +   */
    +  public static CarbonFile[] getCarbonIndexFiles(String segmentPath, Configuration configuration) {
    --- End diff --
   
    ok


---

[qiuchenjian-2]

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

    https://github.com/apache/carbondata/pull/2678#discussion_r216198714
 
    --- Diff: core/src/main/java/org/apache/carbondata/core/reader/CarbonIndexFileReader.java ---
    @@ -79,6 +80,17 @@ public void openThriftReader(String filePath) throws IOException {
         thriftReader.open();
       }
     
    +  /**
    +   * Open the thrift reader
    +   *
    +   * @param filePath
    +   * @throws IOException
    +   */
    +  public void openThriftReader(String filePath, Configuration configuration) throws IOException {
    --- End diff --
   
    ok


---

[qiuchenjian-2]

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

    https://github.com/apache/carbondata/pull/2678#discussion_r216198723
 
    --- Diff: core/src/main/java/org/apache/carbondata/core/reader/ThriftReader.java ---
    @@ -87,6 +88,15 @@ public void open() throws IOException {
         binaryIn = new TCompactProtocol(new TIOStreamTransport(dataInputStream));
       }
     
    +  /**
    +   * Opens the fileName for reading.
    +   */
    +  public void open(Configuration configuration) throws IOException {
    --- End diff --
   
    done


---

[qiuchenjian-2]

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

    https://github.com/apache/carbondata/pull/2678#discussion_r216198737
 
    --- Diff: core/src/main/java/org/apache/carbondata/core/statusmanager/SegmentStatusManager.java ---
    @@ -99,20 +100,21 @@ public static long getTableStatusLastModifiedTime(AbsoluteTableIdentifier identi
        * @return
        * @throws IOException
        */
    -  public ValidAndInvalidSegmentsInfo getValidAndInvalidSegments() throws IOException {
    -    return getValidAndInvalidSegments(null, null);
    +  public ValidAndInvalidSegmentsInfo getValidAndInvalidSegments(Configuration configuration)
    --- End diff --
   
    done


---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

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



---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

    https://github.com/apache/carbondata/pull/2678
 
    Build Failed  with Spark 2.3.1, Please check CI http://136.243.101.176:8080/job/ApacheCarbonPRBuilder2.3/8425/



---

[qiuchenjian-2]

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

    https://github.com/apache/carbondata/pull/2678#discussion_r216207294
 
    --- Diff: core/src/main/java/org/apache/carbondata/core/statusmanager/SegmentStatusManager.java ---
    @@ -93,26 +101,19 @@ public static long getTableStatusLastModifiedTime(AbsoluteTableIdentifier identi
         }
       }
     
    -  /**
    -   * get valid segment for given table
    -   *
    -   * @return
    -   * @throws IOException
    -   */
       public ValidAndInvalidSegmentsInfo getValidAndInvalidSegments() throws IOException {
    -    return getValidAndInvalidSegments(null, null);
    -  }
    -
    -  public ValidAndInvalidSegmentsInfo getValidAndInvalidSegments(
    -      LoadMetadataDetails[] loadMetadataDetails) throws IOException {
    -    return getValidAndInvalidSegments(loadMetadataDetails, null);
    +    if (configuration == null) {
    +      configuration = FileFactory.getConfiguration();
    +    }
    +    return getValidAndInvalidSegments(null, null, configuration);
       }
     
       /**
        * get valid segment for given load status details.
        */
       public ValidAndInvalidSegmentsInfo getValidAndInvalidSegments(
    -      LoadMetadataDetails[] loadMetadataDetails, ReadCommittedScope readCommittedScope)
    +      LoadMetadataDetails[] loadMetadataDetails, ReadCommittedScope readCommittedScope,
    +      Configuration configuration)
    --- End diff --
   
    No need to pass as it is already available in the class


---

[qiuchenjian-2]

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

    https://github.com/apache/carbondata/pull/2678#discussion_r216207416
 
    --- Diff: core/src/main/java/org/apache/carbondata/core/statusmanager/SegmentStatusManager.java ---
    @@ -93,26 +101,19 @@ public static long getTableStatusLastModifiedTime(AbsoluteTableIdentifier identi
         }
       }
     
    -  /**
    -   * get valid segment for given table
    -   *
    -   * @return
    -   * @throws IOException
    -   */
       public ValidAndInvalidSegmentsInfo getValidAndInvalidSegments() throws IOException {
    -    return getValidAndInvalidSegments(null, null);
    -  }
    -
    -  public ValidAndInvalidSegmentsInfo getValidAndInvalidSegments(
    -      LoadMetadataDetails[] loadMetadataDetails) throws IOException {
    -    return getValidAndInvalidSegments(loadMetadataDetails, null);
    +    if (configuration == null) {
    --- End diff --
   
    Please do this check in the constructor


---

[qiuchenjian-2]

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

    https://github.com/apache/carbondata/pull/2678#discussion_r216207701
 
    --- Diff: core/src/main/java/org/apache/carbondata/core/util/AbstractDataFileFooterConverter.java ---
    @@ -135,16 +138,17 @@ private static BitSet getPresenceMeta(
        * @return list of index info
        * @throws IOException problem while reading the index file
        */
    -  public List<DataFileFooter> getIndexInfo(String filePath, byte[] fileData) throws IOException {
    -    return getIndexInfo(filePath, fileData, true);
    +  public List<DataFileFooter> getIndexInfo(String filePath, byte[] fileData,
    +      Configuration configuration) throws IOException {
    --- End diff --
   
    Is it not possible to take from constructor?


---

[qiuchenjian-2]

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

    https://github.com/apache/carbondata/pull/2678#discussion_r216208118
 
    --- Diff: integration/spark2/src/main/scala/org/apache/spark/sql/execution/command/partition/CarbonAlterTableDropHivePartitionCommand.scala ---
    @@ -171,7 +171,7 @@ case class CarbonAlterTableDropHivePartitionCommand(
             ""
           }
           val segments = new SegmentStatusManager(table.getAbsoluteTableIdentifier)
    -        .getValidAndInvalidSegments.getValidSegments
    +        .getValidAndInvalidSegments().getValidSegments
    --- End diff --
   
    no need to change this


---