[GitHub] carbondata pull request #1257: [CARBONDATA-1347] Implemented Columnar Readin...

[qiuchenjian-2]

GitHub user anubhav100 opened a pull request:

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

    [CARBONDATA-1347] Implemented Columnar Reading Of Data For Presto

    This Pr Include Following Features :
   
     - implemented columanr based iterator and executor for reading columns instead of rows
     - create a new improved dictionarydecoder with scala
     -added the lazy blocks
     - added the stream readers in presto
     - removed the redundant execution of parsing filters using map
    -removed the useless code and redudant intilizations
     -fixed the following bugs
            - Wrong values of decimal types in tpch queries
            - class cast exception in intermediate file merger
            - refactor the carbon data comprator
             
   


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

    $ git pull https://github.com/anubhav100/incubator-carbondata feature/CARBONDATA-1347

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

    https://github.com/apache/carbondata/pull/1257.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 #1257
   
----
commit 8ebe591a19fe40e438cfc35fcd91a9b4d1ba8552
Author: anubhav100 <[hidden email]>
Date:   2017-08-10T09:42:22Z

    wip for implemented columnar reading of data for presto

commit e50ef7e1e4f0ca215ed9f6b0b6095fb6d037e5e6
Author: anubhav100 <[hidden email]>
Date:   2017-08-14T09:31:58Z

    corrected the decimal type

commit b5ce0fe7b56b27750b6d0392eff710331a2fedb4
Author: anubhav100 <[hidden email]>
Date:   2017-08-14T13:05:37Z

    resolved minor bugs in core

----


---
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 anubhav100 commented on the issue:

    https://github.com/apache/carbondata/pull/1257
 
    @chenliang613 please review this pr it contains all the changes


---
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 anubhav100 commented on the issue:

    https://github.com/apache/carbondata/pull/1257
 
    Ok to test


---
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/carbondata/pull/1257
 
    Build Failed  with Spark 2.1.0, Please check CI http://136.243.101.176:8080/job/ApacheCarbonPRBuilder/3418/



---
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 chenliang613 commented on the issue:

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


