[GitHub] carbondata pull request #2363: [WIP][CARBONDATA-2591] SDK CarbonReader suppo...

[qiuchenjian-2]

Github user ajantha-bhat commented on a diff in the pull request:

    https://github.com/apache/carbondata/pull/2363#discussion_r194323041
 
    --- Diff: store/sdk/src/main/java/org/apache/carbondata/sdk/file/CarbonReaderBuilder.java ---
    @@ -179,7 +179,11 @@ public CarbonReaderBuilder setEndPoint(String value) {
         if (isTransactionalTable) {
           table = CarbonTable.buildFromTablePath(tableName, "default", tablePath);
         } else {
    -      table = CarbonTable.buildDummyTable(tablePath);
    +      if (filterExpression != null) {
    +        table = CarbonTable.buildTable(tablePath, tableName, true);
    +      } else {
    --- End diff --
   
    Add a comment about the changes and also can you make a separate method ?
    If filterExpression is null, call CarbonTable.buildDummyTable(). else call CarbonTable.buildTable()
   
    because buidTable will not tell whether it is a table with schema or without schema (dummy table).


---

[qiuchenjian-2]

Github user ajantha-bhat commented on a diff in the pull request:

    https://github.com/apache/carbondata/pull/2363#discussion_r194323660
 
    --- Diff: store/sdk/src/test/java/org/apache/carbondata/sdk/file/CarbonReaderTest.java ---
    @@ -105,6 +115,243 @@ public void testWriteAndReadFiles() throws IOException, InterruptedException {
         FileUtils.deleteDirectory(new File(path));
       }
     
    +  @Test
    +  public void testReadWithFilterOfTransactional() throws IOException, InterruptedException {
    --- End diff --
   
    All these code changes of this PR is for NonTransactional table.  I think test case for Transactional table is redundant.


---

[qiuchenjian-2]

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

    https://github.com/apache/carbondata/pull/2363#discussion_r194324248
 
    --- Diff: store/sdk/src/test/java/org/apache/carbondata/sdk/file/CarbonReaderTest.java ---
    @@ -105,6 +115,243 @@ public void testWriteAndReadFiles() throws IOException, InterruptedException {
         FileUtils.deleteDirectory(new File(path));
       }
     
    +  @Test
    +  public void testReadWithFilterOfTransactional() throws IOException, InterruptedException {
    --- End diff --
   
    I think we should add test case for Transactional table , ensure the Transactional table  is available too


---

[qiuchenjian-2]

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

    https://github.com/apache/carbondata/pull/2363#discussion_r194334876
 
    --- Diff: store/sdk/src/main/java/org/apache/carbondata/sdk/file/CarbonReaderBuilder.java ---
    @@ -179,7 +179,11 @@ public CarbonReaderBuilder setEndPoint(String value) {
         if (isTransactionalTable) {
           table = CarbonTable.buildFromTablePath(tableName, "default", tablePath);
         } else {
    -      table = CarbonTable.buildDummyTable(tablePath);
    +      if (filterExpression != null) {
    +        table = CarbonTable.buildTable(tablePath, tableName, true);
    +      } else {
    --- End diff --
   
    ok, done


---

[qiuchenjian-2]

Github user ravipesala commented on the issue:

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



---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

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



---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

    https://github.com/apache/carbondata/pull/2363
 
    Build Success with Spark 2.2.1, Please check CI http://88.99.58.216:8080/job/ApacheCarbonPRBuilder/5131/



---

[qiuchenjian-2]

Github user ajantha-bhat commented on the issue:

    https://github.com/apache/carbondata/pull/2363
 
    LGTM.
   
    This fix need to merge to both master and 1.4 branch


---

[qiuchenjian-2]

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

    https://github.com/apache/carbondata/pull/2363#discussion_r194382421
 
    --- Diff: core/src/main/java/org/apache/carbondata/core/metadata/schema/table/CarbonTable.java ---
    @@ -218,6 +222,40 @@ public static void updateTableInfo(TableInfo tableInfo) {
         }
       }
     
    +  public static CarbonTable buildTable(
    +      String tablePath,
    +      String tableName,
    +      boolean hasFilter) throws IOException {
    --- End diff --
   
    There should not be any filter related flags in Carbon Table API, if you require this check then move to utilities. It should only has buildTable method not `hasFilter` flag.


---

[qiuchenjian-2]

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

    https://github.com/apache/carbondata/pull/2363#discussion_r194595227
 
    --- Diff: core/src/main/java/org/apache/carbondata/core/metadata/schema/table/CarbonTable.java ---
    @@ -218,6 +222,40 @@ public static void updateTableInfo(TableInfo tableInfo) {
         }
       }
     
    +  public static CarbonTable buildTable(
    +      String tablePath,
    +      String tableName,
    +      boolean hasFilter) throws IOException {
    --- End diff --
   
    ok.done


---

[qiuchenjian-2]

Github user ravipesala commented on the issue:

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



---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

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



---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

    https://github.com/apache/carbondata/pull/2363
 
    Build Failed with Spark 2.2.1, Please check CI http://88.99.58.216:8080/job/ApacheCarbonPRBuilder/5138/



---

[qiuchenjian-2]

Github user xubo245 commented on the issue:

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


---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

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



---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

    https://github.com/apache/carbondata/pull/2363
 
    Build Success with Spark 2.2.1, Please check CI http://88.99.58.216:8080/job/ApacheCarbonPRBuilder/5139/



---

[qiuchenjian-2]

Github user ravipesala commented on the issue:

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


---

[qiuchenjian-2]

Github user asfgit closed the pull request at:

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


---

[qiuchenjian-2]

Github user ajantha-bhat commented on the issue:

    https://github.com/apache/carbondata/pull/2363
 
    @ravipesala : please merge this fix to 1.4.0 branch also



---