[GitHub] carbondata pull request #1581: [CARBONDATA-1779] GenericVectorizedReader

[asfgit]

Github user asfgit closed the pull request at:

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


---

[GitHub] carbondata pull request #1581: [CARBONDATA-1779] GenericVectorizedReader

[chenliang613]

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

https://github.com/apache/carbondata/pull/1581#discussion_r156553976
  
--- Diff: 
core/src/main/java/org/apache/carbondata/core/scan/collector/impl/RestructureBasedVectorResultCollector.java
 ---
@@ -238,7 +238,7 @@ private void fillDataForNonExistingMeasures() {
 (long) defaultValue);
   } else if (DataTypes.isDecimal(dataType)) {
 vector.putDecimals(columnVectorInfo.vectorOffset, 
columnVectorInfo.size,
-(Decimal) defaultValue, measure.getPrecision());
+((Decimal) defaultValue).toJavaBigDecimal(), 
measure.getPrecision());
--- End diff --

ok, this is core module.


---

[GitHub] carbondata pull request #1581: [CARBONDATA-1779] GenericVectorizedReader

[bhavya411]

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

https://github.com/apache/carbondata/pull/1581#discussion_r156023011
  
--- Diff: 
core/src/main/java/org/apache/carbondata/core/scan/collector/impl/RestructureBasedVectorResultCollector.java
 ---
@@ -238,7 +238,7 @@ private void fillDataForNonExistingMeasures() {
 (long) defaultValue);
   } else if (DataTypes.isDecimal(dataType)) {
 vector.putDecimals(columnVectorInfo.vectorOffset, 
columnVectorInfo.size,
-(Decimal) defaultValue, measure.getPrecision());
+((Decimal) defaultValue).toJavaBigDecimal(), 
measure.getPrecision());
--- End diff --

We can not remove these imports as the data that is  returned is stored in 
Spark Format , the casting is done to convert it to generic types


---

[GitHub] carbondata pull request #1581: [CARBONDATA-1779] GenericVectorizedReader

[chenliang613]

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

https://github.com/apache/carbondata/pull/1581#discussion_r156017061
  
--- Diff: 
integration/presto/src/main/java/org/apache/carbondata/presto/CarbondataConnector.java
 ---
@@ -31,13 +31,13 @@
   private static final Logger log = Logger.get(CarbondataConnector.class);
 
   private final LifeCycleManager lifeCycleManager;
-  private final CarbondataMetadata metadata;
+  private final ConnectorMetadata metadata;
--- End diff --

ok


---

[GitHub] carbondata pull request #1581: [CARBONDATA-1779] GenericVectorizedReader

[bhavya411]

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

https://github.com/apache/carbondata/pull/1581#discussion_r155492390
  
--- Diff: 
integration/presto/src/main/java/org/apache/carbondata/presto/CarbondataConnector.java
 ---
@@ -31,13 +31,13 @@
   private static final Logger log = Logger.get(CarbondataConnector.class);
 
   private final LifeCycleManager lifeCycleManager;
-  private final CarbondataMetadata metadata;
+  private final ConnectorMetadata metadata;
--- End diff --

CarbonDataMetadata extends ConnectorMetaData, we should always code to 
Interface hence have changed this, also this way in concurrency  we are using 
the standard Presto protocol and providing a ClassLoaderSafeConnectorMetadata


---

[GitHub] carbondata pull request #1581: [CARBONDATA-1779] GenericVectorizedReader

[chenliang613]

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

https://github.com/apache/carbondata/pull/1581#discussion_r155440197
  
--- Diff: 
integration/presto/src/main/java/org/apache/carbondata/presto/impl/CarbonTableReader.java
 ---
@@ -95,12 +99,12 @@
* A cache for Carbon reader, with this cache,
* metadata of a table is only read from file system once.
*/
-  private ConcurrentHashMap cc;
+  private AtomicReference> 
cc;
--- End diff --

the name "cc" also need to change.


---

[GitHub] carbondata pull request #1581: [CARBONDATA-1779] GenericVectorizedReader

[chenliang613]

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

https://github.com/apache/carbondata/pull/1581#discussion_r155439723
  
--- Diff: 
integration/presto/src/main/java/org/apache/carbondata/presto/CarbondataConnector.java
 ---
@@ -31,13 +31,13 @@
   private static final Logger log = Logger.get(CarbondataConnector.class);
 
   private final LifeCycleManager lifeCycleManager;
-  private final CarbondataMetadata metadata;
+  private final ConnectorMetadata metadata;
--- End diff --

what are the difference of using CarbonDataMetadata or ConnectorMetadata?


---

