[CARBONDATA-2876]Fix Avro decimal datatype with precision and scale #2687

Indhumathi27 · 2018-09-03T12:08:39Z

Why is PR for?

1.Add precision and scale for fieldvalue for Avro Decimal logical type.
2.If Avro schema is of union type with multiple record or multiple enum, then add check for schema.

Any interfaces changed?
Any backward compatibility impacted?
Document update required?
Testing done
For large changes, please consider breaking it into sub-tasks under an umbrella JIRA.

Indhumathi27 · 2018-09-03T13:18:07Z

Retest this please

CarbonDataQA · 2018-09-03T13:18:49Z

Build Failed with Spark 2.1.0, Please check CI http://136.243.101.176:8080/job/ApacheCarbonPRBuilder1/8257/

CarbonDataQA · 2018-09-03T14:57:45Z

Build Success with Spark 2.1.0, Please check CI http://136.243.101.176:8080/job/ApacheCarbonPRBuilder1/8260/

CarbonDataQA · 2018-09-03T15:04:57Z

Build Success with Spark 2.2.1, Please check CI http://95.216.28.178:8080/job/ApacheCarbonPRBuilder1/189/

manishgupta88 · 2018-09-04T05:30:54Z

store/sdk/src/main/java/org/apache/carbondata/sdk/file/AvroCarbonWriter.java

+          out = (decimalValue.round(
+              new MathContext(((LogicalTypes.Decimal) avroField.getLogicalType()).getPrecision())))
+              .setScale(((LogicalTypes.Decimal) avroField.getLogicalType()).getScale(),
+                  RoundingMode.HALF_UP);


