Apache CarbonData Dev Mailing List archive › Apache CarbonData JIRA issues

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

classic Classic list List threaded Threaded
♦
♦
 49 messages Options
123
qiuchenjian-2
Reply | Threaded
Open this post in threaded view
♦
♦
|

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

qiuchenjian-2
Github user chenliang613 commented on a diff in the pull request:

    https://github.com/apache/carbondata/pull/1257#discussion_r134460640
 
    --- Diff: integration/presto/src/main/java/org/apache/carbondata/presto/CarbondataRecordSetProvider.java ---
    @@ -73,27 +60,23 @@ public CarbondataRecordSetProvider(CarbondataConnectorId connectorId, CarbonTabl
     
         CarbondataSplit carbondataSplit =
             checkType(split, CarbondataSplit.class, "split is not class CarbondataSplit");
    -    checkArgument(carbondataSplit.getConnectorId().equals(connectorId), "split is not for this connector");
    +    checkArgument(carbondataSplit.getConnectorId().equals(connectorId),
    +        "split is not for this connector");
     
    -    StringBuffer targetColsBuffer = new StringBuffer();
         String targetCols = "";
         // Convert all columns handles
         ImmutableList.Builder<CarbondataColumnHandle> handles = ImmutableList.builder();
         for (ColumnHandle handle : columns) {
           handles.add(checkType(handle, CarbondataColumnHandle.class, "handle"));
    -      targetColsBuffer.append(((CarbondataColumnHandle) handle).getColumnName()).append(",");
    +      targetCols += ((CarbondataColumnHandle) handle).getColumnName() + ",";
    --- End diff --
   
    Please use StringBuffer to assemble "string"


---
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
Reply | Threaded
Open this post in threaded view
♦
♦
|

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

qiuchenjian-2
In reply to this post by qiuchenjian-2
Github user anubhav100 commented on a diff in the pull request:

    https://github.com/apache/carbondata/pull/1257#discussion_r134464233
 
    --- Diff: integration/presto/src/main/java/org/apache/carbondata/presto/CarbondataRecordSetProvider.java ---
    @@ -73,27 +60,23 @@ public CarbondataRecordSetProvider(CarbondataConnectorId connectorId, CarbonTabl
     
         CarbondataSplit carbondataSplit =
             checkType(split, CarbondataSplit.class, "split is not class CarbondataSplit");
    -    checkArgument(carbondataSplit.getConnectorId().equals(connectorId), "split is not for this connector");
    +    checkArgument(carbondataSplit.getConnectorId().equals(connectorId),
    +        "split is not for this connector");
     
    -    StringBuffer targetColsBuffer = new StringBuffer();
         String targetCols = "";
         // Convert all columns handles
         ImmutableList.Builder<CarbondataColumnHandle> handles = ImmutableList.builder();
         for (ColumnHandle handle : columns) {
           handles.add(checkType(handle, CarbondataColumnHandle.class, "handle"));
    -      targetColsBuffer.append(((CarbondataColumnHandle) handle).getColumnName()).append(",");
    +      targetCols += ((CarbondataColumnHandle) handle).getColumnName() + ",";
    --- End diff --
   
    @chenliang613 resolved


---
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
Reply | Threaded
Open this post in threaded view
♦
♦
|

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

qiuchenjian-2
In reply to this post by qiuchenjian-2
Github user chenliang613 commented on a diff in the pull request:

    https://github.com/apache/carbondata/pull/1257#discussion_r134691842
 
    --- Diff: integration/presto/src/main/java/org/apache/carbondata/presto/constants/PrestoCommonConstants.java ---
    @@ -0,0 +1,29 @@
    +/*
    + * 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.presto.constants;
    +
    +/**
    + * Constants for presto functionality
    + */
    --- End diff --
   
    Please don't add the new class : PrestoCommonConstants.java, keep to define the parameter in CarbondataPageSource.java as below:
    private static final int ROWS_PER_REQUEST = 4096;


---
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
Reply | Threaded
Open this post in threaded view
♦
♦
|

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

qiuchenjian-2
In reply to this post by qiuchenjian-2
Github user chenliang613 commented on a diff in the pull request:

    https://github.com/apache/carbondata/pull/1257#discussion_r134692412
 
    --- Diff: integration/presto/src/main/java/org/apache/carbondata/presto/CarbondataRecordSetProvider.java ---
    @@ -73,27 +60,23 @@ public CarbondataRecordSetProvider(CarbondataConnectorId connectorId, CarbonTabl
     
         CarbondataSplit carbondataSplit =
             checkType(split, CarbondataSplit.class, "split is not class CarbondataSplit");
    -    checkArgument(carbondataSplit.getConnectorId().equals(connectorId), "split is not for this connector");
    +    checkArgument(carbondataSplit.getConnectorId().equals(connectorId),
    +        "split is not for this connector");
     
    -    StringBuffer targetColsBuffer = new StringBuffer();
         String targetCols = "";
         // Convert all columns handles
         ImmutableList.Builder<CarbondataColumnHandle> handles = ImmutableList.builder();
         for (ColumnHandle handle : columns) {
           handles.add(checkType(handle, CarbondataColumnHandle.class, "handle"));
    -      targetColsBuffer.append(((CarbondataColumnHandle) handle).getColumnName()).append(",");
    +      targetCols += ((CarbondataColumnHandle) handle).getColumnName() + ",";
    --- End diff --
   
    i didn't see any code to resolve this comment.


---
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
Reply | Threaded
Open this post in threaded view
♦
♦
|

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

qiuchenjian-2
In reply to this post by qiuchenjian-2
Github user chenliang613 commented on a diff in the pull request:

    https://github.com/apache/carbondata/pull/1257#discussion_r134694501
 
    --- Diff: integration/presto/src/main/java/org/apache/carbondata/presto/CarbondataRecordSet.java ---
    @@ -79,30 +81,27 @@ private Expression parseConstraint2Expression(TupleDomain<ColumnHandle> constrai
       @Override public RecordCursor cursor() {
         List<TableBlockInfo> tableBlockInfoList = new ArrayList<TableBlockInfo>();
     
    -    tableBlockInfoList.add(new TableBlockInfo(split.getLocalInputSplit().getPath().toString(),
    +    tableBlockInfoList.add(new TableBlockInfo(split.getLocalInputSplit().getPath(),
             split.getLocalInputSplit().getStart(), split.getLocalInputSplit().getSegmentId(),
             split.getLocalInputSplit().getLocations().toArray(new String[0]),
             split.getLocalInputSplit().getLength(), new BlockletInfos(),
             //blockletInfos,
             ColumnarFormatVersion.valueOf(split.getLocalInputSplit().getVersion()), null));
    -    queryModel.setTableBlockInfos(tableBlockInfoList);
     
    +    queryModel.setTableBlockInfos(tableBlockInfoList);
         queryExecutor = QueryExecutorFactory.getQueryExecutor(queryModel);
     
    -    try {
    +    CarbonProperties.getInstance().addProperty("carbon.detail.batch.size",
    --- End diff --
   
    I can't find where code use this property(carbon.detail.batch.size)? don't know need to add property at here ?


---
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
Reply | Threaded
Open this post in threaded view
♦
♦
|

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

qiuchenjian-2
In reply to this post by qiuchenjian-2
Github user bhavya411 commented on a diff in the pull request:

    https://github.com/apache/carbondata/pull/1257#discussion_r134712614
 
    --- Diff: integration/presto/src/main/java/org/apache/carbondata/presto/CarbondataRecordSet.java ---
    @@ -79,30 +81,27 @@ private Expression parseConstraint2Expression(TupleDomain<ColumnHandle> constrai
       @Override public RecordCursor cursor() {
         List<TableBlockInfo> tableBlockInfoList = new ArrayList<TableBlockInfo>();
     
    -    tableBlockInfoList.add(new TableBlockInfo(split.getLocalInputSplit().getPath().toString(),
    +    tableBlockInfoList.add(new TableBlockInfo(split.getLocalInputSplit().getPath(),
             split.getLocalInputSplit().getStart(), split.getLocalInputSplit().getSegmentId(),
             split.getLocalInputSplit().getLocations().toArray(new String[0]),
             split.getLocalInputSplit().getLength(), new BlockletInfos(),
             //blockletInfos,
             ColumnarFormatVersion.valueOf(split.getLocalInputSplit().getVersion()), null));
    -    queryModel.setTableBlockInfos(tableBlockInfoList);
     
    +    queryModel.setTableBlockInfos(tableBlockInfoList);
         queryExecutor = QueryExecutorFactory.getQueryExecutor(queryModel);
     
    -    try {
    +    CarbonProperties.getInstance().addProperty("carbon.detail.batch.size",
    --- End diff --
   
    Actually in DataBlockIteratoImpl we are using this property to create the batches, we are setting this property here to ensure that the batchsize of Presto and carbondata match so we have better efficiency


---
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
Reply | Threaded
Open this post in threaded view
♦
♦
|

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

qiuchenjian-2
In reply to this post by qiuchenjian-2
Github user anubhav100 commented on a diff in the pull request:

    https://github.com/apache/carbondata/pull/1257#discussion_r134779856
 
    --- Diff: integration/presto/src/main/java/org/apache/carbondata/presto/CarbondataRecordSetProvider.java ---
    @@ -73,27 +60,23 @@ public CarbondataRecordSetProvider(CarbondataConnectorId connectorId, CarbonTabl
     
         CarbondataSplit carbondataSplit =
             checkType(split, CarbondataSplit.class, "split is not class CarbondataSplit");
    -    checkArgument(carbondataSplit.getConnectorId().equals(connectorId), "split is not for this connector");
    +    checkArgument(carbondataSplit.getConnectorId().equals(connectorId),
    +        "split is not for this connector");
     
    -    StringBuffer targetColsBuffer = new StringBuffer();
         String targetCols = "";
         // Convert all columns handles
         ImmutableList.Builder<CarbondataColumnHandle> handles = ImmutableList.builder();
         for (ColumnHandle handle : columns) {
           handles.add(checkType(handle, CarbondataColumnHandle.class, "handle"));
    -      targetColsBuffer.append(((CarbondataColumnHandle) handle).getColumnName()).append(",");
    +      targetCols += ((CarbondataColumnHandle) handle).getColumnName() + ",";
    --- End diff --
   
    Actually last commit was not rebased with my previous one I will  correct it again


---
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
Reply | Threaded
Open this post in threaded view
♦
♦
|

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

qiuchenjian-2
In reply to this post by qiuchenjian-2
Github user anubhav100 commented on a diff in the pull request:

    https://github.com/apache/carbondata/pull/1257#discussion_r134780421
 
    --- Diff: integration/presto/src/main/java/org/apache/carbondata/presto/constants/PrestoCommonConstants.java ---
    @@ -0,0 +1,29 @@
    +/*
    + * 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.presto.constants;
    +
    +/**
    + * Constants for presto functionality
    + */
    --- End diff --
   
    Ok


---
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
Reply | Threaded
Open this post in threaded view
♦
♦
|

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

qiuchenjian-2
In reply to this post by qiuchenjian-2
Github user anubhav100 closed the pull request at:

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


---
123
Free forum by Nabble Edit this page