[GitHub] carbondata pull request #2334: [CARBONDATA-2515][CARBONDATA-2516] fixed Time...

[qiuchenjian-2]

GitHub user sv71294 opened a pull request:

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

    [CARBONDATA-2515][CARBONDATA-2516] fixed Timestamp greaterthan expression and OR filter Expression issue

    **Problem 1 [CARBONDATA-2515]:** OR expression filters not handled in PrestoFilterUtil
   
    **Problem 2 [CARBONDATA-2516]:** Full blocklet scanning for Timestamp Greater than expression as it is not generating in PrestoFilterUtil.
   
    **Solution**: added functionality for OR expression in PrestoFilterUtil and relevant code fixes for the timestamp issue.

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

    $ git pull https://github.com/sv71294/carbondata carbon-presto-filterExpression-fixes

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

    https://github.com/apache/carbondata/pull/2334.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 #2334
   
----
commit 355f41cf78ee337c224ff44c0ebe0b92c1672b16
Author: sv71294 <sv71294@...>
Date:   2018-05-22T11:35:18Z

    carbon presto integraion timestamp greaterthan expression and OR filter expression fixed

----


---

[qiuchenjian-2]

Github user ravipesala commented on the issue:

    https://github.com/apache/carbondata/pull/2334
 
    Can one of the admins verify this patch?


---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

    https://github.com/apache/carbondata/pull/2334
 
    Can one of the admins verify this patch?


---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

    https://github.com/apache/carbondata/pull/2334
 
    Can one of the admins verify this patch?


---

[qiuchenjian-2]

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

    https://github.com/apache/carbondata/pull/2334#discussion_r190103764
 
    --- Diff: integration/presto/src/main/java/org/apache/carbondata/presto/PrestoFilterUtil.java ---
    @@ -17,51 +17,28 @@
     
     package org.apache.carbondata.presto;
     
    -import java.math.BigDecimal;
    -import java.math.BigInteger;
    -import java.sql.Timestamp;
    -import java.util.ArrayList;
    -import java.util.Calendar;
    -import java.util.Date;
    -import java.util.HashMap;
    -import java.util.List;
    -import java.util.Map;
    -
    +import com.facebook.presto.spi.ColumnHandle;
    +import com.facebook.presto.spi.PrestoException;
    +import com.facebook.presto.spi.predicate.Domain;
    +import com.facebook.presto.spi.predicate.Range;
    +import com.facebook.presto.spi.predicate.TupleDomain;
    +import com.facebook.presto.spi.type.*;
    +import io.airlift.slice.Slice;
     import org.apache.carbondata.core.metadata.datatype.DataType;
     import org.apache.carbondata.core.metadata.datatype.DataTypes;
     import org.apache.carbondata.core.metadata.schema.table.CarbonTable;
     import org.apache.carbondata.core.metadata.schema.table.column.ColumnSchema;
     import org.apache.carbondata.core.scan.expression.ColumnExpression;
     import org.apache.carbondata.core.scan.expression.Expression;
     import org.apache.carbondata.core.scan.expression.LiteralExpression;
    -import org.apache.carbondata.core.scan.expression.conditional.EqualToExpression;
    -import org.apache.carbondata.core.scan.expression.conditional.GreaterThanEqualToExpression;
    -import org.apache.carbondata.core.scan.expression.conditional.GreaterThanExpression;
    -import org.apache.carbondata.core.scan.expression.conditional.InExpression;
    -import org.apache.carbondata.core.scan.expression.conditional.LessThanEqualToExpression;
    -import org.apache.carbondata.core.scan.expression.conditional.LessThanExpression;
    -import org.apache.carbondata.core.scan.expression.conditional.ListExpression;
    +import org.apache.carbondata.core.scan.expression.conditional.*;
     import org.apache.carbondata.core.scan.expression.logical.AndExpression;
     import org.apache.carbondata.core.scan.expression.logical.OrExpression;
     
    -import com.facebook.presto.spi.ColumnHandle;
    -import com.facebook.presto.spi.PrestoException;
    -import com.facebook.presto.spi.predicate.Domain;
    -import com.facebook.presto.spi.predicate.Range;
    -import com.facebook.presto.spi.predicate.TupleDomain;
    -import com.facebook.presto.spi.type.BigintType;
    -import com.facebook.presto.spi.type.BooleanType;
    -import com.facebook.presto.spi.type.DateType;
    -import com.facebook.presto.spi.type.DecimalType;
    -import com.facebook.presto.spi.type.Decimals;
    -import com.facebook.presto.spi.type.DoubleType;
    -import com.facebook.presto.spi.type.IntegerType;
    -import com.facebook.presto.spi.type.SmallintType;
    -import com.facebook.presto.spi.type.TimestampType;
    -import com.facebook.presto.spi.type.Type;
    -import com.facebook.presto.spi.type.VarcharType;
    -import com.google.common.collect.ImmutableList;
    -import io.airlift.slice.Slice;
    +import java.math.BigDecimal;
    +import java.math.BigInteger;
    +import java.sql.Timestamp;
    +import java.util.*;
    --- End diff --
   
    Please keep the detail class, not *


