codope commented on code in PR #9315: URL: https://github.com/apache/hudi/pull/9315#discussion_r1278800755
########## hudi-common/src/main/avro/HoodieDeleteRecordList.avsc: ########## @@ -0,0 +1,71 @@ +/* + * 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. + */ +{ + "namespace": "org.apache.hudi.avro.model", + "type": "record", + "name": "HoodieDeleteRecordList", + "doc": "A list of delete records stored in the delete block in log block version 3", + "fields": [ + { + "name": "deleteRecordList", + "type": { + "type": "array", + "items": { + "name": "HoodieDeleteRecord", + "type": "record", + "fields": [ + { + "name": "recordKey", + "type": [ + "null", + "string" + ], + "default": null Review Comment: let's also add doc to these fields. ########## hudi-common/src/main/java/org/apache/hudi/avro/HoodieAvroUtils.java: ########## @@ -1149,4 +1192,98 @@ public static boolean gteqAvro1_9() { public static boolean gteqAvro1_10() { return VersionUtil.compareVersions(AVRO_VERSION, "1.10") >= 0; } + + /** + * Wraps a value into Avro type wrapper. + * + * @param value Java value. + * @return A wrapped value with Avro type wrapper. + */ + public static Object wrapValueIntoAvro(Comparable<?> value) { + if (value == null) { + return null; + } else if (value instanceof Date || value instanceof LocalDate) { + // NOTE: Due to breaking changes in code-gen b/w Avro 1.8.2 and 1.10, we can't + // rely on logical types to do proper encoding of the native Java types, + // and hereby have to encode value manually + LocalDate localDate = value instanceof LocalDate + ? (LocalDate) value + : ((Date) value).toLocalDate(); + return DateWrapper.newBuilder(DATE_WRAPPER_BUILDER_STUB.get()) + .setValue((int) localDate.toEpochDay()) + .build(); + } else if (value instanceof BigDecimal) { + Schema valueSchema = DecimalWrapper.SCHEMA$.getField("value").schema(); + BigDecimal upcastDecimal = tryUpcastDecimal((BigDecimal) value, (LogicalTypes.Decimal) valueSchema.getLogicalType()); + return DecimalWrapper.newBuilder(DECIMAL_WRAPPER_BUILDER_STUB.get()) + .setValue(AVRO_DECIMAL_CONVERSION.toBytes(upcastDecimal, valueSchema, valueSchema.getLogicalType())) + .build(); + } else if (value instanceof Timestamp) { + // NOTE: Due to breaking changes in code-gen b/w Avro 1.8.2 and 1.10, we can't + // rely on logical types to do proper encoding of the native Java types, + // and hereby have to encode value manually + Instant instant = ((Timestamp) value).toInstant(); + return TimestampMicrosWrapper.newBuilder(TIMESTAMP_MICROS_WRAPPER_BUILDER_STUB.get()) + .setValue(instantToMicros(instant)) Review Comment: Just curious why don't we need something similar timestamp-millis? Because that is covered by LongWrapper? ########## hudi-common/src/main/java/org/apache/hudi/avro/HoodieAvroUtils.java: ########## @@ -1149,4 +1192,98 @@ public static boolean gteqAvro1_9() { public static boolean gteqAvro1_10() { return VersionUtil.compareVersions(AVRO_VERSION, "1.10") >= 0; } + + /** + * Wraps a value into Avro type wrapper. + * + * @param value Java value. + * @return A wrapped value with Avro type wrapper. + */ + public static Object wrapValueIntoAvro(Comparable<?> value) { Review Comment: Let's add a test for wrapping/unwrapping especially for timestamp types? I know this is merely moving an existing code to the right util class. But, would be good for future-proofing. We never know when can things break again in some other AVro minor version. ########## hudi-common/src/main/avro/HoodieDeleteRecordList.avsc: ########## @@ -0,0 +1,71 @@ +/* + * 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. + */ +{ + "namespace": "org.apache.hudi.avro.model", + "type": "record", + "name": "HoodieDeleteRecordList", + "doc": "A list of delete records stored in the delete block in log block version 3", + "fields": [ + { + "name": "deleteRecordList", + "type": { + "type": "array", + "items": { + "name": "HoodieDeleteRecord", + "type": "record", + "fields": [ + { + "name": "recordKey", + "type": [ + "null", + "string" + ], + "default": null + }, + { + "name": "partitionPath", + "type": [ + "null", + "string" + ], + "default": null + }, + { + "name": "orderingVal", + "type": [ + "null", + "org.apache.hudi.avro.model.BooleanWrapper", + "org.apache.hudi.avro.model.IntWrapper", + "org.apache.hudi.avro.model.LongWrapper", + "org.apache.hudi.avro.model.FloatWrapper", + "org.apache.hudi.avro.model.DoubleWrapper", + "org.apache.hudi.avro.model.BytesWrapper", + "org.apache.hudi.avro.model.StringWrapper", + "org.apache.hudi.avro.model.DateWrapper", + "org.apache.hudi.avro.model.DecimalWrapper", + "org.apache.hudi.avro.model.TimeMicrosWrapper", + "org.apache.hudi.avro.model.TimestampMicrosWrapper" Review Comment: What's the difference between `TimeMicrosWrapper` and `TimestampMicrosWrapper`? ########## hudi-common/src/main/java/org/apache/hudi/common/table/log/block/HoodieLogBlock.java: ########## @@ -51,7 +51,7 @@ public abstract class HoodieLogBlock { * corresponding changes need to be made to {@link HoodieLogBlockVersion} TODO : Change this to a class, something * like HoodieLogBlockVersionV1/V2 and implement/override operations there */ - public static int version = 2; + public static int version = 3; Review Comment: let's update the above javadoc too? ########## hudi-common/src/test/java/org/apache/hudi/common/table/log/block/TestHoodieDeleteBlock.java: ########## Review Comment: Can we also cover an e2e test (maybe in mor datasource or an upgrade test) which adds a delete block in v2, then merges and writes back in v3 and then reads the snapsphot again? -- 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. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
