wombatu-kun commented on code in PR #19809:
URL: https://github.com/apache/hudi/pull/19809#discussion_r3921006419
##########
hudi-common/src/main/java/org/apache/hudi/common/avro/HoodieAvroUtils.java:
##########
@@ -112,7 +114,44 @@
import static org.apache.hudi.common.util.ValidationUtils.checkState;
/**
- * Helper class to do common stuff across Avro.
+ * Operations on Avro records and values.
+ *
+ * <p>This class owns the record-level half of the Avro world:</p>
+ * <ul>
+ * <li>serialization: {@code avroToBytes} / {@code bytesToAvro} and the JSON
converters</li>
+ * <li>record rewriting between schemas: {@code rewriteRecord*}, {@code
projectRecordToNewSchemaShallow},
+ * {@code stitchRecords}</li>
+ * <li>value coercion for logical types: {@code convertValueFor*}, {@code
convertBytesToBigDecimal}</li>
+ * <li>record-key, metadata-column and nested-value access: {@code
getNestedFieldVal},
+ * {@code addHoodieKeyToRecord}, {@code getRecordColumnValues}, {@code
createHoodieRecordFromAvro}</li>
+ * </ul>
+ *
+ * <p>Raw {@link Schema} helpers live here only when they serve a record
operation on this class (for example
Review Comment:
`projectSchema` is given here as a raw-`Schema` helper that serves a record
operation on this class, but nothing on this class calls it and its only
production caller is `HoodieSchemaUtils.projectSchema`, which the sibling
javadoc lists as a delegation being retired. Could it be dropped from the
example list, leaving `createNewSchemaField` and `unwrapNullable`?
##########
hudi-common/src/main/java/org/apache/hudi/common/avro/HoodieAvroUtils.java:
##########
@@ -221,13 +260,13 @@ public static byte[] avroToFileBytes(IndexedRecord
record) {
* @param record The GenericRecord to convert
* @param pretty Whether to pretty-print the json output
*/
- public static String avroToJsonString(GenericRecord record, boolean pretty)
throws IOException {
+ private static String avroToJsonString(GenericRecord record, boolean pretty)
throws IOException {
return avroToJsonHelper(record, pretty).toString();
}
/**
* Convert a given avro record to a JSON string. If the record contents are
invalid, return the record.toString().
- * Use this method over {@link HoodieAvroUtils#avroToJsonString} when simply
trying to print the record contents without any guarantees around their
correctness.
+ * Use this method over {@link #avroToJson(GenericRecord, boolean)} when
simply trying to print the record contents without any guarantees around their
correctness.
Review Comment:
`avroToJson` returns `byte[]` while `safeAvroToJsonString` returns `String`,
so it is not an alternative a caller would weigh against this method - the
sibling it actually wraps is the now-private `avroToJsonString`. Could the
comparison just be dropped, since the preceding sentence already states the
fallback behaviour?
##########
hudi-common/src/main/java/org/apache/hudi/common/avro/HoodieAvroUtils.java:
##########
@@ -1274,7 +1317,8 @@ private static Object
rewritePrimaryTypeWithDiffSchemaType(Object oldValue, Sche
* bytes is the result of BigDecimal.unscaledValue().toByteArray();
* This is also what Conversions.DecimalConversion.toBytes() outputs inside
a byte buffer
*/
- public static Object convertBytesToFixed(byte[] bytes, Schema schema) {
+ @VisibleForTesting
Review Comment:
`convertBytesToBigDecimal(byte[], LogicalTypes.Decimal)` is in the same
position as `convertBytesToFixed` - its only caller outside this class is
`TestMercifulJsonConverter`, in this package - but stayed public and
unannotated. `rewritePrimaryType` and the `convertToRecord` overloads are the
same shape with test-only callers in other modules: could all four get the
narrowing treatment this PR describes?
##########
hudi-common/src/test/java/org/apache/hudi/common/schema/TestLocalHoodieSchemaCache.java:
##########
@@ -0,0 +1,68 @@
+/*
+ * 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.hudi.common.schema;
+
+import org.apache.hudi.common.testutils.HoodieTestDataGenerator;
+
+import org.junit.jupiter.api.Test;
+
+import java.util.Arrays;
+import java.util.HashSet;
+import java.util.Set;
+
+import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertFalse;
+import static org.junit.jupiter.api.Assertions.assertTrue;
+
+public class TestLocalHoodieSchemaCache {
+
+ @Test
+ public void testBasicCacheUsage() {
+ LocalHoodieSchemaCache schemaCache = LocalHoodieSchemaCache.create();
Review Comment:
Neither test creates two caches, so both still pass if `create()` returned a
shared singleton - the one property the rename and its new javadoc assert.
Could one case call `create()` twice and assert the two id spaces are
independent?
##########
hudi-common/src/main/java/org/apache/hudi/common/schema/HoodieSchemaUtils.java:
##########
@@ -24,34 +24,60 @@
import org.apache.hudi.common.schema.internal.HoodieSchemaException;
import org.apache.hudi.common.util.Option;
import org.apache.hudi.common.util.ValidationUtils;
+import org.apache.hudi.common.util.VisibleForTesting;
import org.apache.hudi.common.util.collection.Pair;
import org.apache.hudi.exception.HoodieException;
import org.apache.avro.JsonProperties;
import org.apache.avro.Schema;
import org.apache.avro.generic.GenericData;
-import java.math.BigDecimal;
import java.util.ArrayList;
import java.util.Collections;
import java.util.List;
import java.util.Map;
import java.util.Objects;
import java.util.Set;
-import java.util.function.Function;
import java.util.regex.Pattern;
import java.util.stream.Collectors;
+import java.util.stream.Stream;
/**
- * Utility class for HoodieSchema operations including table schema
manipulation,
- * compatibility checking, and schema evolution operations.
+ * HoodieSchema-typed structural transforms of table schemas and of the
well-known Hudi record shapes.
*
- * <p>This class provides HoodieSchema equivalents of operations found in
AvroSchemaUtils
- * and HoodieAvroUtils, focusing on table schema management rather than
record-level operations.</p>
+ * <p>What lives here:</p>
Review Comment:
The list omits about a third of the public API - all three
`createNewSchemaField` overloads, `getFieldSchema`, `createHoodieWriteSchema`,
`hasDecimalField`, `createNewSchemaFromFieldsWithReference` and
`toJavaDefaultValue` - and `getNestedField` is filed under "needs more than
HoodieSchema offers on its own" although its own javadoc in this PR calls it a
null-checking facade over `HoodieSchema#getNestedField`. Could the missing ones
that fit an existing bullet be added, and `getNestedField` move to the
"questions about a single schema" line under "Not here"?
--
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]
