[GitHub] carbondata pull request #2546: [CARBONDATA-2775] Adaptive encoding fails for...

[qiuchenjian-2]

GitHub user ajantha-bhat opened a pull request:

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

    [CARBONDATA-2775] Adaptive encoding fails for Unsafe OnHeap if, target data type is SHORT_INT

    problem:
    [CARBONDATA-2775] Adaptive encoding fails for Unsafe OnHeap if, target data type is SHORT_INT
   
    solution: If ENABLE_OFFHEAP_SORT = false, in carbon property.  UnsafeFixLengthColumnPage.java will use different compress logic. Not the raw compression. In that case, for SHORT_INT data type , conversion need to handle.
   
    Be sure to do all of the following checklist to help us incorporate
    your contribution quickly and easily:
   
     - [ ] Any interfaces changed? NA
     
     - [ ] Any backward compatibility impacted? NA
     
     - [ ] Document update required? NA
   
     - [ ] Testing done
    updated UT. Which will run all adaptive test cases with  ENABLE_OFFHEAP_SORT = false.      
   
     - [ ] For large changes, please consider breaking it into sub-tasks under an umbrella JIRA.  NA
   


You can merge this pull request into a Git repository by running:

    $ git pull https://github.com/ajantha-bhat/carbondata issue_fix

Alternatively you can review and apply these changes as the patch at:

    https://github.com/apache/carbondata/pull/2546.patch

To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:

    This closes #2546
   
----
commit 1e8d06a219010983dd9b97b6869206b3678ac68f
Author: ajantha-bhat <ajanthabhat@...>
Date:   2018-07-24T07:03:47Z

    [CARBONDATA-2775] Adaptive encoding fails for Unsafe OnHeap. if, target datatype is SHORT_INT

----


---

