Re: [PR] refactor(schema): schema util contracts, dead API and folds [hudi]
voonhous merged PR #19809: URL: https://github.com/apache/hudi/pull/19809 -- 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]
Re: [PR] refactor(schema): schema util contracts, dead API and folds [hudi]
hudi-bot commented on PR #19809: URL: https://github.com/apache/hudi/pull/19809#issuecomment-5523744314 ## CI report: * f25cd74f84ec2427fe1b8bce20d33c703091cd3b Azure: [SUCCESS](https://dev.azure.com/apachehudi/a1a51da7-8592-47d4-88dc-fd67bed336bb/_build/results?buildId=16796) Bot commands @hudi-bot supports the following commands: - `@hudi-bot run azure` re-run the last Azure build -- 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]
Re: [PR] refactor(schema): schema util contracts, dead API and folds [hudi]
hudi-bot commented on PR #19809: URL: https://github.com/apache/hudi/pull/19809#issuecomment-5522583974 ## CI report: * 7eb2cbf495cf5debaee5500d0ed85a2283479623 Azure: [SUCCESS](https://dev.azure.com/apachehudi/a1a51da7-8592-47d4-88dc-fd67bed336bb/_build/results?buildId=16792) * f25cd74f84ec2427fe1b8bce20d33c703091cd3b Azure: [PENDING](https://dev.azure.com/apachehudi/a1a51da7-8592-47d4-88dc-fd67bed336bb/_build/results?buildId=16796) Bot commands @hudi-bot supports the following commands: - `@hudi-bot run azure` re-run the last Azure build -- 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]
Re: [PR] refactor(schema): schema util contracts, dead API and folds [hudi]
hudi-bot commented on PR #19809: URL: https://github.com/apache/hudi/pull/19809#issuecomment-5522514713 ## CI report: * 7eb2cbf495cf5debaee5500d0ed85a2283479623 Azure: [SUCCESS](https://dev.azure.com/apachehudi/a1a51da7-8592-47d4-88dc-fd67bed336bb/_build/results?buildId=16792) * f25cd74f84ec2427fe1b8bce20d33c703091cd3b UNKNOWN Bot commands @hudi-bot supports the following commands: - `@hudi-bot run azure` re-run the last Azure build -- 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]
Re: [PR] refactor(schema): schema util contracts, dead API and folds [hudi]
hudi-bot commented on PR #19809: URL: https://github.com/apache/hudi/pull/19809#issuecomment-5521838588 ## CI report: * 7eb2cbf495cf5debaee5500d0ed85a2283479623 Azure: [SUCCESS](https://dev.azure.com/apachehudi/a1a51da7-8592-47d4-88dc-fd67bed336bb/_build/results?buildId=16792) Bot commands @hudi-bot supports the following commands: - `@hudi-bot run azure` re-run the last Azure build -- 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]
Re: [PR] refactor(schema): schema util contracts, dead API and folds [hudi]
hudi-bot commented on PR #19809: URL: https://github.com/apache/hudi/pull/19809#issuecomment-5520929244 ## CI report: * 6094dce391a383d3c26aa023973bd458413cd675 Azure: [SUCCESS](https://dev.azure.com/apachehudi/a1a51da7-8592-47d4-88dc-fd67bed336bb/_build/results?buildId=16780) * 7eb2cbf495cf5debaee5500d0ed85a2283479623 Azure: [PENDING](https://dev.azure.com/apachehudi/a1a51da7-8592-47d4-88dc-fd67bed336bb/_build/results?buildId=16792) Bot commands @hudi-bot supports the following commands: - `@hudi-bot run azure` re-run the last Azure build -- 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]
Re: [PR] refactor(schema): schema util contracts, dead API and folds [hudi]
hudi-bot commented on PR #19809: URL: https://github.com/apache/hudi/pull/19809#issuecomment-5520881120 ## CI report: * 6094dce391a383d3c26aa023973bd458413cd675 Azure: [SUCCESS](https://dev.azure.com/apachehudi/a1a51da7-8592-47d4-88dc-fd67bed336bb/_build/results?buildId=16780) * 7eb2cbf495cf5debaee5500d0ed85a2283479623 UNKNOWN Bot commands @hudi-bot supports the following commands: - `@hudi-bot run azure` re-run the last Azure build -- 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]
Re: [PR] refactor(schema): schema util contracts, dead API and folds [hudi]
voonhous commented on code in PR #19809:
URL: https://github.com/apache/hudi/pull/19809#discussion_r3921264289
##
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:
Dropped in 173cef432361.
##
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:
Done in 0b0c95ecdcf5. `convertBytesToBigDecimal(byte[], Decimal)` and the 4-
and 6-arg `convertToRecord` are package-private with `@VisibleForTesting`
(callers: this class plus `TestHoodieAvroUtils` / `TestMercifulJsonConverter`
in the same package). `rewritePrimaryType` and the 7-arg `convertToRecord` keep
`public` with `@VisibleForTesting` because their test callers are in other
modules (`TestHoodieArrayWritableSchemaUtils` in hudi-hadoop-mr,
`TestColStatsRecordWithMetadataRecord` in hudi-spark). PR body updated: 13
narrowed, and the "kept on purpose" line now names only the 5-arg
`createNewSchemaField`.
--
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]
Re: [PR] refactor(schema): schema util contracts, dead API and folds [hudi]
voonhous commented on code in PR #19809: URL: https://github.com/apache/hudi/pull/19809#discussion_r3921266731 ## 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. * - * This class provides HoodieSchema equivalents of operations found in AvroSchemaUtils - * and HoodieAvroUtils, focusing on table schema management rather than record-level operations. + * What lives here: Review Comment: Reworked in 173cef432361: added `createHoodieWriteSchema`, `createNewSchemaFromFieldsWithReference`, the `createNewSchemaField` copy factory (with the two aliases noted), `toJavaDefaultValue`, `getRecordQualifiedName` and `hasDecimalField`; `getNestedField` and `getFieldSchema` now sit under "questions about a single schema" as facades over the `HoodieSchema` instance methods. Still unlisted on purpose: `addMetadataColumnTypes` (test-only), `createSchemaErrorString`, and `createNullableSchema`, which the delegation paragraph already covers. -- 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]
Re: [PR] refactor(schema): schema util contracts, dead API and folds [hudi]
voonhous commented on code in PR #19809:
URL: https://github.com/apache/hudi/pull/19809#discussion_r3921267952
##
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:
Added `testCreateReturnsIndependentCaches` in 7eb2cbf495cf: two caches, the
first cache's id misses in the second, and the same id resolves to a different
schema in each.
--
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]
Re: [PR] refactor(schema): schema util contracts, dead API and folds [hudi]
voonhous commented on code in PR #19809:
URL: https://github.com/apache/hudi/pull/19809#discussion_r3921262931
##
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.
+ *
+ * This class owns the record-level half of the Avro world:
+ *
+ * serialization: {@code avroToBytes} / {@code bytesToAvro} and the JSON
converters
+ * record rewriting between schemas: {@code rewriteRecord*}, {@code
projectRecordToNewSchemaShallow},
+ * {@code stitchRecords}
+ * value coercion for logical types: {@code convertValueFor*}, {@code
convertBytesToBigDecimal}
+ * record-key, metadata-column and nested-value access: {@code
getNestedFieldVal},
+ * {@code addHoodieKeyToRecord}, {@code getRecordColumnValues}, {@code
createHoodieRecordFromAvro}
+ *
+ *
+ * Raw {@link Schema} helpers live here only when they serve a record
operation on this class (for example
Review Comment:
Dropped in 173cef432361; the example list is now `createNewSchemaField`,
`unwrapNullable`.
--
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]
Re: [PR] refactor(schema): schema util contracts, dead API and folds [hudi]
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.
+ *
+ * This class owns the record-level half of the Avro world:
+ *
+ * serialization: {@code avroToBytes} / {@code bytesToAvro} and the JSON
converters
+ * record rewriting between schemas: {@code rewriteRecord*}, {@code
projectRecordToNewSchemaShallow},
+ * {@code stitchRecords}
+ * value coercion for logical types: {@code convertValueFor*}, {@code
convertBytesToBigDecimal}
+ * record-key, metadata-column and nested-value access: {@code
getNestedFieldVal},
+ * {@code addHoodieKeyToRecord}, {@code getRecordColumnValues}, {@code
createHoodieRecordFromAvro}
+ *
+ *
+ * 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
Re: [PR] refactor(schema): schema util contracts, dead API and folds [hudi]
voonhous commented on PR #19809: URL: https://github.com/apache/hudi/pull/19809#issuecomment-5519812529 @wombatu-kun Can you please help to review this when you are free? -- 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]
Re: [PR] refactor(schema): schema util contracts, dead API and folds [hudi]
hudi-bot commented on PR #19809: URL: https://github.com/apache/hudi/pull/19809#issuecomment-5514805945 ## CI report: * 6094dce391a383d3c26aa023973bd458413cd675 Azure: [SUCCESS](https://dev.azure.com/apachehudi/a1a51da7-8592-47d4-88dc-fd67bed336bb/_build/results?buildId=16780) Bot commands @hudi-bot supports the following commands: - `@hudi-bot run azure` re-run the last Azure build -- 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]
Re: [PR] refactor(schema): schema util contracts, dead API and folds [hudi]
hudi-bot commented on PR #19809: URL: https://github.com/apache/hudi/pull/19809#issuecomment-5514135478 ## CI report: * 2abcfe1e3a9ca0882484f234248086b54d3f0e15 Azure: [SUCCESS](https://dev.azure.com/apachehudi/a1a51da7-8592-47d4-88dc-fd67bed336bb/_build/results?buildId=16778) * 6094dce391a383d3c26aa023973bd458413cd675 Azure: [PENDING](https://dev.azure.com/apachehudi/a1a51da7-8592-47d4-88dc-fd67bed336bb/_build/results?buildId=16780) Bot commands @hudi-bot supports the following commands: - `@hudi-bot run azure` re-run the last Azure build -- 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]
Re: [PR] refactor(schema): schema util contracts, dead API and folds [hudi]
voonhous commented on code in PR #19809:
URL: https://github.com/apache/hudi/pull/19809#discussion_r3916152801
##
hudi-common/src/main/java/org/apache/hudi/common/schema/HoodieSchemaTypePromotion.java:
##
@@ -19,20 +19,28 @@
package org.apache.hudi.common.schema;
/**
- * Defines type promotion rules for HoodieSchema compatibility checking.
+ * The single table of primitive widening promotions, used by {@link
HoodieSchemaProjectionChecker}.
*
- * Type promotion allows a reader schema with a "wider" type to read data
- * written with a "narrower" type. This follows Avro's type promotion
rules.
- *
- * Supported promotions:
+ * A promotion lets a reader schema with a wider type read data written
with a narrower one:
*
- * INT → LONG, FLOAT, DOUBLE
- * LONG → FLOAT, DOUBLE
- * FLOAT → DOUBLE
- * STRING ↔ BYTES (bidirectional)
- * Decimal precision widening: (p2-s2) ≥ (p1-s1) and s2 ≥ s1
+ * INT -> LONG, FLOAT, DOUBLE
+ * LONG -> FLOAT, DOUBLE
+ * FLOAT -> DOUBLE
+ * STRING <-> BYTES (bidirectional)
+ * STRING <- any numeric type
+ * decimal widening, see {@link #isDecimalWidening(HoodieSchema,
HoodieSchema)}
*
- *
+ *
+ * Logical-type-over-primitive promotions are deliberately NOT in this
table. A TIMESTAMP reader over a
+ * LONG writer, or a UUID reader over a STRING writer, is accepted by
+ * {@link HoodieSchemaCompatibilityChecker} for reader/writer compatibility,
but it must not make a bare
+ * long a "compatible projection" of a timestamp: writer-schema deduction
would then silently drop the
+ * logical type. Compatibility and projection are different questions, so they
use different tables.
+ *
+ * One more difference is documented rather than resolved:
+ * {@link #isDecimalWidening(HoodieSchema, HoodieSchema)} additionally
requires the same backing (fixed
+ * versus bytes) and, for fixed, an equal fixed size, whereas the decimal
check in
+ * {@code HoodieSchemaCompatibilityChecker} compares only precision and
scale.
*
* This class is package-private and used internally by schema
compatibility checkers.
Review Comment:
Done in d74b3cd4f3b6.
##
hudi-common/src/main/java/org/apache/hudi/common/avro/AvroSchemaUtils.java:
##
@@ -39,7 +39,14 @@
import static org.apache.hudi.common.util.ValidationUtils.checkState;
/**
- * Utils for Avro Schema.
+ * Avro-typed schema helpers, retained only as the delegate target of the call
sites that have not moved to
+ * HoodieSchema yet: {@link
org.apache.hudi.common.schema.HoodieSchemaUtils#asNullable(HoodieSchema)} and
+ * {@code HoodieSchemaUtils#createNullableSchema}, the field construction
inside {@link HoodieSchema.Blob},
+ * and a handful of internal uses in {@link HoodieAvroUtils}.
+ *
+ * This class is being retired under #16639. Do not add methods here: the
HoodieSchema twin of every
+ * method on this class already exists, so use {@link
org.apache.hudi.common.schema.HoodieSchema} or
+ * {@link org.apache.hudi.common.schema.HoodieSchemaUtils} instead.
Review Comment:
Done in d74b3cd4f3b6.
--
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]
Re: [PR] refactor(schema): schema util contracts, dead API and folds [hudi]
hudi-bot commented on PR #19809: URL: https://github.com/apache/hudi/pull/19809#issuecomment-5513724924 ## CI report: * 557a2743c60e6ee0217a8dfa951b8f7e195610a0 Azure: [SUCCESS](https://dev.azure.com/apachehudi/a1a51da7-8592-47d4-88dc-fd67bed336bb/_build/results?buildId=16745) * 2abcfe1e3a9ca0882484f234248086b54d3f0e15 Azure: [PENDING](https://dev.azure.com/apachehudi/a1a51da7-8592-47d4-88dc-fd67bed336bb/_build/results?buildId=16778) * 6094dce391a383d3c26aa023973bd458413cd675 Azure: [PENDING](https://dev.azure.com/apachehudi/a1a51da7-8592-47d4-88dc-fd67bed336bb/_build/results?buildId=16780) Bot commands @hudi-bot supports the following commands: - `@hudi-bot run azure` re-run the last Azure build -- 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]
Re: [PR] refactor(schema): schema util contracts, dead API and folds [hudi]
hudi-bot commented on PR #19809: URL: https://github.com/apache/hudi/pull/19809#issuecomment-5513422482 ## CI report: * 557a2743c60e6ee0217a8dfa951b8f7e195610a0 Azure: [SUCCESS](https://dev.azure.com/apachehudi/a1a51da7-8592-47d4-88dc-fd67bed336bb/_build/results?buildId=16745) * 2abcfe1e3a9ca0882484f234248086b54d3f0e15 Azure: [PENDING](https://dev.azure.com/apachehudi/a1a51da7-8592-47d4-88dc-fd67bed336bb/_build/results?buildId=16778) * 6094dce391a383d3c26aa023973bd458413cd675 UNKNOWN Bot commands @hudi-bot supports the following commands: - `@hudi-bot run azure` re-run the last Azure build -- 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]
Re: [PR] refactor(schema): schema util contracts, dead API and folds [hudi]
hudi-bot commented on PR #19809: URL: https://github.com/apache/hudi/pull/19809#issuecomment-5512737652 ## CI report: * 557a2743c60e6ee0217a8dfa951b8f7e195610a0 Azure: [SUCCESS](https://dev.azure.com/apachehudi/a1a51da7-8592-47d4-88dc-fd67bed336bb/_build/results?buildId=16745) * 2abcfe1e3a9ca0882484f234248086b54d3f0e15 Azure: [PENDING](https://dev.azure.com/apachehudi/a1a51da7-8592-47d4-88dc-fd67bed336bb/_build/results?buildId=16778) Bot commands @hudi-bot supports the following commands: - `@hudi-bot run azure` re-run the last Azure build -- 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]
Re: [PR] refactor(schema): schema util contracts, dead API and folds [hudi]
voonhous commented on code in PR #19809:
URL: https://github.com/apache/hudi/pull/19809#discussion_r3916664606
##
hudi-common/src/main/java/org/apache/hudi/common/avro/HoodieAvroUtils.java:
##
@@ -883,15 +924,16 @@ private static Object
normalizeAvroLogicalTypeToPrimitive(Object value, Schema s
*
* @param record Hoodie record.
* @param columns Names of the columns to get values.
- * @param schema {@link Schema} instance.
+ * @param schema {@link HoodieSchema} instance.
Review Comment:
Done in bb7554071228.
##
hudi-utilities/src/test/java/org/apache/hudi/utilities/sources/TestJsonKafkaSource.java:
##
@@ -296,7 +296,7 @@ private static void verifyDecimalValue(List
records, HoodieSchema
double maxVal = Math.pow(10, decSchema.getPrecision() -
decSchema.getScale());
double minVal = maxVal * 0.1;
for (GenericRecord record : records) {
- BigDecimal dec =
org.apache.hudi.common.schema.HoodieSchemaUtils.convertBytesToBigDecimal(((ByteBuffer)
record.get(fieldname)).array(), decSchema);
+ BigDecimal dec =
org.apache.hudi.common.avro.HoodieAvroUtils.convertBytesToBigDecimal(((ByteBuffer)
record.get(fieldname)).array(), decSchema);
Review Comment:
Done in 6094dce391a3.
--
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]
Re: [PR] refactor(schema): schema util contracts, dead API and folds [hudi]
voonhous commented on code in PR #19809:
URL: https://github.com/apache/hudi/pull/19809#discussion_r3916667723
##
hudi-common/src/test/java/org/apache/hudi/common/avro/TestHoodieAvroUtils.java:
##
@@ -566,6 +567,135 @@ public void
testConvertValueForAvroLogicalTypesCrossAvroVersion() {
assertEquals(FIXTURE_EPOCH_MICROS,
HoodieAvroUtils.convertValueForAvroLogicalTypes(LOCAL_TS_MICROS_SCHEMA,
FIXTURE_LOCAL_DT_MICROS, false));
}
+ @Test
+ public void testConvertValueForSpecificDataTypes_NullSchema() {
+// Test with null schema - should return value unchanged
+String testValue = "test_value";
+Object result = HoodieAvroUtils.convertValueForSpecificDataTypes(null,
testValue, false);
+assertEquals(testValue, result);
+ }
+
+ @Test
+ public void testConvertValueForSpecificDataTypes_NullValue_NullableSchema() {
+// Test with null value and nullable schema - should return null
+Schema nullableIntSchema =
HoodieSchema.createNullable(HoodieSchema.create(HoodieSchemaType.INT)).toAvroSchema();
+Object result =
HoodieAvroUtils.convertValueForSpecificDataTypes(nullableIntSchema, null,
false);
+assertNull(result);
+ }
+
+ @Test
+ public void
testConvertValueForSpecificDataTypes_NullValue_NonNullableSchema() {
+// Test with null value and non-nullable schema - should throw exception
+Schema nonNullableSchema = Schema.create(Schema.Type.STRING);
+assertThrows(IllegalStateException.class, () ->
+HoodieAvroUtils.convertValueForSpecificDataTypes(nonNullableSchema,
null, false));
+ }
+
+ @Test
+ public void testConvertValueForSpecificDataTypes_DateLogicalType() {
+// Create date schema
+Schema dateSchema = HoodieSchema.createDate().toAvroSchema();
Review Comment:
Done in 6094dce391a3.
--
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]
Re: [PR] refactor(schema): schema util contracts, dead API and folds [hudi]
voonhous commented on code in PR #19809:
URL: https://github.com/apache/hudi/pull/19809#discussion_r3916662915
##
hudi-common/src/main/java/org/apache/hudi/common/schema/HoodieSchemaUtils.java:
##
@@ -731,11 +720,41 @@ public static HoodieSchema
getRecordKeyPartitionPathSchema() {
return HoodieSchema.createRecord("HoodieRecordKey", "", "", false,
toBeAddedFields);
}
+ /**
+ * Schema of a native delete log record: the record key plus the ordering
fields, which are
+ * always nullable (see the comment in the body).
+ */
+ public static HoodieSchema createDeleteLogSchema(HoodieSchema tableSchema,
List orderingFieldNames) {
+// Native delete logs store only the record key plus optional ordering
values, so ordering fields in
+// the delete-log schema must always be nullable even when the table
schema marks them required.
+// A delete record such as HoodieEmptyRecord may carry
OrderingValues.getDefault() as an in-memory
+// sentinel rather than a real field value. Persist NULL for that missing
value so readers can map it
+// back to the default ordering without confusing it with a real business
value such as 0.
+List fields = Stream.concat(
+Stream.of(createNewSchemaField(
+HoodieRecord.RECORD_KEY_METADATA_FIELD,
HoodieSchema.create(HoodieSchemaType.STRING), null, null)),
+orderingFieldNames.stream().map(orderingFieldName ->
tableSchema.getField(orderingFieldName)
+.map(field -> createNewSchemaField(
+field.name(), HoodieSchema.createNullable(field.schema()),
field.doc().orElse(null), HoodieSchema.NULL_VALUE))
+.orElseThrow(() ->
+new IllegalArgumentException("Ordering field " +
orderingFieldName + " not found in table schema"
+.collect(Collectors.toList());
+return HoodieSchema.createRecord("hudi_delete_log_record", null, null,
fields);
+ }
+
/**
* Fetches projected schema given list of fields to project. The field can
be nested in format `a.b.c` where a is
- * the top level field, b is at second level and so on.
+ * the top level field, b is at second level and so on. Field names are
matched case-sensitively.
* This is equivalent to {@link HoodieAvroUtils#projectSchema(Schema, List)}
but operates on HoodieSchema.
*
+ * The two sibling projection helpers differ:
+ *
+ * {@link #generateProjectionSchema(HoodieSchema, List)} - top-level
fields only, matched
Review Comment:
Done in bb7554071228.
--
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]
Re: [PR] refactor(schema): schema util contracts, dead API and folds [hudi]
voonhous commented on code in PR #19809:
URL: https://github.com/apache/hudi/pull/19809#discussion_r3916659640
##
hudi-common/src/test/java/org/apache/hudi/common/schema/TestHoodieSchemaUtils.java:
##
@@ -2198,4 +2083,78 @@ public void testGetNestedFieldComplexNestedMapAndArray()
{
assertEquals("value", result.get().getRight().name());
assertEquals(HoodieSchemaType.LONG,
result.get().getRight().schema().getType());
}
+
+ private static HoodieSchema deleteLogTableSchema() {
+return HoodieSchema.createRecord(
+"TestRecord",
+null,
+null,
+Arrays.asList(
+HoodieSchemaField.of("ts",
HoodieSchema.create(HoodieSchemaType.LONG), "ordering field doc", null),
+HoodieSchemaField.of("name",
HoodieSchema.create(HoodieSchemaType.STRING)),
+HoodieSchemaField.of("seq",
HoodieSchema.create(HoodieSchemaType.STRING)),
+HoodieSchemaField.of("opt_ts", HoodieSchema.createUnion(
+HoodieSchema.create(HoodieSchemaType.LONG),
HoodieSchema.create(HoodieSchemaType.NULL)))
+)
+);
+ }
+
+ @Test
+ public void testCreateDeleteLogSchema() {
+HoodieSchema deleteLogSchema =
+HoodieSchemaUtils.createDeleteLogSchema(deleteLogTableSchema(),
Collections.singletonList("ts"));
+
+assertEquals("hudi_delete_log_record", deleteLogSchema.getName());
+assertEquals(2, deleteLogSchema.getFields().size());
+
+// The record key is always present and never nullable.
+HoodieSchemaField recordKeyField = deleteLogSchema.getFields().get(0);
+assertEquals(HoodieRecord.RECORD_KEY_METADATA_FIELD,
recordKeyField.name());
+assertEquals(HoodieSchemaType.STRING, recordKeyField.schema().getType());
+assertFalse(recordKeyField.isNullable());
+
+// The ordering field keeps its doc but is made nullable with a null
default, even though
+// the table schema marks it required.
+HoodieSchemaField orderingField = deleteLogSchema.getFields().get(1);
+assertEquals("ts", orderingField.name());
+assertTrue(orderingField.isNullable());
+assertEquals(HoodieSchemaType.LONG,
orderingField.getNonNullSchema().getType());
+assertEquals("ordering field doc", orderingField.doc().get());
+assertEquals(HoodieSchema.NULL_VALUE, orderingField.defaultVal().get());
+ }
+
+ @Test
+ public void testCreateDeleteLogSchemaOrderingFieldVariants() {
Review Comment:
Done in 6094dce391a3: `ts_ms` (timestamp-millis) and `amount` (decimal 10,2)
added to the fixture; type, precision and scale asserted after the nullable
wrapping.
##
hudi-common/src/test/java/org/apache/hudi/common/schema/TestHoodieSchemaUtils.java:
##
@@ -2198,4 +2083,78 @@ public void testGetNestedFieldComplexNestedMapAndArray()
{
assertEquals("value", result.get().getRight().name());
assertEquals(HoodieSchemaType.LONG,
result.get().getRight().schema().getType());
}
+
+ private static HoodieSchema deleteLogTableSchema() {
+return HoodieSchema.createRecord(
+"TestRecord",
+null,
+null,
+Arrays.asList(
+HoodieSchemaField.of("ts",
HoodieSchema.create(HoodieSchemaType.LONG), "ordering field doc", null),
+HoodieSchemaField.of("name",
HoodieSchema.create(HoodieSchemaType.STRING)),
+HoodieSchemaField.of("seq",
HoodieSchema.create(HoodieSchemaType.STRING)),
+HoodieSchemaField.of("opt_ts", HoodieSchema.createUnion(
+HoodieSchema.create(HoodieSchemaType.LONG),
HoodieSchema.create(HoodieSchemaType.NULL)))
+)
+);
+ }
+
+ @Test
+ public void testCreateDeleteLogSchema() {
+HoodieSchema deleteLogSchema =
+HoodieSchemaUtils.createDeleteLogSchema(deleteLogTableSchema(),
Collections.singletonList("ts"));
+
+assertEquals("hudi_delete_log_record", deleteLogSchema.getName());
+assertEquals(2, deleteLogSchema.getFields().size());
+
+// The record key is always present and never nullable.
+HoodieSchemaField recordKeyField = deleteLogSchema.getFields().get(0);
+assertEquals(HoodieRecord.RECORD_KEY_METADATA_FIELD,
recordKeyField.name());
+assertEquals(HoodieSchemaType.STRING, recordKeyField.schema().getType());
+assertFalse(recordKeyField.isNullable());
+
+// The ordering field keeps its doc but is made nullable with a null
default, even though
+// the table schema marks it required.
+HoodieSchemaField orderingField = deleteLogSchema.getFields().get(1);
+assertEquals("ts", orderingField.name());
+assertTrue(orderingField.isNullable());
+assertEquals(HoodieSchemaType.LONG,
orderingField.getNonNullSchema().getType());
+assertEquals("ordering field doc", orderingField.doc().get());
+assertEquals(HoodieSchema.NULL_VALUE, orderingField.defaultVal().get());
+ }
+
+ @Test
+ public void testCreateDeleteLogSchemaOrderingFieldVariants() {
+HoodieSchema tableSchema = deleteLogTableSchema();
+
+//
Re: [PR] refactor(schema): schema util contracts, dead API and folds [hudi]
voonhous commented on code in PR #19809:
URL: https://github.com/apache/hudi/pull/19809#discussion_r3916656149
##
hudi-common/src/main/java/org/apache/hudi/common/avro/HoodieAvroUtils.java:
##
@@ -883,15 +924,16 @@ private static Object
normalizeAvroLogicalTypeToPrimitive(Object value, Schema s
*
* @param record Hoodie record.
* @param columns Names of the columns to get values.
- * @param schema {@link Schema} instance.
+ * @param schema {@link HoodieSchema} instance.
* @return Column value.
*/
public static Object[] getRecordColumnValues(HoodieRecord record,
String[] columns,
- Schema schema,
+ HoodieSchema schema,
boolean
consistentLogicalTimestampEnabled) {
try {
- GenericRecord genericRecord = (GenericRecord)
(record.toIndexedRecord(HoodieAvroSchemaCache.intern(schema), new
Properties()).get()).getData();
+ // Intern so the identity fast path in BaseAvroPayload#getRecord hits
across callers that parse their own copy of the schema.
+ GenericRecord genericRecord = (GenericRecord)
(record.toIndexedRecord(HoodieSchemaCache.intern(schema), new
Properties()).get()).getData();
Review Comment:
Done in 1125fa3541c9: `HoodieAvroSchemaCache.intern(schema.toAvroSchema())`,
the master path, with the comment reworded to describe the cache choice.
##
hudi-common/src/test/java/org/apache/hudi/common/avro/TestHoodieAvroUtils.java:
##
@@ -955,6 +1085,25 @@ void
testGetSortColumnValuesWithPartitionPathAndRecordKey(boolean suffixRecordKe
}
}
+ @Test
+ void testGetRecordColumnValues() {
Review Comment:
Done in 1125fa3541c9: `testGetRecordColumnValuesInternsSchema` builds the
record on the interned Avro schema under `OverwriteWithLatestAvroPayload` and
reads it with a freshly parsed `HoodieSchema`. Checked that it fails without
the intern (`expected: but was: `). `student.firstnameNested`
dropped from the other test.
--
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]
Re: [PR] refactor(schema): schema util contracts, dead API and folds [hudi]
voonhous commented on code in PR #19809:
URL: https://github.com/apache/hudi/pull/19809#discussion_r3916368048
##
hudi-common/src/main/java/org/apache/hudi/common/avro/HoodieAvroUtils.java:
##
@@ -883,15 +924,16 @@ private static Object
normalizeAvroLogicalTypeToPrimitive(Object value, Schema s
*
* @param record Hoodie record.
* @param columns Names of the columns to get values.
- * @param schema {@link Schema} instance.
+ * @param schema {@link HoodieSchema} instance.
* @return Column value.
*/
public static Object[] getRecordColumnValues(HoodieRecord record,
String[] columns,
- Schema schema,
+ HoodieSchema schema,
boolean
consistentLogicalTimestampEnabled) {
try {
- GenericRecord genericRecord = (GenericRecord)
(record.toIndexedRecord(HoodieAvroSchemaCache.intern(schema), new
Properties()).get()).getData();
+ // Intern so the identity fast path in BaseAvroPayload#getRecord hits
across callers that parse their own copy of the schema.
+ GenericRecord genericRecord = (GenericRecord)
(record.toIndexedRecord(HoodieSchemaCache.intern(schema), new
Properties()).get()).getData();
Review Comment:
**major:** `HoodieSchemaCache` is value-keyed (`weakValues` + `maximumSize`,
`HoodieSchemaCache.java:36-37`) and `HoodieSchema.equals` only short-circuits
on identity before Avro's deep `RecordSchema.equals`, whereas master's
`HoodieAvroSchemaCache` front is `weakKeys` (identity). Every task whose
deserialized schema copy is not the stored key now pays a deep schema compare
per call: twice per comparison in `RDDBucketIndexPartitioner.java:92,94`, once
per record per column at `HoodieTableMetadataUtil.java:331`. Could this be
`HoodieAvroSchemaCache.intern(schema.toAvroSchema())`, which is byte-for-byte
the master path and lands on the same canonical instance?
##
hudi-common/src/main/java/org/apache/hudi/common/schema/HoodieSchemaUtils.java:
##
@@ -769,47 +788,27 @@ public static String getRecordQualifiedName(String
tableName) {
}
public static boolean hasDecimalField(HoodieSchema schema) {
-return hasDecimalWithCondition(schema, unused -> true);
- }
-
- /**
- * Checks whether the provided schema contains a decimal with a precision
less than or equal to 18,
- * which allows the decimal to be stored as int/long instead of a fixed size
byte array in
- * https://github.com/apache/parquet-format/blob/master/LogicalTypes.md";>parquet
logical types
- * @param schema the input schema to search
- * @return true if the schema contains a small precision decimal field and
false otherwise
- */
- public static boolean hasSmallPrecisionDecimalField(HoodieSchema schema) {
-return hasDecimalWithCondition(schema,
HoodieSchemaUtils::isSmallPrecisionDecimalField);
- }
-
- private static boolean hasDecimalWithCondition(HoodieSchema schema,
Function condition) {
switch (schema.getType()) {
case RECORD:
for (HoodieSchemaField field : schema.getFields()) {
- if (hasDecimalWithCondition(field.schema(), condition)) {
+ if (hasDecimalField(field.schema())) {
return true;
}
}
return false;
case ARRAY:
-return hasDecimalWithCondition(schema.getElementType(), condition);
+return hasDecimalField(schema.getElementType());
case MAP:
-return hasDecimalWithCondition(schema.getValueType(), condition);
+return hasDecimalField(schema.getValueType());
case UNION:
-return hasDecimalWithCondition(schema.getNonNullType(), condition);
+return hasDecimalField(schema.getNonNullType());
Review Comment:
**major:** pre-existing, not introduced by this fold: master has the same
arm (`HoodieSchemaUtils.java:799-800`, since #17600). `getNonNullType()`
returns `this` for a union without a null branch
(`HoodieSchema.java:1397-1399`) and `["null","string","int"]` reduces to
exactly that shape (line 1413), so this recurses to `StackOverflowError`.
`SourceFormatAdapter.java:261` calls it on every JSON source schema;
`HoodieSchemaRepair.hasTimestampMillisField:249` has the identical arm. Out of
scope for a behavior-preserving PR; should the
`getTypes().stream().anyMatch(...)` fix go in a separate PR, with an issue
linked here meanwhile?
##
hudi-common/src/test/java/org/apache/hudi/common/schema/TestHoodieSchemaUtils.java:
##
@@ -2198,4 +2083,78 @@ public void testGetNestedFieldComplexNestedMapAndArray()
{
assertEquals("value", result.get().getRight().name());
assertEquals(HoodieSchemaType.LONG,
result.get().getRight().schema().getType());
}
+
+ private static HoodieSchema deleteLogTableSchema() {
+return HoodieSchema.createRecord(
+
Re: [PR] refactor(schema): schema util contracts, dead API and folds [hudi]
voonhous commented on code in PR #19809:
URL: https://github.com/apache/hudi/pull/19809#discussion_r3916407496
##
hudi-common/src/main/java/org/apache/hudi/common/avro/HoodieAvroUtils.java:
##
@@ -883,15 +924,16 @@ private static Object
normalizeAvroLogicalTypeToPrimitive(Object value, Schema s
*
* @param record Hoodie record.
* @param columns Names of the columns to get values.
- * @param schema {@link Schema} instance.
+ * @param schema {@link HoodieSchema} instance.
* @return Column value.
*/
public static Object[] getRecordColumnValues(HoodieRecord record,
String[] columns,
- Schema schema,
+ HoodieSchema schema,
boolean
consistentLogicalTimestampEnabled) {
try {
- GenericRecord genericRecord = (GenericRecord)
(record.toIndexedRecord(HoodieAvroSchemaCache.intern(schema), new
Properties()).get()).getData();
+ // Intern so the identity fast path in BaseAvroPayload#getRecord hits
across callers that parse their own copy of the schema.
Review Comment:
Intended: master never interned in the sibling, and this PR is
behavior-preserving, so it stays as is. The comment now describes the cache
choice rather than the callers.
--
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]
Re: [PR] refactor(schema): schema util contracts, dead API and folds [hudi]
voonhous commented on code in PR #19809:
URL: https://github.com/apache/hudi/pull/19809#discussion_r3916149704
##
hudi-common/src/test/java/org/apache/hudi/common/table/read/lsm/TestLsmFileGroupRecordIterator.java:
##
@@ -166,7 +166,7 @@ void testUpdateProcessorOnlyRunsForRecordsFromLogs() throws
Exception {
@Test
void testDeleteLogSchemaUsesRecordKeyAndOrderingFields() {
-HoodieSchema deleteLogSchema =
HoodieSchemas.createDeleteLogSchema(tableSchema(), Arrays.asList("ts"));
+HoodieSchema deleteLogSchema =
HoodieSchemaUtils.createDeleteLogSchema(tableSchema(), Arrays.asList("ts"));
Review Comment:
Done in 2abcfe1e3a9c.
##
hudi-common/src/main/java/org/apache/hudi/common/avro/HoodieAvroUtils.java:
##
@@ -221,7 +259,7 @@ 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 {
Review Comment:
Done in d74b3cd4f3b6: now links `avroToJson(GenericRecord, boolean)`.
--
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]
Re: [PR] refactor(schema): schema util contracts, dead API and folds [hudi]
voonhous commented on code in PR #19809:
URL: https://github.com/apache/hudi/pull/19809#discussion_r3916404564
##
hudi-common/src/main/java/org/apache/hudi/common/schema/HoodieSchemaUtils.java:
##
@@ -769,47 +788,27 @@ public static String getRecordQualifiedName(String
tableName) {
}
public static boolean hasDecimalField(HoodieSchema schema) {
-return hasDecimalWithCondition(schema, unused -> true);
- }
-
- /**
- * Checks whether the provided schema contains a decimal with a precision
less than or equal to 18,
- * which allows the decimal to be stored as int/long instead of a fixed size
byte array in
- * https://github.com/apache/parquet-format/blob/master/LogicalTypes.md";>parquet
logical types
- * @param schema the input schema to search
- * @return true if the schema contains a small precision decimal field and
false otherwise
- */
- public static boolean hasSmallPrecisionDecimalField(HoodieSchema schema) {
-return hasDecimalWithCondition(schema,
HoodieSchemaUtils::isSmallPrecisionDecimalField);
- }
-
- private static boolean hasDecimalWithCondition(HoodieSchema schema,
Function condition) {
switch (schema.getType()) {
case RECORD:
for (HoodieSchemaField field : schema.getFields()) {
- if (hasDecimalWithCondition(field.schema(), condition)) {
+ if (hasDecimalField(field.schema())) {
return true;
}
}
return false;
case ARRAY:
-return hasDecimalWithCondition(schema.getElementType(), condition);
+return hasDecimalField(schema.getElementType());
case MAP:
-return hasDecimalWithCondition(schema.getValueType(), condition);
+return hasDecimalField(schema.getValueType());
case UNION:
-return hasDecimalWithCondition(schema.getNonNullType(), condition);
+return hasDecimalField(schema.getNonNullType());
Review Comment:
Filed as #19825 (with the `HoodieSchemaRepair.hasTimestampMillisField`
twin); fix goes in a separate PR, this one keeps the arm as on master.
##
hudi-common/src/test/java/org/apache/hudi/common/schema/TestHoodieSchemaUtils.java:
##
@@ -1047,6 +1043,11 @@ public void testGenerateProjectionSchema() {
assertTrue(fieldNames1.contains("_row_key"));
assertTrue(fieldNames1.contains("timestamp"));
+// Field names are matched case-insensitively; HiveHoodieReaderContext
lowercases names before calling this.
+HoodieSchema schema2 =
HoodieSchemaUtils.generateProjectionSchema(originalSchema,
Arrays.asList("_ROW_KEY"));
Review Comment:
Filed as #19826; fix in a separate PR. Correction to the claim above:
`generateProjectionSchema` has six callers, `HiveHoodieReaderContext` is the
one that pre-lowercases (with `Locale.ROOT`); the others lowercase both sides
through this call and stay consistent.
--
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]
Re: [PR] refactor(schema): schema util contracts, dead API and folds [hudi]
hudi-bot commented on PR #19809: URL: https://github.com/apache/hudi/pull/19809#issuecomment-5512651803 ## CI report: * 557a2743c60e6ee0217a8dfa951b8f7e195610a0 Azure: [SUCCESS](https://dev.azure.com/apachehudi/a1a51da7-8592-47d4-88dc-fd67bed336bb/_build/results?buildId=16745) * 2abcfe1e3a9ca0882484f234248086b54d3f0e15 UNKNOWN Bot commands @hudi-bot supports the following commands: - `@hudi-bot run azure` re-run the last Azure build -- 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]
Re: [PR] refactor(schema): schema util contracts, dead API and folds [hudi]
voonhous commented on code in PR #19809:
URL: https://github.com/apache/hudi/pull/19809#discussion_r3916140220
##
hudi-common/src/test/java/org/apache/hudi/common/schema/TestHoodieSchemaUtils.java:
##
@@ -2198,4 +2085,48 @@ public void testGetNestedFieldComplexNestedMapAndArray()
{
assertEquals("value", result.get().getRight().name());
assertEquals(HoodieSchemaType.LONG,
result.get().getRight().schema().getType());
}
+
+ private static HoodieSchema deleteLogTableSchema() {
+return HoodieSchema.createRecord(
+"TestRecord",
+null,
+null,
+Arrays.asList(
+HoodieSchemaField.of("ts",
HoodieSchema.create(HoodieSchemaType.LONG), "ordering field doc", null),
+HoodieSchemaField.of("name",
HoodieSchema.create(HoodieSchemaType.STRING))
+)
+);
+ }
+
+ @Test
+ public void testCreateDeleteLogSchema() {
Review Comment:
Done in 2abcfe1e3a9c: the helper schema gains a STRING `seq` and a
`["long","null"]` `opt_ts`; cases added for `["ts","seq"]` (order and per-field
type), `emptyList()` (key only) and `opt_ts` (nullable LONG, no default,
pinning the current `HoodieSchemaField.of` behaviour for non-null-first unions).
--
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]
Re: [PR] refactor(schema): schema util contracts, dead API and folds [hudi]
voonhous commented on code in PR #19809:
URL: https://github.com/apache/hudi/pull/19809#discussion_r3916155984
##
hudi-client/hudi-spark-client/src/main/scala/org/apache/spark/sql/HoodieInternalRowUtils.scala:
##
@@ -123,7 +123,7 @@ object HoodieInternalRowUtils {
/**
* Get or create [[StructType]] for provided [[Schema]]
- * @param schema [[Schema]] to convert to [[StructType]], NOTE: It is best
that the schema passed in is cached through
[[org.apache.hudi.common.avro.AvroSchemaCache]], so that we can reduce the
overhead of schema lookup in the map
+ * @param schema [[Schema]] to convert to [[StructType]], NOTE: It is best
that the schema passed in is cached through
[[org.apache.hudi.common.schema.HoodieAvroSchemaCache]], so that we can reduce
the overhead of schema lookup in the map
* @return [[StructType]] for provided [[Schema]]
Review Comment:
Done in d74b3cd4f3b6.
##
hudi-common/src/test/java/org/apache/hudi/common/avro/TestHoodieAvroUtils.java:
##
@@ -566,6 +567,129 @@ public void
testConvertValueForAvroLogicalTypesCrossAvroVersion() {
assertEquals(FIXTURE_EPOCH_MICROS,
HoodieAvroUtils.convertValueForAvroLogicalTypes(LOCAL_TS_MICROS_SCHEMA,
FIXTURE_LOCAL_DT_MICROS, false));
}
+ @Test
+ public void testConvertValueForSpecificDataTypes_NullSchema() {
+// Test with null schema - should return value unchanged
+String testValue = "test_value";
+Object result = HoodieAvroUtils.convertValueForSpecificDataTypes(null,
testValue, false);
+assertEquals(testValue, result);
+ }
+
+ @Test
+ public void testConvertValueForSpecificDataTypes_NullValue_NullableSchema() {
+// Test with null value and nullable schema - should return null
+Schema nullableIntSchema =
HoodieSchema.createNullable(HoodieSchema.create(HoodieSchemaType.INT)).toAvroSchema();
+Object result =
HoodieAvroUtils.convertValueForSpecificDataTypes(nullableIntSchema, null,
false);
+assertNull(result);
+ }
+
+ @Test
+ public void
testConvertValueForSpecificDataTypes_NullValue_NonNullableSchema() {
+// Test with null value and non-nullable schema - should throw exception
+Schema nonNullableSchema = Schema.create(Schema.Type.STRING);
+assertThrows(IllegalStateException.class, () ->
+HoodieAvroUtils.convertValueForSpecificDataTypes(nonNullableSchema,
null, false));
+ }
+
+ @Test
+ public void testConvertValueForSpecificDataTypes_DateLogicalType() {
+// Create date schema
+Schema dateSchema = HoodieSchema.createDate().toAvroSchema();
+
+// Test value: epoch days for 2023-01-01
+int epochDays = 19358;
+Object result =
HoodieAvroUtils.convertValueForSpecificDataTypes(dateSchema, epochDays, false);
+assertNotNull(result);
+assertTrue(result instanceof LocalDate);
+assertEquals(LocalDate.of(2023, 1, 1), result);
+ }
+
+ @Test
+ public void testConvertValueForSpecificDataTypes_TimestampMillis_Enabled() {
+// Create timestamp-millis schema
+Schema timestampMillisSchema =
HoodieSchema.createTimestampMillis().toAvroSchema();
+
+// Test value: milliseconds for 2023-01-01 00:00:00
+long millis = 167256000L;
+Object result =
HoodieAvroUtils.convertValueForSpecificDataTypes(timestampMillisSchema, millis,
true);
+assertNotNull(result);
+assertTrue(result instanceof Timestamp);
+assertEquals(new Timestamp(millis), result);
+ }
+
+ @Test
+ public void testConvertValueForSpecificDataTypes_TimestampMillis_Disabled() {
+// Create timestamp-millis schema
+Schema timestampMillisSchema =
HoodieSchema.createTimestampMillis().toAvroSchema();
+long millis = 167256000L;
+Object result =
HoodieAvroUtils.convertValueForSpecificDataTypes(timestampMillisSchema, millis,
false);
+assertEquals(millis, result);
+ }
+
+ @Test
+ public void testConvertValueForSpecificDataTypes_TimestampMicros_Enabled() {
+// Create timestamp-micros schema
+Schema timestampMicrosSchema =
HoodieSchema.createTimestampMicros().toAvroSchema();
+
+// Test value: microseconds for 2023-01-01 00:00:00
+long micros = 16725600L;
+Object result =
HoodieAvroUtils.convertValueForSpecificDataTypes(timestampMicrosSchema, micros,
true);
+assertNotNull(result);
+assertTrue(result instanceof Timestamp);
+assertEquals(new Timestamp(micros / 1000), result);
+ }
+
+ @Test
+ public void testConvertValueForSpecificDataTypes_DecimalBytes() {
+// Create decimal schema with precision=10, scale=2
+Schema decimalSchema = HoodieSchema.createDecimal(10, 2).toAvroSchema();
+
+// Create test value: 1234.56
+BigDecimal expectedDecimal = new BigDecimal("1234.56");
+ByteBuffer byteBuffer =
ByteBuffer.wrap(expectedDecimal.unscaledValue().toByteArray());
+Object result =
HoodieAvroUtils.convertValueForSpecificDataTypes(decimalSchema, byteBuffer,
false);
+assertNotNull(result);
+assertTrue(result instanceof
Re: [PR] refactor(schema): schema util contracts, dead API and folds [hudi]
voonhous commented on code in PR #19809:
URL: https://github.com/apache/hudi/pull/19809#discussion_r3916141747
##
hudi-common/src/test/java/org/apache/hudi/common/avro/TestHoodieAvroUtils.java:
##
@@ -955,6 +1079,21 @@ void
testGetSortColumnValuesWithPartitionPathAndRecordKey(boolean suffixRecordKe
}
}
+ @Test
+ void testGetRecordColumnValues() {
Review Comment:
Done in 2abcfe1e3a9c. Rather than a shared helper, the test now uses
`SCHEMA_WITH_NESTED_FIELD_STR`, so nothing is duplicated and it pins a nested
`student.firstnameNested`, a nested null and a missing column (null).
##
hudi-common/src/main/java/org/apache/hudi/common/schema/LocalHoodieSchemaCache.java:
##
@@ -46,7 +46,10 @@ private LocalHoodieSchemaCache() {
this.schemaToVersionId = new HashMap<>();
}
- public static LocalHoodieSchemaCache getInstance() {
+ /**
+ * Returns a new, empty cache. Each caller owns its own version-id space.
+ */
+ public static LocalHoodieSchemaCache create() {
Review Comment:
Done in 2abcfe1e3a9c: `TestLocalHoodieSchemaCache` restores the two tests
from `TestLocalAvroSchemaCache` (four distinct schemas get four ids; two parses
of the same schema share one id).
--
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]
Re: [PR] refactor(schema): schema util contracts, dead API and folds [hudi]
voonhous commented on code in PR #19809:
URL: https://github.com/apache/hudi/pull/19809#discussion_r3916144952
##
hudi-common/src/main/java/org/apache/hudi/common/schema/HoodieSchemaUtils.java:
##
@@ -731,11 +721,41 @@ public static HoodieSchema
getRecordKeyPartitionPathSchema() {
return HoodieSchema.createRecord("HoodieRecordKey", "", "", false,
toBeAddedFields);
}
+ /**
+ * Schema of a native delete log record: the record key plus the ordering
fields, which are
+ * always nullable (see the comment in the body).
+ */
+ public static HoodieSchema createDeleteLogSchema(HoodieSchema tableSchema,
List orderingFieldNames) {
+// Native delete logs store only the record key plus optional ordering
values, so ordering fields in
+// the delete-log schema must always be nullable even when the table
schema marks them required.
+// A delete record such as HoodieEmptyRecord may carry
OrderingValues.getDefault() as an in-memory
+// sentinel rather than a real field value. Persist NULL for that missing
value so readers can map it
+// back to the default ordering without confusing it with a real business
value such as 0.
+List fields = Stream.concat(
+Stream.of(createNewSchemaField(
+HoodieRecord.RECORD_KEY_METADATA_FIELD,
HoodieSchema.create(HoodieSchemaType.STRING), null, null)),
+orderingFieldNames.stream().map(orderingFieldName ->
tableSchema.getField(orderingFieldName)
+.map(field -> createNewSchemaField(
+field.name(), HoodieSchema.createNullable(field.schema()),
field.doc().orElse(null), HoodieSchema.NULL_VALUE))
+.orElseThrow(() ->
+new IllegalArgumentException("Ordering field " +
orderingFieldName + " not found in table schema"
+.collect(Collectors.toList());
+return HoodieSchema.createRecord("hudi_delete_log_record", null, null,
fields);
+ }
+
/**
* Fetches projected schema given list of fields to project. The field can
be nested in format `a.b.c` where a is
- * the top level field, b is at second level and so on.
+ * the top level field, b is at second level and so on. Field names are
matched case-sensitively.
* This is equivalent to {@link HoodieAvroUtils#projectSchema(Schema, List)}
but operates on HoodieSchema.
*
+ * The two sibling projection helpers differ:
+ *
+ * {@link #generateProjectionSchema(HoodieSchema, List)} - top-level
fields only, matched
+ * case-insensitively
Review Comment:
Done in 2abcfe1e3a9c: `_ROW_KEY` projects to the field named `_row_key`.
--
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]
Re: [PR] refactor(schema): schema util contracts, dead API and folds [hudi]
voonhous commented on code in PR #19809:
URL: https://github.com/apache/hudi/pull/19809#discussion_r3916146578
##
hudi-common/src/main/java/org/apache/hudi/common/avro/HoodieAvroUtils.java:
##
@@ -883,15 +923,15 @@ private static Object
normalizeAvroLogicalTypeToPrimitive(Object value, Schema s
*
* @param record Hoodie record.
* @param columns Names of the columns to get values.
- * @param schema {@link Schema} instance.
+ * @param schema {@link HoodieSchema} instance.
* @return Column value.
*/
public static Object[] getRecordColumnValues(HoodieRecord record,
String[] columns,
- Schema schema,
+ HoodieSchema schema,
boolean
consistentLogicalTimestampEnabled) {
try {
- GenericRecord genericRecord = (GenericRecord)
(record.toIndexedRecord(HoodieAvroSchemaCache.intern(schema), new
Properties()).get()).getData();
+ GenericRecord genericRecord = (GenericRecord)
(record.toIndexedRecord(schema, new Properties()).get()).getData();
Review Comment:
Done in 23a1729823fb: `getRecordColumnValues` interns through
`HoodieSchemaCache` again, so the canonical instance reaches
`BaseAvroPayload#getRecord` as before.
##
hudi-common/src/main/java/org/apache/hudi/common/schema/HoodieSchemaUtils.java:
##
@@ -779,7 +799,8 @@ public static boolean hasDecimalField(HoodieSchema schema) {
* @param schema the input schema to search
* @return true if the schema contains a small precision decimal field and
false otherwise
*/
- public static boolean hasSmallPrecisionDecimalField(HoodieSchema schema) {
+ @VisibleForTesting
+ static boolean hasSmallPrecisionDecimalField(HoodieSchema schema) {
Review Comment:
Done in 1812ee46ad8d: both methods and `testHasSmallPrecisionDecimalField`
deleted; `hasDecimalWithCondition` folded into `hasDecimalField` since the
predicate had one remaining value.
--
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]
Re: [PR] refactor(schema): schema util contracts, dead API and folds [hudi]
voonhous commented on code in PR #19809:
URL: https://github.com/apache/hudi/pull/19809#discussion_r3915867255
##
hudi-common/src/main/java/org/apache/hudi/common/schema/HoodieSchemaUtils.java:
##
@@ -731,11 +721,41 @@ public static HoodieSchema
getRecordKeyPartitionPathSchema() {
return HoodieSchema.createRecord("HoodieRecordKey", "", "", false,
toBeAddedFields);
}
+ /**
+ * Schema of a native delete log record: the record key plus the ordering
fields, which are
+ * always nullable (see the comment in the body).
+ */
+ public static HoodieSchema createDeleteLogSchema(HoodieSchema tableSchema,
List orderingFieldNames) {
+// Native delete logs store only the record key plus optional ordering
values, so ordering fields in
+// the delete-log schema must always be nullable even when the table
schema marks them required.
+// A delete record such as HoodieEmptyRecord may carry
OrderingValues.getDefault() as an in-memory
+// sentinel rather than a real field value. Persist NULL for that missing
value so readers can map it
+// back to the default ordering without confusing it with a real business
value such as 0.
+List fields = Stream.concat(
+Stream.of(createNewSchemaField(
+HoodieRecord.RECORD_KEY_METADATA_FIELD,
HoodieSchema.create(HoodieSchemaType.STRING), null, null)),
+orderingFieldNames.stream().map(orderingFieldName ->
tableSchema.getField(orderingFieldName)
Review Comment:
Filed an issue for. this here, will fix it outside of this PR and keep this
PR purely for refactoring.
https://github.com/apache/hudi/issues/19823
--
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]
Re: [PR] refactor(schema): schema util contracts, dead API and folds [hudi]
voonhous commented on code in PR #19809:
URL: https://github.com/apache/hudi/pull/19809#discussion_r3914749475
##
hudi-common/src/main/java/org/apache/hudi/common/schema/HoodieSchemaUtils.java:
##
@@ -731,11 +721,41 @@ public static HoodieSchema
getRecordKeyPartitionPathSchema() {
return HoodieSchema.createRecord("HoodieRecordKey", "", "", false,
toBeAddedFields);
}
+ /**
+ * Schema of a native delete log record: the record key plus the ordering
fields, which are
+ * always nullable (see the comment in the body).
+ */
+ public static HoodieSchema createDeleteLogSchema(HoodieSchema tableSchema,
List orderingFieldNames) {
+// Native delete logs store only the record key plus optional ordering
values, so ordering fields in
+// the delete-log schema must always be nullable even when the table
schema marks them required.
+// A delete record such as HoodieEmptyRecord may carry
OrderingValues.getDefault() as an in-memory
+// sentinel rather than a real field value. Persist NULL for that missing
value so readers can map it
+// back to the default ordering without confusing it with a real business
value such as 0.
+List fields = Stream.concat(
+Stream.of(createNewSchemaField(
+HoodieRecord.RECORD_KEY_METADATA_FIELD,
HoodieSchema.create(HoodieSchemaType.STRING), null, null)),
+orderingFieldNames.stream().map(orderingFieldName ->
tableSchema.getField(orderingFieldName)
Review Comment:
**major:** pre-existing, not introduced by this move.
`tableSchema.getField(name)` is an exact top-level lookup
(`HoodieSchema.java:1215`), so a nested ordering field such as
`nested_record.level` throws `IllegalArgumentException` here on the first
delete of a v10 native-log table: nested precombine is supported (HUDI-4051)
and `HoodieNativeLogAppendHandle.java:93` passes the names through verbatim.
Out of scope for a behavior-preserving PR; should the fix (writer
`createDeleteLogFieldValues` and reader `getValue` aligned on a flat field
name) go in a separate PR, with a note on #16639 meanwhile?
##
hudi-common/src/test/java/org/apache/hudi/common/schema/TestHoodieSchemaUtils.java:
##
@@ -2198,4 +2085,48 @@ public void testGetNestedFieldComplexNestedMapAndArray()
{
assertEquals("value", result.get().getRight().name());
assertEquals(HoodieSchemaType.LONG,
result.get().getRight().schema().getType());
}
+
+ private static HoodieSchema deleteLogTableSchema() {
+return HoodieSchema.createRecord(
+"TestRecord",
+null,
+null,
+Arrays.asList(
+HoodieSchemaField.of("ts",
HoodieSchema.create(HoodieSchemaType.LONG), "ordering field doc", null),
+HoodieSchemaField.of("name",
HoodieSchema.create(HoodieSchemaType.STRING))
+)
+);
+ }
+
+ @Test
+ public void testCreateDeleteLogSchema() {
Review Comment:
**minor:** not blocking. Every caller and test passes one LONG ordering
field, so an implementation that hardcoded LONG or kept only the first field
would still pass; order is load-bearing since
`HoodieNativeLogFormatWriter.java:186-199` fills `fieldValues[i + 1]` by
position. An already-nullable `["long","null"]` field also differs:
`HoodieSchemaField.of` drops the NULL default for a non-null-first union, so
`defaultVal()` comes back empty. Could the helper schema gain a STRING `seq`
and a `["long","null"]` field, with cases for `["ts","seq"]` and `emptyList()`?
##
hudi-common/src/main/java/org/apache/hudi/common/schema/HoodieSchemaUtils.java:
##
@@ -731,11 +721,41 @@ public static HoodieSchema
getRecordKeyPartitionPathSchema() {
return HoodieSchema.createRecord("HoodieRecordKey", "", "", false,
toBeAddedFields);
}
+ /**
+ * Schema of a native delete log record: the record key plus the ordering
fields, which are
+ * always nullable (see the comment in the body).
+ */
+ public static HoodieSchema createDeleteLogSchema(HoodieSchema tableSchema,
List orderingFieldNames) {
+// Native delete logs store only the record key plus optional ordering
values, so ordering fields in
+// the delete-log schema must always be nullable even when the table
schema marks them required.
+// A delete record such as HoodieEmptyRecord may carry
OrderingValues.getDefault() as an in-memory
+// sentinel rather than a real field value. Persist NULL for that missing
value so readers can map it
+// back to the default ordering without confusing it with a real business
value such as 0.
+List fields = Stream.concat(
+Stream.of(createNewSchemaField(
+HoodieRecord.RECORD_KEY_METADATA_FIELD,
HoodieSchema.create(HoodieSchemaType.STRING), null, null)),
+orderingFieldNames.stream().map(orderingFieldName ->
tableSchema.getField(orderingFieldName)
+.map(field -> createNewSchemaField(
+field.name(),
Re: [PR] refactor(schema): schema util contracts, dead API and folds [hudi]
hudi-bot commented on PR #19809: URL: https://github.com/apache/hudi/pull/19809#issuecomment-5497868753 ## CI report: * 557a2743c60e6ee0217a8dfa951b8f7e195610a0 Azure: [SUCCESS](https://dev.azure.com/apachehudi/a1a51da7-8592-47d4-88dc-fd67bed336bb/_build/results?buildId=16745) Bot commands @hudi-bot supports the following commands: - `@hudi-bot run azure` re-run the last Azure build -- 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]
Re: [PR] refactor(schema): schema util contracts, dead API and folds [hudi]
hudi-bot commented on PR #19809: URL: https://github.com/apache/hudi/pull/19809#issuecomment-5496740033 ## CI report: * 5be7b58363beefa52ff136efbbc08ee6398b1331 Azure: [SUCCESS](https://dev.azure.com/apachehudi/a1a51da7-8592-47d4-88dc-fd67bed336bb/_build/results?buildId=16742) * 557a2743c60e6ee0217a8dfa951b8f7e195610a0 Azure: [PENDING](https://dev.azure.com/apachehudi/a1a51da7-8592-47d4-88dc-fd67bed336bb/_build/results?buildId=16745) Bot commands @hudi-bot supports the following commands: - `@hudi-bot run azure` re-run the last Azure build -- 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]
Re: [PR] refactor(schema): schema util contracts, dead API and folds [hudi]
hudi-bot commented on PR #19809: URL: https://github.com/apache/hudi/pull/19809#issuecomment-5496567398 ## CI report: * 5be7b58363beefa52ff136efbbc08ee6398b1331 Azure: [PENDING](https://dev.azure.com/apachehudi/a1a51da7-8592-47d4-88dc-fd67bed336bb/_build/results?buildId=16742) * 557a2743c60e6ee0217a8dfa951b8f7e195610a0 Azure: [PENDING](https://dev.azure.com/apachehudi/a1a51da7-8592-47d4-88dc-fd67bed336bb/_build/results?buildId=16745) Bot commands @hudi-bot supports the following commands: - `@hudi-bot run azure` re-run the last Azure build -- 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]
Re: [PR] refactor(schema): schema util contracts, dead API and folds [hudi]
hudi-bot commented on PR #19809: URL: https://github.com/apache/hudi/pull/19809#issuecomment-5496486794 ## CI report: * 5be7b58363beefa52ff136efbbc08ee6398b1331 Azure: [PENDING](https://dev.azure.com/apachehudi/a1a51da7-8592-47d4-88dc-fd67bed336bb/_build/results?buildId=16742) * 557a2743c60e6ee0217a8dfa951b8f7e195610a0 UNKNOWN Bot commands @hudi-bot supports the following commands: - `@hudi-bot run azure` re-run the last Azure build -- 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]
Re: [PR] refactor(schema): schema util contracts, dead API and folds [hudi]
codecov-commenter commented on PR #19809: URL: https://github.com/apache/hudi/pull/19809#issuecomment-5496098321 ## [Codecov](https://app.codecov.io/gh/apache/hudi/pull/19809?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report :x: Patch coverage is `45.8%` with `13 lines` in your changes missing coverage. Please review. :white_check_mark: Project coverage is 78.31%. Comparing base ([`c59987a`](https://app.codecov.io/gh/apache/hudi/commit/c59987a024cd021cbb9aa34bdc74d341ea834b4d?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`5be7b58`](https://app.codecov.io/gh/apache/hudi/commit/5be7b58363beefa52ff136efbbc08ee6398b1331?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)). :warning: Report is 1 commits behind head on master. | [Files with missing lines](https://app.codecov.io/gh/apache/hudi/pull/19809?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Patch % | Lines | |---|---|---| | [...a/org/apache/hudi/common/avro/HoodieAvroUtils.java](https://app.codecov.io/gh/apache/hudi/pull/19809?src=pr&el=tree&filepath=hudi-common%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fhudi%2Fcommon%2Favro%2FHoodieAvroUtils.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-aHVkaS1jb21tb24vc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2h1ZGkvY29tbW9uL2F2cm8vSG9vZGllQXZyb1V0aWxzLmphdmE=) | 16.66% | [5 Missing :warning: ](https://app.codecov.io/gh/apache/hudi/pull/19809?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | | [...g/apache/hudi/common/schema/HoodieSchemaUtils.java](https://app.codecov.io/gh/apache/hudi/pull/19809?src=pr&el=tree&filepath=hudi-common%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fhudi%2Fcommon%2Fschema%2FHoodieSchemaUtils.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-aHVkaS1jb21tb24vc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2h1ZGkvY29tbW9uL3NjaGVtYS9Ib29kaWVTY2hlbWFVdGlscy5qYXZh) | 60.00% | [4 Missing :warning: ](https://app.codecov.io/gh/apache/hudi/pull/19809?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | | [...n/avro/processors/DecimalLogicalTypeProcessor.java](https://app.codecov.io/gh/apache/hudi/pull/19809?src=pr&el=tree&filepath=hudi-common%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fhudi%2Fcommon%2Favro%2Fprocessors%2FDecimalLogicalTypeProcessor.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-aHVkaS1jb21tb24vc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2h1ZGkvY29tbW9uL2F2cm8vcHJvY2Vzc29ycy9EZWNpbWFsTG9naWNhbFR5cGVQcm9jZXNzb3IuamF2YQ==) | 0.00% | [1 Missing :warning: ](https://app.codecov.io/gh/apache/hudi/pull/19809?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | | [...org/apache/hudi/common/model/HoodieAvroRecord.java](https://app.codecov.io/gh/apache/hudi/pull/19809?src=pr&el=tree&filepath=hudi-common%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fhudi%2Fcommon%2Fmodel%2FHoodieAvroRecord.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-aHVkaS1jb21tb24vc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2h1ZGkvY29tbW9uL21vZGVsL0hvb2RpZUF2cm9SZWNvcmQuamF2YQ==) | 0.00% | [1 Missing :warning: ](https://app.codecov.io/gh/apache/hudi/pull/19809?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | | [...e/hudi/common/table/read/lsm/LsmFileIterators.java](https://app.codecov.io/gh/apache/hudi/pull/19809?src=pr&el=tree&filepath=hudi-common%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fhudi%2Fcommon%2Ftable%2Fread%2Flsm%2FLsmFileIterators.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-aHVkaS1jb21tb24vc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2h1ZGkvY29tbW9uL3RhYmxlL3JlYWQvbHNtL0xzbUZpbGVJdGVyYXRvcnMuamF2YQ==) | 0.00% | [1 Missing :warning: ](https://app.codecov.io/gh/apache/hudi/pull/19809?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | | [.../apache/hudi/metadata/HoodieTableMetadataUtil.java](https://app.codecov.io/gh/apache/hudi/pull/19809?src=pr&el=tree&filepath=hudi-common%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fhudi%2Fmetadata%2FHoodieTableMetadataUtil.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-aHVkaS1jb21tb24vc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2h1ZGkvbWV0YWRhdGEvSG9vZGllVGFibGVNZXRhZGF0YVV0aWwuamF2YQ==) |
Re: [PR] refactor(schema): schema util contracts, dead API and folds [hudi]
voonhous commented on code in PR #19809: URL: https://github.com/apache/hudi/pull/19809#discussion_r3905302470 ## hudi-common/src/main/java/org/apache/hudi/metadata/HoodieMetadataPayload.java: ## @@ -110,7 +109,7 @@ public class HoodieMetadataPayload implements HoodieRecordPayload HoodieSchema migration Review Comment: Reworded in 557a2743c60e. The field still anchors the `==` fast path in `getInsertValue` (line 428); `HoodieMetadataRecord.getClassSchema()` returns the generated class's `SCHEMA$` singleton, so the reference is stable without interning. ## hudi-common/src/main/java/org/apache/hudi/common/avro/HoodieAvroUtils.java: ## @@ -1581,15 +1631,17 @@ public static GenericRecord rewriteRecordDeep(GenericRecord oldRecord, Schema ne return rewriteRecordWithNewSchema(oldRecord, newSchema, Collections.EMPTY_MAP, validate); } + @VisibleForTesting Review Comment: Keeping it as is. `gteqAvro1_9` has test callers outside the package and outside the module (`TestHoodieLogFormat` in hudi-hadoop-common, `TestObjectSizeCalculator` in `common.util`), so package-private is not an option, and the annotation is what records that the method is public only for tests -- same pattern as `makeFieldNonNull` and `rewriteRecords` in this class. `gteqAvro1_10` and `gteqAvro1_12` could be narrowed because their only callers are in-package or in-class. -- 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]
Re: [PR] refactor(schema): schema util contracts, dead API and folds [hudi]
hudi-bot commented on PR #19809: URL: https://github.com/apache/hudi/pull/19809#issuecomment-5495541535 ## CI report: * 5be7b58363beefa52ff136efbbc08ee6398b1331 Azure: [PENDING](https://dev.azure.com/apachehudi/a1a51da7-8592-47d4-88dc-fd67bed336bb/_build/results?buildId=16742) Bot commands @hudi-bot supports the following commands: - `@hudi-bot run azure` re-run the last Azure build -- 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]
Re: [PR] refactor(schema): schema util contracts, dead API and folds [hudi]
hudi-agent commented on code in PR #19809: URL: https://github.com/apache/hudi/pull/19809#discussion_r3905140596 ## hudi-common/src/main/java/org/apache/hudi/common/avro/HoodieAvroUtils.java: ## @@ -1581,15 +1631,17 @@ public static GenericRecord rewriteRecordDeep(GenericRecord oldRecord, Schema ne return rewriteRecordWithNewSchema(oldRecord, newSchema, Collections.EMPTY_MAP, validate); } + @VisibleForTesting Review Comment: 🤖 nit: `@VisibleForTesting` on a `public` method is a bit contradictory — the sibling `gteqAvro1_10` was correctly narrowed to package-private to match the annotation's intent. Could `gteqAvro1_9` follow the same treatment, or if it has real external callers, drop the annotation? ⚠️ AI-generated; verify before applying. React 👍/👎 to flag quality. ## hudi-common/src/main/java/org/apache/hudi/metadata/HoodieMetadataPayload.java: ## @@ -110,7 +109,7 @@ public class HoodieMetadataPayload implements HoodieRecordPayload HoodieSchema migration Review Comment: 🤖 nit: the comment above still says 'Cache the Avro schema reference for O(1) equality checks' but `AvroSchemaCache.intern()` was removed on the next line — could you update or drop it so it matches what the code actually does? ⚠️ AI-generated; verify before applying. React 👍/👎 to flag quality. -- 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]
