[GitHub] [carbondata] akkio-97 opened a new pull request #4073: WIP

[GitBox]

ajantha-bhat commented on a change in pull request #4073:
URL: https://github.com/apache/carbondata/pull/4073#discussion_r566594846



##########
File path: core/src/main/java/org/apache/carbondata/core/metadata/datatype/DecimalConverterFactory.java
##########
@@ -141,72 +151,48 @@ public void fillVector(Object valuesToBeConverted, int size,
         // for string, varchar, binary, date, decimal types
         lengthStoredInBytes = shortSizeInBytes;
       }
-      byte[] data = (byte[]) valuesToBeConverted;
-      if (pageType == DataTypes.BYTE) {
-        for (int i = 0; i < size; i++) {
-          if (nullBitSet.get(i)) {
-            vector.putNull(i);
-          } else {
-            BigDecimal value = BigDecimal.valueOf(data[i], scale);
-            if (value.scale() < newMeasureScale) {
-              value = value.setScale(newMeasureScale);
-            }
-            vector.putDecimal(i, value, precision);
-          }
-        }
-      } else if (pageType == DataTypes.SHORT) {
-        for (int i = 0; i < size; i++) {
-          if (nullBitSet.get(i)) {
-            vector.putNull(i);
-          } else {
-            BigDecimal value = BigDecimal
-                .valueOf(ByteUtil.toShortLittleEndian(data, i * shortSizeInBytes),
-                    scale);
-            if (value.scale() < newMeasureScale) {
-              value = value.setScale(newMeasureScale);
-            }
-            vector.putDecimal(i, value, precision);
-          }
-        }
-      } else if (pageType == DataTypes.SHORT_INT) {
-        int shortIntSizeInBytes = DataTypes.SHORT_INT.getSizeInBytes();
+      if (this instanceof DecimalUnscaledConverter && scale < newMeasureScale) {
+        scale = newMeasureScale;
+      }
+      if (valuesToBeConverted instanceof byte[][]) {

Review comment:
       revert this change as discussed.




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
[hidden email]

[GitBox]

ajantha-bhat commented on a change in pull request #4073:
URL: https://github.com/apache/carbondata/pull/4073#discussion_r566597101



##########
File path: core/src/main/java/org/apache/carbondata/core/metadata/datatype/DecimalConverterFactory.java
##########
@@ -109,29 +113,35 @@ public BigDecimal getDecimal(Object valueToBeConverted) {
       return BigDecimal.valueOf((Long) valueToBeConverted, scale);
     }
 
+    private void initializeValues(int size, CarbonColumnVector vector,
+        ColumnVectorInfo vectorInfo) {
+      if (vectorInfo.measure == null) {
+        DecimalType decimalType = (DecimalType) vector.getType();
+        // complex primitive decimal flow comes as dimension
+        this.precision = decimalType.getPrecision();
+        this.newMeasureScale = decimalType.getScale();
+        this.updatedSize = ColumnVectorInfo.getUpdatedPageSizeForChildVector(vectorInfo, size);
+      } else {
+        CarbonMeasure carbonMeasure = vectorInfo.measure.getMeasure();
+        this.precision = carbonMeasure.getPrecision();
+        this.newMeasureScale = carbonMeasure.getScale();
+      }
+    }
+
     @Override
     public void fillVector(Object valuesToBeConverted, int size,
         ColumnVectorInfo vectorInfo, BitSet nullBitSet, DataType pageType) {
-      if (!(valuesToBeConverted instanceof byte[])) {
-        throw new UnsupportedOperationException("This object type " + valuesToBeConverted.getClass()
-            + " is not supported in this method");
-      }
       // TODO we need to find way to directly set to vector with out conversion. This way is very
       // inefficient.
       CarbonColumnVector vector = getCarbonColumnVector(vectorInfo, nullBitSet);
-      int precision;
-      int newMeasureScale;
-      if (vectorInfo.measure == null) {
-        // complex primitive decimal flow comes as dimension
-        precision = ((DecimalType) vector.getType()).getPrecision();
-        newMeasureScale = ((DecimalType) vector.getType()).getScale();
-        size = ColumnVectorInfo.getUpdatedPageSizeForChildVector(vectorInfo, size);
-      } else {
-        precision = vectorInfo.measure.getMeasure().getPrecision();
-        newMeasureScale = vectorInfo.measure.getMeasure().getScale();
+      initializeValues(size, vector, vectorInfo);

Review comment:
       If used only once, no need to extract common code.




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
[hidden email]

[GitBox]

CarbonDataQA2 commented on pull request #4073:
URL: https://github.com/apache/carbondata/pull/4073#issuecomment-769709334


   Build Success with Spark 2.3.4, Please check CI http://121.244.95.60:12444/job/ApacheCarbonPRBuilder2.3/5376/
   


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
[hidden email]

[GitBox]

CarbonDataQA2 commented on pull request #4073:
URL: https://github.com/apache/carbondata/pull/4073#issuecomment-769715382


   Build Success with Spark 2.4.5, Please check CI http://121.244.95.60:12444/job/ApacheCarbon_PR_Builder_2.4.5/3616/
   


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
[hidden email]

[GitBox]

akkio-97 commented on a change in pull request #4073:
URL: https://github.com/apache/carbondata/pull/4073#discussion_r566734422



##########
File path: core/src/main/java/org/apache/carbondata/core/metadata/datatype/DecimalConverterFactory.java
##########
@@ -324,33 +311,6 @@ public BigDecimal getDecimal(Object valueToBeConverted) {
       return new BigDecimal(bigInteger, scale);
     }
 
-    @Override
-    public void fillVector(Object valuesToBeConverted, int size,
-        ColumnVectorInfo vectorInfo, BitSet nullBitSet, DataType pageType) {
-      CarbonColumnVector vector = getCarbonColumnVector(vectorInfo, nullBitSet);
-      //TODO handle complex child
-      int precision = vectorInfo.measure.getMeasure().getPrecision();
-      int newMeasureScale = vectorInfo.measure.getMeasure().getScale();
-      if (scale < newMeasureScale) {
-        scale = newMeasureScale;
-      }
-      if (valuesToBeConverted instanceof byte[][]) {
-        byte[][] data = (byte[][]) valuesToBeConverted;
-        for (int i = 0; i < size; i++) {
-          if (nullBitSet.get(i)) {
-            vector.putNull(i);
-          } else {
-            BigInteger bigInteger = new BigInteger(data[i]);
-            BigDecimal value = new BigDecimal(bigInteger, scale);
-            if (value.scale() < newMeasureScale) {
-              value = value.setScale(newMeasureScale);
-            }
-            vector.putDecimal(i, value, precision);
-          }
-        }
-      }

Review comment:
       okay




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
[hidden email]

[GitBox]

ajantha-bhat commented on a change in pull request #4073:
URL: https://github.com/apache/carbondata/pull/4073#discussion_r566778045



##########
File path: core/src/main/java/org/apache/carbondata/core/metadata/datatype/DecimalConverterFactory.java
##########
@@ -327,25 +287,28 @@ public BigDecimal getDecimal(Object valueToBeConverted) {
     @Override
     public void fillVector(Object valuesToBeConverted, int size,
         ColumnVectorInfo vectorInfo, BitSet nullBitSet, DataType pageType) {
-      CarbonColumnVector vector = getCarbonColumnVector(vectorInfo, nullBitSet);
-      //TODO handle complex child
-      int precision = vectorInfo.measure.getMeasure().getPrecision();
-      int newMeasureScale = vectorInfo.measure.getMeasure().getScale();
-      if (scale < newMeasureScale) {
-        scale = newMeasureScale;
-      }
-      if (valuesToBeConverted instanceof byte[][]) {
-        byte[][] data = (byte[][]) valuesToBeConverted;
-        for (int i = 0; i < size; i++) {
-          if (nullBitSet.get(i)) {
-            vector.putNull(i);
-          } else {
-            BigInteger bigInteger = new BigInteger(data[i]);
-            BigDecimal value = new BigDecimal(bigInteger, scale);
-            if (value.scale() < newMeasureScale) {
-              value = value.setScale(newMeasureScale);
+      if (valuesToBeConverted instanceof byte[]) {
+        super.fillVector(valuesToBeConverted, size, vectorInfo, nullBitSet, pageType);

Review comment:
       use return and remove else.




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
[hidden email]

[GitBox]

ajantha-bhat commented on a change in pull request #4073:
URL: https://github.com/apache/carbondata/pull/4073#discussion_r566780442



##########
File path: core/src/main/java/org/apache/carbondata/core/metadata/datatype/DecimalConverterFactory.java
##########
@@ -110,8 +110,8 @@ public BigDecimal getDecimal(Object valueToBeConverted) {
     }
 
     @Override
-    public void fillVector(Object valuesToBeConverted, int size,
-        ColumnVectorInfo vectorInfo, BitSet nullBitSet, DataType pageType) {
+    public void fillVector(Object valuesToBeConverted, int size, ColumnVectorInfo vectorInfo,

Review comment:
       revert this style change




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
[hidden email]

[GitBox]

ajantha-bhat commented on a change in pull request #4073:
URL: https://github.com/apache/carbondata/pull/4073#discussion_r566804833



##########
File path: core/src/main/java/org/apache/carbondata/core/metadata/datatype/DecimalConverterFactory.java
##########
@@ -327,25 +287,28 @@ public BigDecimal getDecimal(Object valueToBeConverted) {
     @Override
     public void fillVector(Object valuesToBeConverted, int size,
         ColumnVectorInfo vectorInfo, BitSet nullBitSet, DataType pageType) {
-      CarbonColumnVector vector = getCarbonColumnVector(vectorInfo, nullBitSet);
-      //TODO handle complex child
-      int precision = vectorInfo.measure.getMeasure().getPrecision();
-      int newMeasureScale = vectorInfo.measure.getMeasure().getScale();
-      if (scale < newMeasureScale) {
-        scale = newMeasureScale;
-      }
-      if (valuesToBeConverted instanceof byte[][]) {
-        byte[][] data = (byte[][]) valuesToBeConverted;
-        for (int i = 0; i < size; i++) {
-          if (nullBitSet.get(i)) {
-            vector.putNull(i);
-          } else {
-            BigInteger bigInteger = new BigInteger(data[i]);
-            BigDecimal value = new BigDecimal(bigInteger, scale);
-            if (value.scale() < newMeasureScale) {
-              value = value.setScale(newMeasureScale);
+      if (valuesToBeConverted instanceof byte[]) {
+        super.fillVector(valuesToBeConverted, size, vectorInfo, nullBitSet, pageType);

Review comment:
       could have merged into single statement. no need to modify now.  it's enough




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
[hidden email]

[GitBox]

ajantha-bhat commented on pull request #4073:
URL: https://github.com/apache/carbondata/pull/4073#issuecomment-769791390


   LGTM


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
[hidden email]

[GitBox]

CarbonDataQA2 commented on pull request #4073:
URL: https://github.com/apache/carbondata/pull/4073#issuecomment-769837818


   Build Success with Spark 2.3.4, Please check CI http://121.244.95.60:12444/job/ApacheCarbonPRBuilder2.3/5379/
   


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
[hidden email]

[GitBox]

CarbonDataQA2 commented on pull request #4073:
URL: https://github.com/apache/carbondata/pull/4073#issuecomment-769838251


   Build Success with Spark 2.4.5, Please check CI http://121.244.95.60:12444/job/ApacheCarbon_PR_Builder_2.4.5/3619/
   


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
[hidden email]

[GitBox]

asfgit closed pull request #4073:
URL: https://github.com/apache/carbondata/pull/4073


   


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
[hidden email]