---

[qiuchenjian-2]

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

    https://github.com/apache/carbondata/pull/2334#discussion_r190103900
 
    --- Diff: integration/presto/src/main/java/org/apache/carbondata/presto/PrestoFilterUtil.java ---
    @@ -17,51 +17,28 @@
     
     package org.apache.carbondata.presto;
     
    -import java.math.BigDecimal;
    -import java.math.BigInteger;
    -import java.sql.Timestamp;
    -import java.util.ArrayList;
    -import java.util.Calendar;
    -import java.util.Date;
    -import java.util.HashMap;
    -import java.util.List;
    -import java.util.Map;
    -
    +import com.facebook.presto.spi.ColumnHandle;
    +import com.facebook.presto.spi.PrestoException;
    +import com.facebook.presto.spi.predicate.Domain;
    +import com.facebook.presto.spi.predicate.Range;
    +import com.facebook.presto.spi.predicate.TupleDomain;
    +import com.facebook.presto.spi.type.*;
    +import io.airlift.slice.Slice;
     import org.apache.carbondata.core.metadata.datatype.DataType;
     import org.apache.carbondata.core.metadata.datatype.DataTypes;
     import org.apache.carbondata.core.metadata.schema.table.CarbonTable;
     import org.apache.carbondata.core.metadata.schema.table.column.ColumnSchema;
     import org.apache.carbondata.core.scan.expression.ColumnExpression;
     import org.apache.carbondata.core.scan.expression.Expression;
     import org.apache.carbondata.core.scan.expression.LiteralExpression;
    -import org.apache.carbondata.core.scan.expression.conditional.EqualToExpression;
    -import org.apache.carbondata.core.scan.expression.conditional.GreaterThanEqualToExpression;
    -import org.apache.carbondata.core.scan.expression.conditional.GreaterThanExpression;
    -import org.apache.carbondata.core.scan.expression.conditional.InExpression;
    -import org.apache.carbondata.core.scan.expression.conditional.LessThanEqualToExpression;
    -import org.apache.carbondata.core.scan.expression.conditional.LessThanExpression;
    -import org.apache.carbondata.core.scan.expression.conditional.ListExpression;
    +import org.apache.carbondata.core.scan.expression.conditional.*;
     import org.apache.carbondata.core.scan.expression.logical.AndExpression;
     import org.apache.carbondata.core.scan.expression.logical.OrExpression;
     
    -import com.facebook.presto.spi.ColumnHandle;
    -import com.facebook.presto.spi.PrestoException;
    -import com.facebook.presto.spi.predicate.Domain;
    -import com.facebook.presto.spi.predicate.Range;
    -import com.facebook.presto.spi.predicate.TupleDomain;
    -import com.facebook.presto.spi.type.BigintType;
    -import com.facebook.presto.spi.type.BooleanType;
    -import com.facebook.presto.spi.type.DateType;
    -import com.facebook.presto.spi.type.DecimalType;
    -import com.facebook.presto.spi.type.Decimals;
    -import com.facebook.presto.spi.type.DoubleType;
    -import com.facebook.presto.spi.type.IntegerType;
    -import com.facebook.presto.spi.type.SmallintType;
    -import com.facebook.presto.spi.type.TimestampType;
    -import com.facebook.presto.spi.type.Type;
    -import com.facebook.presto.spi.type.VarcharType;
    -import com.google.common.collect.ImmutableList;
    -import io.airlift.slice.Slice;
    +import java.math.BigDecimal;
    --- End diff --
   
    please optimize the order of class, unify with others


---

