hudi-agent commented on code in PR #19855:
URL: https://github.com/apache/hudi/pull/19855#discussion_r3945460888
##########
hudi-common/src/main/java/org/apache/hudi/common/schema/HoodieSchemaRepair.java:
##########
@@ -249,8 +250,7 @@ public static boolean hasTimestampMillisField(HoodieSchema
tableSchema) {
return hasTimestampMillisField(tableSchema.getNonNullType());
case TIMESTAMP:
- HoodieSchema.Timestamp timestampType = (HoodieSchema.Timestamp)
tableSchema;
- return timestampType.getPrecision() ==
HoodieSchema.TimePrecision.MILLIS;
+ return HoodieTableMetadataUtil.isTimestampMillisField(tableSchema);
Review Comment:
🤖 This makes the base `common.schema` package depend on
`org.apache.hudi.metadata.HoodieTableMetadataUtil`, which already imports
`HoodieSchemaUtils` and pulls in a large transitive graph (meta client,
records, engine context). Would it make sense to invert this — move
`isTimestampMillisField` into `HoodieSchemaUtils` (or `HoodieSchema.Timestamp`)
and have `HoodieTableMetadataUtil` delegate to it — so the dependency flows
from metadata → schema rather than the other way around?
<sub><i>⚠️ AI-generated; verify before applying. React 👍/👎 to flag
quality.</i></sub>
##########
hudi-common/src/main/java/org/apache/hudi/common/schema/HoodieSchemaUtils.java:
##########
@@ -520,6 +520,41 @@ public static HoodieSchema
generateProjectionSchema(HoodieSchema originalSchema,
return HoodieSchema.createRecord(originalSchema.getName(),
originalSchema.getNamespace().orElse(null),
originalSchema.getDoc().orElse(null), projectedFields);
}
+ /**
+ * Generate a reader schema off the provided writeSchema, to just project
out the provided columns.
+ *
+ * <p>This overload is intended for callers that already have a
name-to-field map,
+ * such as the realtime reader.</p>
+ *
+ * @param writeSchema the source schema
+ * @param schemaFieldsMap prebuilt case-insensitive field-name map
+ * @param fieldNames the list of field names to include in the
projection
+ * @param isError whether the projected schema is an error schema
+ * @return new HoodieSchema containing only the specified fields
+ */
+ public static HoodieSchema generateProjectionSchema(HoodieSchema writeSchema,
+ Map<String,
HoodieSchemaField> schemaFieldsMap,
+ List<String> fieldNames,
+ boolean isError) {
+ ValidationUtils.checkArgument(writeSchema != null, "Write schema cannot be
null");
+ ValidationUtils.checkArgument(schemaFieldsMap != null, "Schema fields map
cannot be null");
Review Comment:
🤖 nit: the original method had a useful comment explaining why the field
lookup is case-insensitive (Avro/Presto are case-sensitive while
Hive/Impala/SparkSQL default to case-insensitive). Worth carrying that context
over here since it got dropped in the move.
<sub><i>⚠️ AI-generated; verify before applying. React 👍/👎 to flag
quality.</i></sub>
##########
hudi-hadoop-mr/src/main/java/org/apache/hudi/hadoop/realtime/AbstractRealtimeRecordReader.java:
##########
@@ -183,7 +183,7 @@ private void init() throws Exception {
// TODO(vc): In the future, the reader schema should be updated based on
log files & be able
// to null out fields not present before
- readerSchema =
HoodieRealtimeRecordReaderUtils.generateProjectionSchema(writerSchema,
schemaFieldsMap, projectionFields);
+ readerSchema = HoodieSchemaUtils.generateProjectionSchema(writerSchema,
schemaFieldsMap, projectionFields,writerSchema.isError());
Review Comment:
🤖 nit: missing space after the comma —
`projectionFields,writerSchema.isError())`.
<sub><i>⚠️ AI-generated; verify before applying. React 👍/👎 to flag
quality.</i></sub>
--
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]
