You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@hudi.apache.org by GitBox <gi...@apache.org> on 2020/08/12 05:57:52 UTC
[GitHub] [hudi] zhedoubushishi opened a new pull request #1953: [HUDI-1181] Fix decimal type display issue for record key field
zhedoubushishi opened a new pull request #1953:
URL: https://github.com/apache/hudi/pull/1953
## *Tips*
- *Thank you very much for contributing to Apache Hudi.*
- *Please review https://hudi.apache.org/contributing.html before opening a pull request.*
## What is the purpose of the pull request
When using ```fixed_len_byte_array``` decimal type as Hudi record key, Hudi would not correctly display the decimal value, instead, Hudi would display it as a byte array.
During the Hudi writing phase, Hudi would save the parquet source data into Avro Generic Record. For example, the source parquet data has a column with decimal type:
```
optional fixed_len_byte_array(16) LN_LQDN_OBJ_ID (DECIMAL(38,0));
```
Then Hudi will convert it into the following avro decimal type:
```
{
"name" : "LN_LQDN_OBJ_ID",
"type" : [ {
"type" : "fixed",
"name" : "fixed",
"namespace" : "hoodie.hudi_ln_lqdn.hudi_ln_lqdn_record.LN_LQDN_OBJ_ID",
"size" : 16,
"logicalType" : "decimal",
"precision" : 38,
"scale" : 0
}, "null" ]
}
```
This decimal field would be stored as a fixed length bytes array. And in the reading phase, Hudi will convert this bytes array back to a readable decimal value through this [converter](https://github.com/apache/hudi/blob/master/hudi-spark/src/main/scala/org/apache/hudi/AvroConversionHelper.scala#L58).
However, the problem is, when setting decimal type as record keys, Hudi would read the value from Avro Generic Record and then directly convert it into ```String``` type(See [here](https://github.com/apache/hudi/blob/master/hudi-spark/src/main/java/org/apache/hudi/DataSourceUtils.java#L76)). As a result, what shows in the ```_hoodie_record_key``` field would be something like: ```LN_LQDN_OBJ_ID:[0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 25, 40, 95, -71]```.
So we need to handle this special case to convert bytes array back before converting to ```String```.
## Brief change log
Similar to what we did for Date type columns: https://github.com/apache/hudi/commit/2d040145810b8b14c59c5882f9115698351039d1#diff-21f77fb372831d468dab018505592e12, I added another logic to handle decimal type column.
## Verify this pull request
This change added tests and can be verified as follows:
- *Added a decimal test case in TestDataSourceUtils.java to verify the change.*
## Committer checklist
- [x] Has a corresponding JIRA in PR title & commit
- [ ] Commit message is descriptive of the change
- [ ] CI is green
- [ ] Necessary doc changes done or have another open PR
- [ ] For large changes, please consider breaking it into sub-tasks under an umbrella JIRA.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [hudi] umehrot2 commented on a change in pull request #1953: [HUDI-1181] Fix decimal type display issue for record key field
Posted by GitBox <gi...@apache.org>.
umehrot2 commented on a change in pull request #1953:
URL: https://github.com/apache/hudi/pull/1953#discussion_r476840436
##########
File path: hudi-common/src/main/java/org/apache/hudi/avro/HoodieAvroUtils.java
##########
@@ -433,23 +434,46 @@ private static Object convertValueForSpecificDataTypes(Schema fieldSchema, Objec
return fieldValue;
}
- if (isLogicalTypeDate(fieldSchema)) {
- return LocalDate.ofEpochDay(Long.parseLong(fieldValue.toString()));
+ if (fieldSchema.getType() == Schema.Type.UNION) {
+ for (Schema schema : fieldSchema.getTypes()) {
+ if (schema.getType() != Schema.Type.NULL) {
Review comment:
Do we need to check this explicitly ? `convertValueForAvroLogicalTypes` will either ways just return the field value in this case.
The check is either ways not being performed in case Schema type is not `UNION`. It might be okay to remove this.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [hudi] zhedoubushishi commented on a change in pull request #1953: [HUDI-1181] Fix decimal type display issue for record key field
Posted by GitBox <gi...@apache.org>.
zhedoubushishi commented on a change in pull request #1953:
URL: https://github.com/apache/hudi/pull/1953#discussion_r476901023
##########
File path: hudi-common/src/main/java/org/apache/hudi/avro/HoodieAvroUtils.java
##########
@@ -433,23 +434,46 @@ private static Object convertValueForSpecificDataTypes(Schema fieldSchema, Objec
return fieldValue;
}
- if (isLogicalTypeDate(fieldSchema)) {
- return LocalDate.ofEpochDay(Long.parseLong(fieldValue.toString()));
+ if (fieldSchema.getType() == Schema.Type.UNION) {
+ for (Schema schema : fieldSchema.getTypes()) {
+ if (schema.getType() != Schema.Type.NULL) {
Review comment:
For schema like:
```
{
"name":"event_cost3",
"type":[
"null",
{
"type":"fixed",
"name":"dc",
"size":5,
"logicalType":"decimal",
"precision":10,
"scale":6
}
]
```
or
```
{
"name":"event_date3",
"type":[
"null",
{
"type":"int",
"logicalType":"date"
}
]
```
The first schema it gets is null, w/o this condition, it will directly return the fieldValue w/o any special conversion. BTW this comment remind me to add another test case for decimal type with union.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [hudi] umehrot2 commented on pull request #1953: [HUDI-1181] Fix decimal type display issue for record key field
Posted by GitBox <gi...@apache.org>.
umehrot2 commented on pull request #1953:
URL: https://github.com/apache/hudi/pull/1953#issuecomment-685975783
@vinothchandar if its okay with you I can merge this.
@zhedoubushishi can you squash all the commits into one ?
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [hudi] vinothchandar commented on pull request #1953: [HUDI-1181] Fix decimal type display issue for record key field
Posted by GitBox <gi...@apache.org>.
vinothchandar commented on pull request #1953:
URL: https://github.com/apache/hudi/pull/1953#issuecomment-675845027
cc @umehrot2
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [hudi] vinothchandar commented on pull request #1953: [HUDI-1181] Fix decimal type display issue for record key field
Posted by GitBox <gi...@apache.org>.
vinothchandar commented on pull request #1953:
URL: https://github.com/apache/hudi/pull/1953#issuecomment-688579652
@umehrot2 lgtm. you can just `Squash and merge` this yourself
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [hudi] linshan-ma commented on pull request #1953: [HUDI-1181] Fix decimal type display issue for record key field
Posted by GitBox <gi...@apache.org>.
linshan-ma commented on pull request #1953:
URL: https://github.com/apache/hudi/pull/1953#issuecomment-675810867
hudi-1154 is the same problem as this one，I've already tested this pr，it is ok。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:
users@infra.apache.org
[GitHub] [hudi] umehrot2 commented on a change in pull request #1953: [HUDI-1181] Fix decimal type display issue for record key field
Posted by GitBox <gi...@apache.org>.
umehrot2 commented on a change in pull request #1953:
URL: https://github.com/apache/hudi/pull/1953#discussion_r475021802
##########
File path: hudi-common/src/main/java/org/apache/hudi/avro/HoodieAvroUtils.java
##########
@@ -431,25 +435,30 @@ private static Object convertValueForSpecificDataTypes(Schema fieldSchema, Objec
return fieldValue;
}
- if (isLogicalTypeDate(fieldSchema)) {
+ if (fieldSchema.getType() == Schema.Type.UNION) {
+ for (Schema schema : fieldSchema.getTypes()) {
+ if (schema.getLogicalType() == LogicalTypes.date() || schema.getLogicalType() instanceof LogicalTypes.Decimal) {
+ return convertValueForSpecificDataTypes(schema, fieldValue);
+ }
+ }
+ } else if (fieldSchema.getLogicalType() == LogicalTypes.date()) {
+ // special handle for Logical Date type
return LocalDate.ofEpochDay(Long.parseLong(fieldValue.toString()));
+ } else if (fieldSchema.getLogicalType() instanceof LogicalTypes.Decimal) {
+ // special handle for Logical Decimal type
+ Decimal dc = (Decimal) fieldSchema.getLogicalType();
+ DecimalConversion decimalConversion = new DecimalConversion();
+ if (fieldSchema.getType() == Schema.Type.FIXED) {
+ return decimalConversion.fromFixed((GenericFixed) fieldValue, fieldSchema,
+ LogicalTypes.decimal(dc.getPrecision(), dc.getScale()));
+ } else if (fieldSchema.getType() == Schema.Type.BYTES) {
+ return decimalConversion.fromBytes((ByteBuffer) fieldValue, fieldSchema,
+ LogicalTypes.decimal(dc.getPrecision(), dc.getScale()));
+ }
Review comment:
I would suggest to abstract this out into its own function like convertValueForAvroLogicalTypes() and call it from both places instead of using recursive call which is adding overhead of more repetitive checks happening. Since this will be called for each record I would like to keep it as lightweight as possible.
```
if (fieldSchema.getType() == Schema.Type.UNION) {
for (Schema schema : fieldSchema.getTypes()) {
return convertValueForAvroLogicalTypes()
}
} else {
return convertValueForAvroLogicalTypes()
}
```
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [hudi] umehrot2 merged pull request #1953: [HUDI-1181] Fix decimal type display issue for record key field
Posted by GitBox <gi...@apache.org>.
umehrot2 merged pull request #1953:
URL: https://github.com/apache/hudi/pull/1953
----------------------------------------------------------------
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:
users@infra.apache.org