[GitHub] carbondata pull request #1581: [CARBONDATA-1779] GenericVectorizedReader

[bhavya411]

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

https://github.com/apache/carbondata/pull/1581#discussion_r155438974
  
--- Diff: 
integration/presto/src/main/java/org/apache/carbondata/presto/CarbonVectorBatch.java
 ---
@@ -0,0 +1,112 @@
+package org.apache.carbondata.presto;
+
+import java.util.Arrays;
+import java.util.HashSet;
+import java.util.Set;
+
+public class CarbonVectorBatch {
+
+  private static final int DEFAULT_BATCH_SIZE = 1024;
--- End diff --

The batchSize is the size of the complete batch so for every Column we have 
to set it as byteSize , it is the number of records in the batch so it has to 
be equivalent to the batch size


---

[GitHub] carbondata pull request #1581: [CARBONDATA-1779] GenericVectorizedReader

[bhavya411]

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

https://github.com/apache/carbondata/pull/1581#discussion_r155435994
  
--- Diff: integration/presto/pom.xml ---
@@ -431,14 +435,27 @@
 
 
   org.apache.spark
-  spark-sql_2.11
+  spark-network-common_2.11
--- End diff --

Yes it is used only in Tests, we have defined the scope as test for all 
these dependencies


---

[GitHub] carbondata pull request #1581: [CARBONDATA-1779] GenericVectorizedReader

[chenliang613]

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

https://github.com/apache/carbondata/pull/1581#discussion_r155219726
  
--- Diff: 
integration/presto/src/main/java/org/apache/carbondata/presto/CarbonVectorBatch.java
 ---
@@ -0,0 +1,112 @@
+package org.apache.carbondata.presto;
--- End diff --

miss license header


---

[GitHub] carbondata pull request #1581: [CARBONDATA-1779] GenericVectorizedReader

[chenliang613]

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

https://github.com/apache/carbondata/pull/1581#discussion_r155218744
  
--- Diff: 
integration/presto/src/main/java/org/apache/carbondata/presto/CarbonColumnVectorImpl.java
 ---
@@ -0,0 +1,249 @@
+/*
+ * 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
--- End diff --

Please keep the license header consistent with other class



---

[GitHub] carbondata pull request #1581: [CARBONDATA-1779] GenericVectorizedReader

[chenliang613]

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

https://github.com/apache/carbondata/pull/1581#discussion_r155217593
  
--- Diff: integration/presto/pom.xml ---
@@ -431,14 +435,27 @@
 
 
   org.apache.spark
-  spark-sql_2.11
+  spark-network-common_2.11
--- End diff --

ok, is it only for test cases , right ?


---

[GitHub] carbondata pull request #1581: [CARBONDATA-1779] GenericVectorizedReader

[bhavya411]

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

https://github.com/apache/carbondata/pull/1581#discussion_r155185364
  
--- Diff: integration/presto/pom.xml ---
@@ -431,14 +435,27 @@
 
 
   org.apache.spark
-  spark-sql_2.11
+  spark-network-common_2.11
--- End diff --

For Running the Integration Tests as when we create a CarbonStore we need 
the Spark Dependency, so what we have done is that we have kept the scope of 
those dependency as Test , no spark Jar will be part of the final Jar that we 
copy to presto plugin


---

[GitHub] carbondata pull request #1581: [CARBONDATA-1779] GenericVectorizedReader

[chenliang613]

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

https://github.com/apache/carbondata/pull/1581#discussion_r155179202
  
--- Diff: integration/presto/pom.xml ---
@@ -431,14 +435,27 @@
 
 
   org.apache.spark
-  spark-sql_2.11
+  spark-network-common_2.11
--- End diff --

Even though we remove all spark dependency in presto integration module, 
why here still need spark dependency in pom.xml of presto?


---

[GitHub] carbondata pull request #1581: [CARBONDATA-1779] GenericVectorizedReader

[chenliang613]

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

https://github.com/apache/carbondata/pull/1581#discussion_r155178064
  
--- Diff: integration/presto/pom.xml ---
@@ -31,7 +31,7 @@
   presto-plugin
 
   
-0.186
+0.187
--- End diff --

for 1.3.0, we have to finalize the presto version. 


---

[GitHub] carbondata pull request #1581: [CARBONDATA-1779] GenericVectorizedReader

[chenliang613]

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

https://github.com/apache/carbondata/pull/1581#discussion_r155176489
  
--- Diff: 
core/src/main/java/org/apache/carbondata/core/scan/collector/impl/RestructureBasedVectorResultCollector.java
 ---
@@ -238,7 +238,7 @@ private void fillDataForNonExistingMeasures() {
 (long) defaultValue);
   } else if (DataTypes.isDecimal(dataType)) {
 vector.putDecimals(columnVectorInfo.vectorOffset, 
columnVectorInfo.size,
-(Decimal) defaultValue, measure.getPrecision());
+((Decimal) defaultValue).toJavaBigDecimal(), 
measure.getPrecision());
--- End diff --

please remove these import : 
 import org.apache.spark.sql.types.Decimal;
 import org.apache.spark.unsafe.types.UTF8String;   


---

[GitHub] carbondata pull request #1581: [CARBONDATA-1779] GenericVectorizedReader

[bhavya411]

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

https://github.com/apache/carbondata/pull/1581#discussion_r154860147
  
--- Diff: integration/presto/pom.xml ---
@@ -31,7 +31,7 @@
   presto-plugin
 
   
-0.186
+0.187
--- End diff --

Yes I think we need to specify 0.187 version upgrade in another mailing 
list, with 0.187 in carbon it works with 0.186 but not with 0.166 as the 
interfaces for Connector is changed in 0.186


---

[GitHub] carbondata pull request #1581: [CARBONDATA-1779] GenericVectorizedReader

[ravipesala]

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

https://github.com/apache/carbondata/pull/1581#discussion_r154838692
  
--- Diff: 
integration/presto/src/main/java/org/apache/carbondata/presto/CarbonColumnVectorImpl.java
 ---
@@ -0,0 +1,249 @@
+/*
+ * 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;
+
+import java.math.BigDecimal;
+import java.util.Arrays;
+import java.util.BitSet;
+
+import org.apache.carbondata.core.metadata.datatype.DataType;
+import org.apache.carbondata.core.metadata.datatype.DataTypes;
+import org.apache.carbondata.core.metadata.datatype.DecimalType;
+import org.apache.carbondata.core.scan.result.vector.CarbonColumnVector;
+
+
+
+public class CarbonColumnVectorImpl implements CarbonColumnVector {
--- End diff --

I think you can keep this class in core package, hive also can use it in 
future


---

[GitHub] carbondata pull request #1581: [CARBONDATA-1779] GenericVectorizedReader

[ravipesala]

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

https://github.com/apache/carbondata/pull/1581#discussion_r154838503
  
--- Diff: 
integration/spark2/src/main/java/org/apache/carbondata/spark/vectorreader/ColumnarVectorWrapper.java
 ---
@@ -200,7 +205,7 @@ public ColumnarVectorWrapper(ColumnVector columnVector, 
boolean[] filteredRows)
   }
 
   @Override public DataType getType() {
-return columnVector.dataType();
+return 
CarbonScalaUtil.convertSparkToCarbonDataType(columnVector.dataType());
--- End diff --

store the dataType as class variable, don't convert each time.


---

[GitHub] carbondata pull request #1581: [CARBONDATA-1779] GenericVectorizedReader

[ravipesala]

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

https://github.com/apache/carbondata/pull/1581#discussion_r154838347
  
--- Diff: 
integration/presto/src/main/java/org/apache/carbondata/presto/StructField.java 
---
@@ -0,0 +1,51 @@
+/*
+ * 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;
+
+import org.apache.carbondata.core.metadata.datatype.DataType;
+
+/**
+ * This class is to pass the Schema Information to the Carbon Columnar 
Batch
+ */
+public class StructField {
--- End diff --

Carbon core already has this class right? why new class


---

[GitHub] carbondata pull request #1581: [CARBONDATA-1779] GenericVectorizedReader

[ravipesala]

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

https://github.com/apache/carbondata/pull/1581#discussion_r154838240
  
--- Diff: 
integration/presto/src/main/java/org/apache/carbondata/presto/ColumnarVectorWrapper.java
 ---
@@ -17,23 +17,24 @@
 
 package org.apache.carbondata.presto;
 
+import java.math.BigDecimal;
--- End diff --

Better give some other name to this class, same name classes confuses


---

[GitHub] carbondata pull request #1581: [CARBONDATA-1779] GenericVectorizedReader

[chenliang613]

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

https://github.com/apache/carbondata/pull/1581#discussion_r154812647
  
--- Diff: integration/presto/pom.xml ---
@@ -31,7 +31,7 @@
   presto-plugin
 
   
-0.186
+0.187
--- End diff --

two question :1. as per mailing list discussed, the version of presto be 
suggested with 0.186, here changes to 0.187 , i think we should have a stable 
and fixed version number,it might not be good if carbon1.3 optionally upgrade 
version number for presto integration module. 2. if users still use the old 
presto version, whether it works , or not ?


---