replace bigdecimal conversion with below code at all places in the code
BigDecimal bigDecimal = new BigDecimal(new String(((ByteBuffer) fieldValue).array(), CarbonCommonConstants.DEFAULT_CHARSET_CLASS) .setScale(dimension.getColumnSchema().getScale(), RoundingMode.HALF_UP);

CarbonDataQA · 2018-09-04T11:59:16Z

Build Failed with Spark 2.2.1, Please check CI http://95.216.28.178:8080/job/ApacheCarbonPRBuilder1/227/

CarbonDataQA · 2018-09-04T12:02:52Z

Build Success with Spark 2.1.0, Please check CI http://136.243.101.176:8080/job/ApacheCarbonPRBuilder1/8297/

CarbonDataQA · 2018-09-05T14:09:40Z

Build Success with Spark 2.1.0, Please check CI http://136.243.101.176:8080/job/ApacheCarbonPRBuilder2.1/6/

CarbonDataQA · 2018-09-06T10:58:17Z

Build Success with Spark 2.1.0, Please check CI http://136.243.101.176:8080/job/ApacheCarbonPRBuilder2.1/119/

CarbonDataQA · 2018-09-06T11:46:00Z

Build Failed with Spark 2.3.1, Please check CI http://136.243.101.176:8080/job/ApacheCarbonPRBuilder2.3/8357/

CarbonDataQA · 2018-09-06T11:50:43Z

Build Failed with Spark 2.2.1, Please check CI http://95.216.28.178:8080/job/ApacheCarbonPRBuilder1/287/

CarbonDataQA · 2018-09-06T12:49:19Z

Build Success with Spark 2.1.0, Please check CI http://136.243.101.176:8080/job/ApacheCarbonPRBuilder2.1/121/

CarbonDataQA · 2018-09-06T13:04:09Z

Build Success with Spark 2.1.0, Please check CI http://136.243.101.176:8080/job/ApacheCarbonPRBuilder2.1/122/

CarbonDataQA · 2018-09-06T14:14:54Z

Build Failed with Spark 2.3.1, Please check CI http://136.243.101.176:8080/job/ApacheCarbonPRBuilder2.3/8360/

CarbonDataQA · 2018-09-06T14:29:07Z

Build Success with Spark 2.2.1, Please check CI http://95.216.28.178:8080/job/ApacheCarbonPRBuilder1/290/

CarbonDataQA · 2018-09-07T08:11:45Z

Build Success with Spark 2.1.0, Please check CI http://136.243.101.176:8080/job/ApacheCarbonPRBuilder2.1/138/

CarbonDataQA · 2018-09-07T08:57:06Z

Build Success with Spark 2.2.1, Please check CI http://95.216.28.178:8080/job/ApacheCarbonPRBuilder1/306/

CarbonDataQA · 2018-09-07T09:09:20Z

Build Failed with Spark 2.3.1, Please check CI http://136.243.101.176:8080/job/ApacheCarbonPRBuilder2.3/8376/

CarbonDataQA · 2018-09-07T10:33:54Z

Build Success with Spark 2.1.0, Please check CI http://136.243.101.176:8080/job/ApacheCarbonPRBuilder2.1/147/

CarbonDataQA · 2018-09-07T11:21:16Z

Build Failed with Spark 2.3.1, Please check CI http://136.243.101.176:8080/job/ApacheCarbonPRBuilder2.3/8386/

CarbonDataQA · 2018-09-07T11:25:32Z

Build Failed with Spark 2.2.1, Please check CI http://95.216.28.178:8080/job/ApacheCarbonPRBuilder1/315/

CarbonDataQA · 2018-09-07T13:04:24Z

Build Success with Spark 2.1.0, Please check CI http://136.243.101.176:8080/job/ApacheCarbonPRBuilder2.1/155/

CarbonDataQA · 2018-09-07T13:32:09Z

Build Failed with Spark 2.3.1, Please check CI http://136.243.101.176:8080/job/ApacheCarbonPRBuilder2.3/8394/

CarbonDataQA · 2018-09-07T14:05:43Z

Build Success with Spark 2.2.1, Please check CI http://95.216.28.178:8080/job/ApacheCarbonPRBuilder1/323/

KanakaKumar · 2018-09-08T06:32:04Z

...carbondata/spark/testsuite/createTable/TestNonTransactionalCarbonTableWithAvroDataType.scala

+    checkExistence(sql("select * from sdkOutputTable"), true, "12.80")
+  }
+
+  test("test logical type decimal with data having greater precision") {


Please add a test to verify boundary values like negative and large big decimal values (Big precission needs long to store unscaled value).

store/sdk/src/main/java/org/apache/carbondata/sdk/file/AvroCarbonWriter.java

KanakaKumar · 2018-09-08T06:42:04Z

...carbondata/spark/testsuite/createTable/TestNonTransactionalCarbonTableWithAvroDataType.scala

+        |	"name": "Union_data3",
+        |	"fields": [
+        |      {
+        |		  "name": "emp_id",


Please use related names for field. emp_id and union types are not related

KanakaKumar · 2018-09-08T06:45:32Z

store/sdk/src/main/java/org/apache/carbondata/sdk/file/AvroCarbonWriter.java

   * @return
   */
-  private boolean checkFieldValueType(Schema.Type type, Object fieldValue) {
+  private boolean checkFieldValueType(Schema.Type type, Object fieldValue, Schema unionField) {


Rename method to validateUnionFieldValue()

KanakaKumar · 2018-09-08T07:04:03Z

...carbondata/spark/testsuite/createTable/TestNonTransactionalCarbonTableWithAvroDataType.scala

+        | "type": "record",
+        | "fields": [
+        |  { "name": "street", "type": "string"},
+        |  { "name": "city", "type": {"type" : "bytes",


city and decimal is not correct example

KanakaKumar · 2018-09-08T07:17:43Z

store/sdk/src/main/java/org/apache/carbondata/sdk/file/AvroCarbonWriter.java

@@ -525,6 +559,11 @@ private static Field prepareFields(Schema.Field avroField) {
          int precision = ((LogicalTypes.Decimal) childSchema.getLogicalType()).getPrecision();
          int scale = ((LogicalTypes.Decimal) childSchema.getLogicalType()).getScale();
          return new Field(fieldName, DataTypes.createDecimalType(precision, scale));
+        } else if (logicalType == null && childSchema.getObjectProp("logicalType")


Why this specific check is required?
If this case is for invalid schema, AVRO should validate and throw error. If avro consideres this case just like Bytes, we can also consider same. carbon needs not have this specific check.

In the else case we should throw exception or leave to default case to throw bytes is not supported exception.

KanakaKumar · 2018-09-08T07:18:07Z

store/sdk/src/main/java/org/apache/carbondata/sdk/file/AvroCarbonWriter.java

@@ -621,6 +660,11 @@ private static StructField prepareSubFields(String fieldName, Schema childSchema
          int precision = ((LogicalTypes.Decimal) childSchema.getLogicalType()).getPrecision();
          int scale = ((LogicalTypes.Decimal) childSchema.getLogicalType()).getScale();
          return new StructField(fieldName, DataTypes.createDecimalType(precision, scale));
+        } else if (logicalType == null && childSchema.getObjectProp("logicalType")


Consider the previous comment in this case also.

KanakaKumar · 2018-09-08T07:18:46Z

store/sdk/src/main/java/org/apache/carbondata/sdk/file/AvroCarbonWriter.java

@@ -714,6 +758,11 @@ private static DataType getMappingDataTypeForCollectionRecord(Schema childSchema
          int precision = ((LogicalTypes.Decimal) childSchema.getLogicalType()).getPrecision();
          int scale = ((LogicalTypes.Decimal) childSchema.getLogicalType()).getScale();
          return DataTypes.createDecimalType(precision, scale);
+        } else if (logicalType == null && childSchema.getObjectProp("logicalType")


Consider the previous comment for decimal here also.

CarbonDataQA · 2018-09-08T10:06:16Z

Build Success with Spark 2.1.0, Please check CI http://136.243.101.176:8080/job/ApacheCarbonPRBuilder2.1/175/

CarbonDataQA · 2018-09-08T10:54:26Z

Build Failed with Spark 2.3.1, Please check CI http://136.243.101.176:8080/job/ApacheCarbonPRBuilder2.3/8414/

CarbonDataQA · 2018-09-08T10:54:43Z

Build Success with Spark 2.2.1, Please check CI http://95.216.28.178:8080/job/ApacheCarbonPRBuilder1/343/

KanakaKumar · 2018-09-08T11:00:16Z

store/sdk/src/main/java/org/apache/carbondata/sdk/file/AvroCarbonWriter.java

+      case BYTES:
+        // DECIMAL type is defined in Avro as a BYTE type with the logicalType property
+        // set to "decimal" and a specified precision and scale
+        // As binary type is not supported yet,value will be null


Please remove comment. Its misleading.

KanakaKumar · 2018-09-08T11:05:20Z

store/sdk/src/main/java/org/apache/carbondata/sdk/file/AvroCarbonWriter.java

+        // set to "decimal" and a specified precision and scale
+        // As binary type is not supported yet,value will be null
+        if (logicalType instanceof LogicalTypes.Decimal) {
+          BigDecimal dataValue = new BigDecimal(new BigInteger(((ByteBuffer) fieldValue).array()),


Please extract logic of constructing the BigDecimal value and validation to a separate method and reuse in both places

KanakaKumar · 2018-09-08T11:15:51Z

...carbondata/spark/testsuite/createTable/TestNonTransactionalCarbonTableWithAvroDataType.scala

+      Seq(Row(Row(Row(null,null),Row("ab")))))
+  }
+
+  test("test union with multiple Enum type") {


Can you add test case to read using data source file format. (Syntax "using carbon" with schema Refer SparkCarbonDataSourceTest ).
This will help users on how to define schema for avro logical types & union .

CarbonDataQA · 2018-09-08T13:04:17Z

Build Success with Spark 2.1.0, Please check CI http://136.243.101.176:8080/job/ApacheCarbonPRBuilder2.1/178/

KanakaKumar · 2018-09-08T13:13:26Z

LGTM

CarbonDataQA · 2018-09-08T13:29:13Z

Build Success with Spark 2.1.0, Please check CI http://136.243.101.176:8080/job/ApacheCarbonPRBuilder2.1/179/

CarbonDataQA · 2018-09-08T14:39:32Z

Build Failed with Spark 2.3.1, Please check CI http://136.243.101.176:8080/job/ApacheCarbonPRBuilder2.3/8418/

CarbonDataQA · 2018-09-08T14:53:51Z

Build Success with Spark 2.2.1, Please check CI http://95.216.28.178:8080/job/ApacheCarbonPRBuilder1/347/

manishgupta88 · 2018-09-10T06:36:23Z

LGTM

Indhumathi27 force-pushed the decimal_fix branch from a6e5149 to aece78c Compare September 3, 2018 13:50

manishgupta88 reviewed Sep 4, 2018

View reviewed changes

Indhumathi27 force-pushed the decimal_fix branch from aece78c to 576a9e3 Compare September 4, 2018 10:45

Indhumathi27 force-pushed the decimal_fix branch from 576a9e3 to ecec394 Compare September 6, 2018 10:36

Indhumathi27 force-pushed the decimal_fix branch from ecec394 to 142e9b4 Compare September 6, 2018 12:39

Indhumathi27 force-pushed the decimal_fix branch from 142e9b4 to 21ab8f5 Compare September 6, 2018 12:50

Indhumathi27 force-pushed the decimal_fix branch from 21ab8f5 to 8869fe5 Compare September 7, 2018 07:01

Indhumathi27 force-pushed the decimal_fix branch from 8869fe5 to cd64696 Compare September 7, 2018 10:11

Indhumathi27 force-pushed the decimal_fix branch from cd64696 to 08fc944 Compare September 7, 2018 12:51

KanakaKumar reviewed Sep 8, 2018

View reviewed changes

store/sdk/src/main/java/org/apache/carbondata/sdk/file/AvroCarbonWriter.java Outdated Show resolved Hide resolved

KanakaKumar reviewed Sep 8, 2018

View reviewed changes

Indhumathi27 force-pushed the decimal_fix branch from 08fc944 to 9634c4f Compare September 8, 2018 09:41

KanakaKumar reviewed Sep 8, 2018

View reviewed changes

Indhumathi27 force-pushed the decimal_fix branch from 9634c4f to d38b4a7 Compare September 8, 2018 12:52

Fix decimal type for Avro

d673027

Indhumathi27 force-pushed the decimal_fix branch from d38b4a7 to d673027 Compare September 8, 2018 13:16

asfgit closed this in 9ebab57 Sep 10, 2018

[CARBONDATA-2876]Fix Avro decimal datatype with precision and scale #2687

[CARBONDATA-2876]Fix Avro decimal datatype with precision and scale #2687

Conversation

Indhumathi27 commented Sep 3, 2018 • edited

Indhumathi27 commented Sep 3, 2018

CarbonDataQA commented Sep 3, 2018

CarbonDataQA commented Sep 3, 2018

CarbonDataQA commented Sep 3, 2018

Choose a reason for hiding this comment

CarbonDataQA commented Sep 4, 2018

CarbonDataQA commented Sep 4, 2018

CarbonDataQA commented Sep 5, 2018

CarbonDataQA commented Sep 6, 2018

CarbonDataQA commented Sep 6, 2018

CarbonDataQA commented Sep 6, 2018

CarbonDataQA commented Sep 6, 2018

CarbonDataQA commented Sep 6, 2018

CarbonDataQA commented Sep 6, 2018

CarbonDataQA commented Sep 6, 2018

CarbonDataQA commented Sep 7, 2018

CarbonDataQA commented Sep 7, 2018

CarbonDataQA commented Sep 7, 2018

CarbonDataQA commented Sep 7, 2018

CarbonDataQA commented Sep 7, 2018

CarbonDataQA commented Sep 7, 2018

CarbonDataQA commented Sep 7, 2018

CarbonDataQA commented Sep 7, 2018

CarbonDataQA commented Sep 7, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CarbonDataQA commented Sep 8, 2018

CarbonDataQA commented Sep 8, 2018

CarbonDataQA commented Sep 8, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KanakaKumar Sep 8, 2018 • edited

Choose a reason for hiding this comment

CarbonDataQA commented Sep 8, 2018

KanakaKumar commented Sep 8, 2018

CarbonDataQA commented Sep 8, 2018

CarbonDataQA commented Sep 8, 2018

CarbonDataQA commented Sep 8, 2018

manishgupta88 commented Sep 10, 2018

Indhumathi27 commented Sep 3, 2018 •

edited

KanakaKumar Sep 8, 2018 •

edited