[qiuchenjian-2]

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

    https://github.com/apache/carbondata/pull/2334#discussion_r190103982
 
    --- Diff: integration/presto/src/main/java/org/apache/carbondata/presto/PrestoFilterUtil.java ---
    @@ -92,14 +69,15 @@ private static DataType Spi2CarbondataTypeMapper(CarbondataColumnHandle carbonda
         else if (colType == DateType.DATE) return DataTypes.DATE;
         else if (colType == TimestampType.TIMESTAMP) return DataTypes.TIMESTAMP;
         else if (colType.equals(DecimalType.createDecimalType(carbondataColumnHandle.getPrecision(),
    -        carbondataColumnHandle.getScale()))) return DataTypes
    -        .createDecimalType(carbondataColumnHandle.getPrecision(),
    -            carbondataColumnHandle.getScale());
    +            carbondataColumnHandle.getScale()))) return DataTypes
    --- End diff --
   
    Why change this one?


---

[qiuchenjian-2]

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

    https://github.com/apache/carbondata/pull/2334#discussion_r190104019
 
    --- Diff: integration/presto/src/main/java/org/apache/carbondata/presto/PrestoFilterUtil.java ---
    @@ -110,28 +88,30 @@ else if (colType.equals(DecimalType.createDecimalType(carbondataColumnHandle.get
         for (ColumnHandle columnHandle : originalConstraint.getDomains().get().keySet()) {
           CarbondataColumnHandle carbondataColumnHandle = (CarbondataColumnHandle) columnHandle;
           List<ColumnSchema> partitionedColumnSchema = columnSchemas.stream().filter(
    -          columnSchema -> carbondataColumnHandle.getColumnName().equals(columnSchema.getColumnName())).collect(toList());
    -      if(partitionedColumnSchema.size() != 0) {
    +              columnSchema -> carbondataColumnHandle.getColumnName().equals(columnSchema.getColumnName())).collect(toList());
    --- End diff --
   
    Why change this one?


---

[qiuchenjian-2]

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

    https://github.com/apache/carbondata/pull/2334#discussion_r190104064
 
    --- Diff: integration/presto/src/main/java/org/apache/carbondata/presto/PrestoFilterUtil.java ---
    @@ -110,28 +88,30 @@ else if (colType.equals(DecimalType.createDecimalType(carbondataColumnHandle.get
         for (ColumnHandle columnHandle : originalConstraint.getDomains().get().keySet()) {
           CarbondataColumnHandle carbondataColumnHandle = (CarbondataColumnHandle) columnHandle;
           List<ColumnSchema> partitionedColumnSchema = columnSchemas.stream().filter(
    -          columnSchema -> carbondataColumnHandle.getColumnName().equals(columnSchema.getColumnName())).collect(toList());
    -      if(partitionedColumnSchema.size() != 0) {
    +              columnSchema -> carbondataColumnHandle.getColumnName().equals(columnSchema.getColumnName())).collect(toList());
    +      if (partitionedColumnSchema.size() != 0) {
             filter.addAll(createPartitionFilters(originalConstraint, carbondataColumnHandle));
           }
         }
         return filter;
       }
     
    -  /** Returns list of partition key and values using domain constraints
    +  /**
    +   * Returns list of partition key and values using domain constraints
    +   *
        * @param originalConstraint
        * @param carbonDataColumnHandle
        */
       private static List<String> createPartitionFilters(TupleDomain<ColumnHandle> originalConstraint,
    -      CarbondataColumnHandle carbonDataColumnHandle) {
    +                                                     CarbondataColumnHandle carbonDataColumnHandle) {
    --- End diff --
   
    Why change this one?


---

[qiuchenjian-2]

Github user xubo245 commented on the issue:

    https://github.com/apache/carbondata/pull/2334
 
    Please optimize the code style of this PR


---

[qiuchenjian-2]

Github user sv71294 commented on the issue:

    https://github.com/apache/carbondata/pull/2334
 
    Please review it now


---

[qiuchenjian-2]

Github user jackylk commented on the issue:

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


---

[qiuchenjian-2]

Github user jackylk commented on the issue:

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


---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

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



---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

    https://github.com/apache/carbondata/pull/2334
 
    Build Failed  with Spark 2.1.0, Please check CI http://136.243.101.176:8080/job/ApacheCarbonPRBuilder1/6146/



---

[qiuchenjian-2]

Github user sv71294 commented on the issue:

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


---

[qiuchenjian-2]

Github user xubo245 commented on the issue:

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


---

[qiuchenjian-2]

Github user chenliang613 commented on the issue:

    https://github.com/apache/carbondata/pull/2334
 
    add to whitelist


---

[qiuchenjian-2]

Github user chenliang613 commented on the issue:

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


---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

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



---