[GitHub] incubator-carbondata pull request #697: [CARBONDATA-708] Fixed Between and L...

[qiuchenjian-2]

GitHub user bhavya411 opened a pull request:

    https://github.com/apache/incubator-carbondata/pull/697

    [CARBONDATA-708] Fixed Between and Logical AND issue

    - Change CarbonFilters.scala to fix BETWEEN and Logical AND operator
    - Add  Unit Test Cases
    - Tested on Spark 1.6 and 2.1
    - Manual testing done to verify the fix
    - Build Success with 1.6 and 2.1

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

    $ git pull https://github.com/bhavya411/incubator-carbondata CARBONDATA-708

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

    https://github.com/apache/incubator-carbondata/pull/697.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 #697
   
----
commit b53f711af9324d765cbe9ee3c45ca58fca8b6a85
Author: Bhavya <[hidden email]>
Date:   2017-03-24T11:16:39Z

    Fixed issue 708 and added testcases for the same

----


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [hidden email] or file a JIRA ticket
with INFRA.
---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

    https://github.com/apache/incubator-carbondata/pull/697
 
    Build Success with Spark 1.6.2, Please check CI http://136.243.101.176:8080/job/ApacheCarbonPRBuilder/1327/



---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [hidden email] or file a JIRA ticket
with INFRA.
---

[qiuchenjian-2]

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

    https://github.com/apache/incubator-carbondata/pull/697#discussion_r107920260
 
    --- Diff: integration/spark-common/src/main/scala/org/apache/carbondata/spark/CarbonFilters.scala ---
    @@ -111,16 +111,24 @@ object CarbonFilters {
         }
     
         def getCarbonLiteralExpression(name: String, value: Any): CarbonExpression = {
    -      new CarbonLiteralExpression(value,
    -        CarbonScalaUtil.convertSparkToCarbonDataType(dataTypeOf(name)))
    +      val dataTypeOfAttribute = CarbonScalaUtil.convertSparkToCarbonDataType(dataTypeOf(name))
    +      val dataType = if (Option(value).isDefined
    +                         && dataTypeOfAttribute == DataType.STRING
    +                         && value.isInstanceOf[Double]) {
    +        DataType.DOUBLE
    --- End diff --
   
    what's the reason to change datatype?


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [hidden email] or file a JIRA ticket
with INFRA.
---

[qiuchenjian-2]

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

    https://github.com/apache/incubator-carbondata/pull/697#discussion_r107919139
 
    --- Diff: integration/spark-common/src/main/scala/org/apache/carbondata/spark/CarbonFilters.scala ---
    @@ -111,16 +111,24 @@ object CarbonFilters {
         }
     
         def getCarbonLiteralExpression(name: String, value: Any): CarbonExpression = {
    -      new CarbonLiteralExpression(value,
    -        CarbonScalaUtil.convertSparkToCarbonDataType(dataTypeOf(name)))
    +      val dataTypeOfAttribute = CarbonScalaUtil.convertSparkToCarbonDataType(dataTypeOf(name))
    +      val dataType = if (Option(value).isDefined
    +                         && dataTypeOfAttribute == DataType.STRING
    +                         && value.isInstanceOf[Double]) {
    +        DataType.DOUBLE
    +      }
    +      else {
    --- End diff --
   
    take care codestyle


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [hidden email] or file a JIRA ticket
with INFRA.
---

[qiuchenjian-2]

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

    https://github.com/apache/incubator-carbondata/pull/697#discussion_r107918771
 
    --- Diff: integration/spark-common/src/main/scala/org/apache/carbondata/spark/CarbonFilters.scala ---
    @@ -111,16 +111,24 @@ object CarbonFilters {
         }
     
         def getCarbonLiteralExpression(name: String, value: Any): CarbonExpression = {
    -      new CarbonLiteralExpression(value,
    -        CarbonScalaUtil.convertSparkToCarbonDataType(dataTypeOf(name)))
    +      val dataTypeOfAttribute = CarbonScalaUtil.convertSparkToCarbonDataType(dataTypeOf(name))
    +      val dataType = if (Option(value).isDefined
    +                         && dataTypeOfAttribute == DataType.STRING
    +                         && value.isInstanceOf[Double]) {
    +        DataType.DOUBLE
    +      }
    +      else {
    +        dataTypeOfAttribute
    +      }
    +      new CarbonLiteralExpression(value, dataType)
         }
     
         createFilter(predicate)
       }
     
     
       // Check out which filters can be pushed down to carbon, remaining can be handled in spark layer.
    -  // Mostly dimension filters are only pushed down since it is faster in carbon.
    +  // Mostly dimension filters are only pushed down since it is faster in carbo  n.
    --- End diff --
   
    redundant blank


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [hidden email] or file a JIRA ticket
with INFRA.
---

[qiuchenjian-2]

Github user ravipesala commented on the issue:

    https://github.com/apache/incubator-carbondata/pull/697
 
    LGTM, except few style issues raised by david, please fix them


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [hidden email] or file a JIRA ticket
with INFRA.
---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

    https://github.com/apache/incubator-carbondata/pull/697
 
    Build Success with Spark 1.6.2, Please check CI http://136.243.101.176:8080/job/ApacheCarbonPRBuilder/1349/



---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [hidden email] or file a JIRA ticket
with INFRA.
---

[qiuchenjian-2]

Github user bhavya411 commented on the issue:

    https://github.com/apache/incubator-carbondata/pull/697
 
    Fixed the coding styles issues as well as the spacing issue


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [hidden email] or file a JIRA ticket
with INFRA.
---

[qiuchenjian-2]

Github user asfgit closed the pull request at:

    https://github.com/apache/incubator-carbondata/pull/697


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [hidden email] or file a JIRA ticket
with INFRA.
---