[qiuchenjian-2]

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

    https://github.com/apache/carbondata/pull/2546#discussion_r204654611
 
    --- Diff: core/src/main/java/org/apache/carbondata/core/datastore/page/UnsafeFixLengthColumnPage.java ---
    @@ -495,6 +495,8 @@ private int getEndLoop() {
           return totalLength / ByteUtil.SIZEOF_BYTE;
         } else if (dataType == DataTypes.SHORT) {
           return totalLength / ByteUtil.SIZEOF_SHORT;
    +    } else if (dataType == DataTypes.SHORT_INT) {
    --- End diff --
   
    I think this is also required for SafeFixLengthColumnPage please check


---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

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



---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

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



---

[qiuchenjian-2]

Github user ajantha-bhat commented on the issue:

    https://github.com/apache/carbondata/pull/2546
 
    @ravipesala : please review


---

[qiuchenjian-2]

Github user kumarvishal09 commented on the issue:

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


---

[qiuchenjian-2]

Github user ajantha-bhat commented on the issue:

    https://github.com/apache/carbondata/pull/2546
 
    retest SDV please


---

[qiuchenjian-2]

Github user brijoobopanna commented on the issue:

    https://github.com/apache/carbondata/pull/2546
 
    retest sdv please


---

[qiuchenjian-2]

Github user ravipesala commented on the issue:

    https://github.com/apache/carbondata/pull/2546
 
    SDV Build Fail , Please check CI http://144.76.159.231:8080/job/ApacheSDVTests/6000/



---

[qiuchenjian-2]

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

    https://github.com/apache/carbondata/pull/2546#discussion_r205409053
 
    --- Diff: integration/spark-common-test/src/test/scala/org/apache/carbondata/integration/spark/testsuite/complexType/TestAdaptiveEncodingUnsafeHeapColumnPageForComplexDataType.scala ---
    @@ -0,0 +1,61 @@
    +/*
    + * 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.integration.spark.testsuite.complexType
    +
    +import java.io.File
    +
    +import org.apache.spark.sql.test.util.QueryTest
    +import org.scalatest.BeforeAndAfterAll
    +
    +import org.apache.carbondata.core.constants.CarbonCommonConstants
    +import org.apache.carbondata.core.util.CarbonProperties
    +
    +/**
    + * Test class of Adaptive Encoding UnSafe Column Page with Complex Data type
    + *
    + */
    +
    +class TestAdaptiveEncodingUnsafeHeapColumnPageForComplexDataType
    +  extends QueryTest with BeforeAndAfterAll with TestAdaptiveComplexType {
    +
    +  override def beforeAll(): Unit = {
    +
    +    new File(CarbonProperties.getInstance().getSystemFolderLocation).delete()
    +    sql("DROP TABLE IF EXISTS adaptive")
    +    CarbonProperties.getInstance()
    +      .addProperty(CarbonCommonConstants.ENABLE_UNSAFE_COLUMN_PAGE,
    +        "true")
    +
    +    CarbonProperties.getInstance()
    +      .addProperty(CarbonCommonConstants.ENABLE_OFFHEAP_SORT,
    +        "false")
    +  }
    +
    --- End diff --
   
    1. only beforeAll and afterAll implementation is there....where is the test case?
    2. Avoid creating a new class, try to add the test case in one of existing Encoding test case related classes


---

[qiuchenjian-2]

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

    https://github.com/apache/carbondata/pull/2546#discussion_r205410245
 
    --- Diff: integration/spark-common-test/src/test/scala/org/apache/carbondata/integration/spark/testsuite/complexType/TestAdaptiveEncodingUnsafeHeapColumnPageForComplexDataType.scala ---
    @@ -0,0 +1,61 @@
    +/*
    + * 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.integration.spark.testsuite.complexType
    +
    +import java.io.File
    +
    +import org.apache.spark.sql.test.util.QueryTest
    +import org.scalatest.BeforeAndAfterAll
    +
    +import org.apache.carbondata.core.constants.CarbonCommonConstants
    +import org.apache.carbondata.core.util.CarbonProperties
    +
    +/**
    + * Test class of Adaptive Encoding UnSafe Column Page with Complex Data type
    + *
    + */
    +
    +class TestAdaptiveEncodingUnsafeHeapColumnPageForComplexDataType
    +  extends QueryTest with BeforeAndAfterAll with TestAdaptiveComplexType {
    +
    +  override def beforeAll(): Unit = {
    +
    +    new File(CarbonProperties.getInstance().getSystemFolderLocation).delete()
    +    sql("DROP TABLE IF EXISTS adaptive")
    +    CarbonProperties.getInstance()
    +      .addProperty(CarbonCommonConstants.ENABLE_UNSAFE_COLUMN_PAGE,
    +        "true")
    +
    +    CarbonProperties.getInstance()
    +      .addProperty(CarbonCommonConstants.ENABLE_OFFHEAP_SORT,
    +        "false")
    +  }
    +
    --- End diff --
   
    @manishgupta88 :
    1. This new class extends, **TestAdaptiveComplexType** . So all the test case present in that will run with this carbon property set.  we need both the scenarios to test with and without property.
    2. Actually in this case creating a new class by extending one class has avoided the duplication of the test case. It is very efficient test case !


---

[qiuchenjian-2]

Github user ajantha-bhat commented on the issue:

    https://github.com/apache/carbondata/pull/2546
 
    retest sdv please


---

[qiuchenjian-2]

Github user ravipesala commented on the issue:

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



---

[qiuchenjian-2]

Github user ajantha-bhat commented on the issue:

    https://github.com/apache/carbondata/pull/2546
 
    @kumarvishal09 : PR is ready. please Merge


---

[qiuchenjian-2]

Github user brijoobopanna commented on the issue:

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


---

[qiuchenjian-2]

Github user brijoobopanna commented on the issue:

    https://github.com/apache/carbondata/pull/2546
 
    retest sdv please


---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

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



---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

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



---

[qiuchenjian-2]

Github user ravipesala commented on the issue:

    https://github.com/apache/carbondata/pull/2546
 
    SDV Build Fail , Please check CI http://144.76.159.231:8080/job/ApacheSDVTests/6048/



---

[qiuchenjian-2]

Github user kumarvishal09 commented on the issue:

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


---