[GitHub] carbondata pull request #2816: [CARBONDATA-300] Suppor read batch row in CSD...

[qiuchenjian-2]

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

    https://github.com/apache/carbondata/pull/2816#discussion_r227679865
 
    --- Diff: core/src/main/java/org/apache/carbondata/core/scan/result/iterator/ChunkRowIterator.java ---
    @@ -74,4 +76,13 @@ public ChunkRowIterator(CarbonIterator<RowBatch> iterator) {
         return currentChunk.next();
       }
     
    +  /**
    +   * get
    --- End diff --
   
    description is not correct


---

[qiuchenjian-2]

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

    https://github.com/apache/carbondata/pull/2816#discussion_r227680374
 
    --- Diff: store/CSDK/CMakeLists.txt ---
    @@ -1,17 +1,17 @@
    -cmake_minimum_required (VERSION 2.8)
    -project (CJDK)
    +cmake_minimum_required(VERSION 2.8)
    --- End diff --
   
    Shouldn't add license header?


---

[qiuchenjian-2]

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

    https://github.com/apache/carbondata/pull/2816#discussion_r227680646
 
    --- Diff: store/CSDK/CarbonReader.cpp ---
    @@ -17,6 +17,7 @@
     
     #include "CarbonReader.h"
     #include <jni.h>
    +#include <sys/time.h>
    --- End diff --
   
    I think system header file should be before carbon header file, so "CarbonReader.h" should be moved down


---

[qiuchenjian-2]

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

    https://github.com/apache/carbondata/pull/2816#discussion_r227681257
 
    --- Diff: store/CSDK/main.cpp ---
    @@ -21,6 +21,8 @@
     #include <iostream>
     #include <unistd.h>
     #include "CarbonReader.h"
    +#include "CarbonRow.h"
    +#include <sys/time.h>
    --- End diff --
   
    Move before "CarbonReader.h"


---

[qiuchenjian-2]

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

    https://github.com/apache/carbondata/pull/2816#discussion_r227681343
 
    --- Diff: store/CSDK/main.cpp ---
    @@ -21,6 +21,8 @@
     #include <iostream>
     #include <unistd.h>
     #include "CarbonReader.h"
    +#include "CarbonRow.h"
    +#include <sys/time.h>
     
     using namespace std;
    --- End diff --
   
    why this file is called main.cpp?


---

[qiuchenjian-2]

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

    https://github.com/apache/carbondata/pull/2816#discussion_r227681593
 
    --- Diff: store/sdk/src/test/java/org/apache/carbondata/sdk/file/CarbonReaderTest.java ---
    @@ -1521,4 +1521,294 @@ public boolean accept(File dir, String name) {
           e.printStackTrace();
         }
       }
    +
    +   @Test
    --- End diff --
   
    Is there testcase in CPP?


---

[qiuchenjian-2]

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

    https://github.com/apache/carbondata/pull/2816#discussion_r227681835
 
    --- Diff: README.md ---
    @@ -61,6 +61,7 @@ CarbonData is built using Apache Maven, to [build CarbonData](https://github.com
      * [CarbonData Pre-aggregate DataMap](https://github.com/apache/carbondata/blob/master/docs/preaggregate-datamap-guide.md)
      * [CarbonData Timeseries DataMap](https://github.com/apache/carbondata/blob/master/docs/timeseries-datamap-guide.md)
     * [SDK Guide](https://github.com/apache/carbondata/blob/master/docs/sdk-guide.md)
    +* [CSDK Guide](https://github.com/apache/carbondata/blob/master/docs/CSDK-guide.md)
    --- End diff --
   
    I think it is better call C++ SDK Guide. The SDK is for C++.


---

[qiuchenjian-2]

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

    https://github.com/apache/carbondata/pull/2816#discussion_r228024318
 
    --- Diff: core/src/main/java/org/apache/carbondata/core/scan/result/iterator/ChunkRowIterator.java ---
    @@ -74,4 +76,13 @@ public ChunkRowIterator(CarbonIterator<RowBatch> iterator) {
         return currentChunk.next();
       }
     
    +  /**
    +   * get
    --- End diff --
   
    ok,optimized


---

[qiuchenjian-2]

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

    https://github.com/apache/carbondata/pull/2816#discussion_r228025429
 
    --- Diff: store/CSDK/CMakeLists.txt ---
    @@ -1,17 +1,17 @@
    -cmake_minimum_required (VERSION 2.8)
    -project (CJDK)
    +cmake_minimum_required(VERSION 2.8)
    --- End diff --
   
    I think no need to add license header in this file.
    CMakeLists.txt like pom.xml, it's not code file.
    Tensorflow and caffe project also didn't add license header in CMakeLists.txt .


---

[qiuchenjian-2]

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

    https://github.com/apache/carbondata/pull/2816#discussion_r228025597
 
    --- Diff: store/CSDK/CarbonReader.cpp ---
    @@ -17,6 +17,7 @@
     
     #include "CarbonReader.h"
     #include <jni.h>
    +#include <sys/time.h>
    --- End diff --
   
    ok, done


---

[qiuchenjian-2]

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

    https://github.com/apache/carbondata/pull/2816#discussion_r228025690
 
    --- Diff: store/CSDK/main.cpp ---
    @@ -21,6 +21,8 @@
     #include <iostream>
     #include <unistd.h>
     #include "CarbonReader.h"
    +#include "CarbonRow.h"
    +#include <sys/time.h>
    --- End diff --
   
    ok, I also change the others.


---

[qiuchenjian-2]

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

    https://github.com/apache/carbondata/pull/2816#discussion_r228026273
 
    --- Diff: store/CSDK/CMakeLists.txt ---
    @@ -1,17 +1,17 @@
    -cmake_minimum_required (VERSION 2.8)
    -project (CJDK)
    +cmake_minimum_required(VERSION 2.8)
    --- End diff --
   
    ok, added


---

[qiuchenjian-2]

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

    https://github.com/apache/carbondata/pull/2816#discussion_r228028346
 
    --- Diff: store/CSDK/main.cpp ---
    @@ -21,6 +21,8 @@
     #include <iostream>
     #include <unistd.h>
     #include "CarbonReader.h"
    +#include "CarbonRow.h"
    +#include <sys/time.h>
     
     using namespace std;
    --- End diff --
   
    This is main file in C/C++. but only for test. In the future, CSDK will support test framework(such as googletest) to instead of  main.cpp.


---

[qiuchenjian-2]

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

    https://github.com/apache/carbondata/pull/2816#discussion_r228028896
 
    --- Diff: README.md ---
    @@ -61,6 +61,7 @@ CarbonData is built using Apache Maven, to [build CarbonData](https://github.com
      * [CarbonData Pre-aggregate DataMap](https://github.com/apache/carbondata/blob/master/docs/preaggregate-datamap-guide.md)
      * [CarbonData Timeseries DataMap](https://github.com/apache/carbondata/blob/master/docs/timeseries-datamap-guide.md)
     * [SDK Guide](https://github.com/apache/carbondata/blob/master/docs/sdk-guide.md)
    +* [CSDK Guide](https://github.com/apache/carbondata/blob/master/docs/CSDK-guide.md)
    --- End diff --
   
    ok, done


---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

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



---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

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



---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

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



---

[qiuchenjian-2]

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

    https://github.com/apache/carbondata/pull/2816#discussion_r228156985
 
    --- Diff: store/sdk/src/test/java/org/apache/carbondata/sdk/file/CarbonReaderTest.java ---
    @@ -1616,7 +1616,6 @@ public boolean accept(File dir, String name) {
             assertEquals(RowUtil.getDouble(data, 4), ((double) i) / 2);
             assert (RowUtil.getBoolean(data, 5));
             assertEquals(RowUtil.getInt(data, 6), 17957);
    -        assertEquals(RowUtil.getLong(data, 7), 1549911814000000L);
    --- End diff --
   
    Why removed this validation ?


---

[qiuchenjian-2]

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

    https://github.com/apache/carbondata/pull/2816#discussion_r228183334
 
    --- Diff: core/src/main/java/org/apache/carbondata/core/scan/result/RowBatch.java ---
    @@ -100,4 +100,24 @@ public int getSize() {
         counter++;
         return row;
       }
    +
    +  /**
    +   * read next batch
    +   *
    +   * @param batch batch size
    +   * @return rows
    +   */
    +  public List<Object[]> nextBatch(int batch) {
    +    if (!hasNext()) {
    +      throw new NoSuchElementException();
    +    }
    +    List<Object[]> row;
    +    if (counter + batch > rows.size()) {
    +      row = rows.subList(counter, rows.size());
    +    } else {
    +      row = rows.subList(counter, counter + batch);
    +    }
    +    counter = counter + batch;
    --- End diff --
   
    What if code enters if check (line 116), incrementing a counter by batch size wrong. because we fetched data lesser that batch size (only till row size)


---

[qiuchenjian-2]

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

    https://github.com/apache/carbondata/pull/2816#discussion_r228191657
 
    --- Diff: examples/spark2/src/main/java/org/apache/carbondata/benchmark/SDKReaderBenchmark.java ---
    @@ -0,0 +1,262 @@
    +/*
    + * Licensed to the Apache Software Foundation (ASF) under one or more
    + * contributor license agreements.  See the NOTICE file distributed with
    + * this work for additional information regarding copyright ownership.
    + * The ASF licenses this file to You under the Apache License, Version 2.0
    + * (the "License"); you may not use this file except in compliance with
    + * the License.  You may obtain a copy of the License at
    + *
    + *    http://www.apache.org/licenses/LICENSE-2.0
    + *
    + * Unless required by applicable law or agreed to in writing, software
    + * distributed under the License is distributed on an "AS IS" BASIS,
    + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
    + * See the License for the specific language governing permissions and
    + * limitations under the License.
    + */
    +
    +package org.apache.carbondata.benchmark;
    +
    +import java.io.File;
    +import java.io.FilenameFilter;
    +import java.io.IOException;
    +import java.sql.Timestamp;
    +import java.util.HashMap;
    +import java.util.Map;
    +import java.util.Random;
    +
    +import org.apache.hadoop.conf.Configuration;
    +
    +import org.apache.carbondata.common.exceptions.sql.InvalidLoadOptionException;
    +import org.apache.carbondata.core.constants.CarbonCommonConstants;
    +import org.apache.carbondata.core.util.CarbonProperties;
    +import org.apache.carbondata.sdk.file.*;
    +
    +/**
    + * Test SDK read performance
    + */
    +public class SDKReaderBenchmark {
    --- End diff --
   
    This call not required. Please remove.


---