---
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 chenliang613 commented on a diff in the pull request:

    https://github.com/apache/carbondata/pull/1257#discussion_r133370557
 
    --- Diff: core/src/main/java/org/apache/carbondata/core/scan/expression/ExpressionResult.java ---
    @@ -183,7 +183,7 @@ public String getString() throws FilterIllegalMemberException {
                 return parser.format((java.sql.Date) value);
               } else if (value instanceof Long) {
                 if (isLiteral) {
    -              return parser.format(new Timestamp((long) value / 1000));
    +              return parser.format(new Timestamp((long) value));
    --- End diff --
   
    Why need do this change (remove /1000) for getString() ?


---
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 anubhav100 commented on a diff in the pull request:

    https://github.com/apache/carbondata/pull/1257#discussion_r133371582
 
    --- Diff: core/src/main/java/org/apache/carbondata/core/scan/expression/ExpressionResult.java ---
    @@ -183,7 +183,7 @@ public String getString() throws FilterIllegalMemberException {
                 return parser.format((java.sql.Date) value);
               } else if (value instanceof Long) {
                 if (isLiteral) {
    -              return parser.format(new Timestamp((long) value / 1000));
    +              return parser.format(new Timestamp((long) value));
    --- End diff --
   
    @chenliang613 earlier time stamp was getting multiplied by 1000 in carbondata so thats why it was wriiten as
    -return parser.format(new Timestamp((long) value / 1000));
    but now it is not getting multipled by 1000 so at time of building expression result so now there is no need of dividing timestamp by 1000



---
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 chenliang613 commented on a diff in the pull request:

    https://github.com/apache/carbondata/pull/1257#discussion_r133372855
 
    --- Diff: integration/presto/src/main/java/org/apache/carbondata/presto/constants/CarbonCommonConstants.java ---
    @@ -0,0 +1,1319 @@
    +/*
    --- End diff --
   
    please only keep these CommonConstants which be used by presto module, don't need to copy all.


---
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 chenliang613 commented on a diff in the pull request:

    https://github.com/apache/carbondata/pull/1257#discussion_r133375740
 
    --- Diff: integration/presto/src/main/java/org/apache/carbondata/presto/memory/AbstractAggregatedMemoryContext.java ---
    @@ -0,0 +1,37 @@
    +/*
    + * Licensed 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
    + *
    --- End diff --
   
    The license header is wrong.


---
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 chenliang613 commented on a diff in the pull request:

    https://github.com/apache/carbondata/pull/1257#discussion_r133375796
 
    --- Diff: integration/presto/src/main/java/org/apache/carbondata/presto/memory/AggregatedMemoryContext.java ---
    @@ -0,0 +1,62 @@
    +/*
    + * Licensed 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
    --- End diff --
   
    The license header is wrong.


---
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 chenliang613 commented on a diff in the pull request:

    https://github.com/apache/carbondata/pull/1257#discussion_r133375914
 
    --- Diff: integration/presto/src/main/java/org/apache/carbondata/presto/memory/LocalMemoryContext.java ---
    @@ -0,0 +1,40 @@
    +/*
    + * Licensed 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.
    --- End diff --
   
    The license header is wrong.


---
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 chenliang613 commented on a diff in the pull request:

    https://github.com/apache/carbondata/pull/1257#discussion_r133376989
 
    --- Diff: integration/presto/src/main/java/org/apache/carbondata/presto/readers/BooleanStreamReader.java ---
    @@ -0,0 +1,31 @@
    +package org.apache.carbondata.presto.readers;
    --- End diff --
   
    The license header is missing.


---
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 chenliang613 commented on a diff in the pull request:

    https://github.com/apache/carbondata/pull/1257#discussion_r133377118
 
    --- Diff: integration/presto/src/main/java/org/apache/carbondata/presto/readers/BooleanStreamReader.java ---
    @@ -0,0 +1,31 @@
    +package org.apache.carbondata.presto.readers;
    +
    +import java.io.IOException;
    +
    +import com.facebook.presto.spi.block.Block;
    +import com.facebook.presto.spi.block.BlockBuilder;
    +import com.facebook.presto.spi.block.BlockBuilderStatus;
    +import com.facebook.presto.spi.type.Type;
    +
    +public class BooleanStreamReader implements StreamReader {
    --- End diff --
   
    Can you explain why add the class BooleanStreamReader , it is for which purpose?


---
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 chenliang613 commented on a diff in the pull request:

    https://github.com/apache/carbondata/pull/1257#discussion_r133377209
 
    --- Diff: integration/presto/src/main/java/org/apache/carbondata/presto/readers/DecimalSliceStreamReader.java ---
    @@ -0,0 +1,103 @@
    +package org.apache.carbondata.presto.readers;
    --- End diff --
   
    license header is missing


---
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 chenliang613 commented on a diff in the pull request:

    https://github.com/apache/carbondata/pull/1257#discussion_r133377510
 
    --- Diff: integration/presto/src/main/java/org/apache/carbondata/presto/readers/DecimalSliceStreamReader.java ---
    @@ -0,0 +1,103 @@
    +package org.apache.carbondata.presto.readers;
    +
    +import java.io.IOException;
    +import java.math.BigDecimal;
    +import java.math.BigInteger;
    +
    +import com.facebook.presto.spi.block.Block;
    +import com.facebook.presto.spi.block.BlockBuilder;
    +import com.facebook.presto.spi.block.BlockBuilderStatus;
    +import com.facebook.presto.spi.type.DecimalType;
    +import com.facebook.presto.spi.type.Decimals;
    +import com.facebook.presto.spi.type.Type;
    +import io.airlift.slice.Slice;
    +
    +import static com.facebook.presto.spi.type.Decimals.encodeUnscaledValue;
    +import static com.facebook.presto.spi.type.Decimals.isShortDecimal;
    +import static com.facebook.presto.spi.type.Decimals.rescale;
    +import static com.google.common.base.Preconditions.checkArgument;
    +import static com.google.common.base.Preconditions.checkState;
    +import static io.airlift.slice.Slices.utf8Slice;
    +import static java.math.RoundingMode.HALF_UP;
    +
    +public class DecimalSliceStreamReader implements StreamReader {
    --- End diff --
   
    This pr is optimizing for column reader, why need to add StreamReader ?  please consider using different PR to implement different features.


---
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 anubhav100 commented on a diff in the pull request:

    https://github.com/apache/carbondata/pull/1257#discussion_r133407524
 
    --- Diff: integration/presto/src/main/java/org/apache/carbondata/presto/constants/CarbonCommonConstants.java ---
    @@ -0,0 +1,1319 @@
    +/*
    --- End diff --
   
    done


---
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/carbondata/pull/1257
 
    SDV Build Failed with Spark 2.1, Please check CI http://144.76.159.231:8080/job/ApacheSDVTests/197/



---
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 chenliang613 commented on the issue:

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


---
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/carbondata/pull/1257
 
    SDV Build Success with Spark 2.1, Please check CI http://144.76.159.231:8080/job/ApacheSDVTests/217/



---
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 chenliang613 commented on the issue:

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


---
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.
---