[GitHub] carbondata pull request #2738: [CARBONDATA-2952] Provide c++ interface for S...

[qiuchenjian-2]

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

    https://github.com/apache/carbondata/pull/2738#discussion_r221149564
 
    --- Diff: store/sdk/src/main/java/org/apache/carbondata/sdk/file/CarbonReader.java ---
    @@ -90,6 +91,33 @@ public T readNextRow() throws IOException, InterruptedException {
         return currentReader.getCurrentValue();
       }
     
    +  /**
    +   * Read and return next string row object
    +   */
    +  public Object[] readNextStringRow() throws IOException, InterruptedException {
    --- End diff --
   
    readNextStringRow is not a standard API I think. It can not work with nested complex data types. Like Array[Array[...]], Struct[Array].
   
    I suggest to add a better API like CarbonRow wrapping on object[] and give reader utility methods like getInt, getString in CarbonRow .
   
    If it can't be finished in this PR scope, temporarily you can move this string making logic to JNI layer code so that it can be improved later. **Adding in java SDK layer will add more confusion to users**


---

[qiuchenjian-2]

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

    https://github.com/apache/carbondata/pull/2738#discussion_r221153398
 
    --- Diff: store/sdk/src/main/java/org/apache/carbondata/sdk/file/CarbonReader.java ---
    @@ -90,6 +91,33 @@ public T readNextRow() throws IOException, InterruptedException {
         return currentReader.getCurrentValue();
       }
     
    +  /**
    +   * Read and return next string row object
    +   */
    +  public Object[] readNextStringRow() throws IOException, InterruptedException {
    --- End diff --
   
    I am developing in local and will raise a new PR for it. CSDK only need read Array[String] now.


---

[qiuchenjian-2]

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

    https://github.com/apache/carbondata/pull/2738#discussion_r221218207
 
    --- Diff: store/sdk/src/main/java/org/apache/carbondata/sdk/file/CarbonReader.java ---
    @@ -90,6 +91,33 @@ public T readNextRow() throws IOException, InterruptedException {
         return currentReader.getCurrentValue();
       }
     
    +  /**
    +   * Read and return next string row object
    +   */
    +  public Object[] readNextStringRow() throws IOException, InterruptedException {
    --- End diff --
   
    OK.. Please add this limitation as only single dimension Array is supported in method signature and remove after enhancing with CarbonRow based API in other PR


---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

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



---

[qiuchenjian-2]

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

    https://github.com/apache/carbondata/pull/2738#discussion_r221423350
 
    --- Diff: store/sdk/src/main/java/org/apache/carbondata/sdk/file/CarbonReader.java ---
    @@ -90,6 +91,33 @@ public T readNextRow() throws IOException, InterruptedException {
         return currentReader.getCurrentValue();
       }
     
    +  /**
    +   * Read and return next string row object
    +   */
    +  public Object[] readNextStringRow() throws IOException, InterruptedException {
    --- End diff --
   
    OK，I added and will raise new PR for enhancing！


---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

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



---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

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



---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

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



---

[qiuchenjian-2]

Github user kunal642 commented on the issue:

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


---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

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



---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

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



---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

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



---

[qiuchenjian-2]

Github user kunal642 commented on the issue:

    https://github.com/apache/carbondata/pull/2738
 
    LGTM.
    One minor comment -> Reference for CSDK-guide.md is missing from README due to which there is no way to navigate to the guide. Please handle this as part of #2792


---

[qiuchenjian-2]

Github user asfgit closed the pull request at:

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


---