voonhous commented on code in PR #19855:
URL: https://github.com/apache/hudi/pull/19855#discussion_r3965344284
##########
hudi-common/src/main/java/org/apache/hudi/common/schema/HoodieSchemaUtils.java:
##########
@@ -520,6 +520,49 @@ 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
+ * @return new HoodieSchema containing only the specified fields
+ */
+ public static HoodieSchema generateProjectionSchema(HoodieSchema writeSchema,
Review Comment:
**major:** This leaves two byte-identical copies of the lookup loop in one
class: the 2-arg overload's at `:510-518` and this one at `:552-560`, same
error string included. #19835 (a7deb61f7426) had to patch this exact
`Locale.ROOT` lookup in both files but pinned only the 2-arg copy
(`testGenerateProjectionSchemaIgnoresDefaultLocale`), so the realtime reader's
path is still untested. Could the 2-arg build its map and delegate here, so
there is one loop and that test covers both callers?
<details>
<summary>Why the isError delta does not block delegating</summary>
The only behavioural difference is `writeSchema.isError()`, and it is always
`false` at the sole caller: `AbstractRealtimeRecordReader:177` runs
`addPartitionFields` unconditionally, which rebuilds the record through
`appendFieldsToSchemaBase` and `createNewSchemaFromFieldsWithReference`, and
that uses the 4-arg `createRecord`, which hard-codes `isError=false`. #19841
asked for the flag to be carried over without knowing it is dead here.
Delegating makes the 2-arg preserve `isError` for its other callers, which is
inert since no Hudi table schema is an Avro error record.
</details>
##########
hudi-common/src/main/java/org/apache/hudi/common/schema/HoodieSchemaUtils.java:
##########
@@ -520,6 +520,49 @@ 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
Review Comment:
**minor:** "prebuilt case-insensitive field-name map" understates the
contract now that this is public: the lookup at `:553` needs keys already
lowercased with `Locale.ROOT`. Same-typed original-case maps exist in the tree
(`SparkDataSourceTableUtils:57-60`), and passing one fails every mixed-case
name with "Field X not found in log schema", the #19835 symptom. Could the
`@param` spell that out? Not blocking.
```suggestion
* @param schemaFieldsMap field map keyed by names lowercased with {@code
Locale.ROOT}, as built by
* {@code
HoodieRealtimeRecordReaderUtils#getNameToFieldMap}
```
##########
hudi-common/src/main/java/org/apache/hudi/common/schema/HoodieSchemaUtils.java:
##########
@@ -1001,4 +1044,19 @@ private static HoodieSchema
appendFieldsToSchemaBase(HoodieSchema schema, List<H
return createNewSchemaFromFieldsWithReference(schema, fields);
}
+
+ /**
+ * Checks if a schema field is of type timestamp_millis (timestamp-millis or
local-timestamp-millis).
+ *
+ * @param fieldSchema The schema of the field to check
+ * @return true if the field is of type timestamp_millis, false otherwise
+ */
+ public static boolean isTimestampMillisField(HoodieSchema fieldSchema) {
Review Comment:
**minor:** Now that this is public, two union behaviours are undocumented:
`["null","string","timestamp-millis"]` returns `false` because
`getNonNullType()` yields a UNION, and `["null"]` throws IAE from
`createUnion`. Not live today (`isColumnTypeSupported` denies UNION), but
#19834 fixed exactly this pattern in the sibling
`HoodieSchemaRepair#hasTimestampMillisField` three days ago, so after merge the
two siblings answer multi-branch unions differently. Could the javadoc state
that multi-branch unions answer `false`? Not blocking.
##########
hudi-common/src/main/java/org/apache/hudi/common/schema/HoodieSchemaUtils.java:
##########
@@ -520,6 +520,49 @@ public static HoodieSchema
generateProjectionSchema(HoodieSchema originalSchema,
return HoodieSchema.createRecord(originalSchema.getName(),
originalSchema.getNamespace().orElse(null),
originalSchema.getDoc().orElse(null), projectedFields);
Review Comment:
**minor:** The javadoc for this method (line 490) still says "see
`HoodieRealtimeRecordReaderUtils#generateProjectionSchema`" for the
Hive-lowercasing rationale, and this PR deletes that method; the sentence came
in with #19835 four days ago. Could it point at `{@link
#generateProjectionSchema(HoodieSchema, Map, List)}` instead, now that the
sibling lives in this class? Not blocking.
##########
hudi-common/src/main/java/org/apache/hudi/common/schema/HoodieSchemaUtils.java:
##########
@@ -520,6 +520,49 @@ public static HoodieSchema
generateProjectionSchema(HoodieSchema originalSchema,
return HoodieSchema.createRecord(originalSchema.getName(),
originalSchema.getNamespace().orElse(null),
originalSchema.getDoc().orElse(null), projectedFields);
}
+ /**
Review Comment:
**nit:** The contract indexes #19809 added are now stale: the class-level
"projection and pruning" bullet (line 60) and the "two sibling projection
helpers" block on `projectSchema` (line 828) list only the 2-arg overload, and
`isTimestampMillisField` is missing from the predicates bullet next to
`hasDecimalField` (line 75). Could both new public methods be registered there?
Feel free to ignore.
##########
hudi-common/src/main/java/org/apache/hudi/common/schema/HoodieSchemaUtils.java:
##########
@@ -520,6 +520,49 @@ 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
+ * @return new HoodieSchema containing only the specified fields
+ */
+ public static HoodieSchema generateProjectionSchema(HoodieSchema writeSchema,
+ Map<String,
HoodieSchemaField> schemaFieldsMap,
+ List<String> fieldNames)
{
+ ValidationUtils.checkArgument(writeSchema != null, "Write schema cannot be
null");
+ ValidationUtils.checkArgument(schemaFieldsMap != null, "Schema fields map
cannot be null");
+ ValidationUtils.checkArgument(fieldNames != null, "Field names cannot be
null");
+
+ /*
+ * Avro & Presto field names seems to be case sensitive (support fields
differing only in case) whereas
+ * Hive/Impala/SparkSQL(default) are case-insensitive. Spark allows this
to be configurable using
+ * spark.sql.caseSensitive=true
+ *
+ * For a RT table setup with no delta-files (for a latest file-slice) ->
we translate parquet schema to Avro Here
+ * the field-name case is dependent on parquet schema. Hive (1.x/2.x/CDH)
translate column projections to
+ * lower-cases
+ *
+ */
+ List<HoodieSchemaField> projectedFields = new
ArrayList<>(fieldNames.size());
+ for (String fn : fieldNames) {
+ HoodieSchemaField field =
schemaFieldsMap.get(fn.toLowerCase(Locale.ROOT));
+ if (field == null) {
+ throw new HoodieException("Field " + fn + " not found in log schema.
Query cannot proceed! "
+ + "Derived Schema Fields: " + new
ArrayList<>(schemaFieldsMap.keySet()));
Review Comment:
**nit:** Continuation lines here and at `:563` indent 16/12 spaces where the
sibling overload uses 12, and the javadoc drops the `@since` and `@throws` tags
the 2-arg carries. Checkstyle passes, so purely cosmetic. Feel free to ignore.
##########
hudi-common/src/test/java/org/apache/hudi/common/schema/TestHoodieSchemaUtils.java:
##########
@@ -2370,4 +2370,32 @@ public void
testCreateDeleteLogSchemaWithUnknownOrderingField() {
() -> HoodieSchemaUtils.createDeleteLogSchema(tableSchema,
Collections.singletonList("not_a_field")));
assertEquals("Ordering field not_a_field not found in table schema",
exception.getMessage());
}
+
+ @Test
+ void testIsTimestampMillisField() {
Review Comment:
**minor:** The javadoc on `isTimestampMillisField` names
local-timestamp-millis, but no assertion here exercises it. The claim holds
(`createLocalTimestampMillis()` builds a `HoodieSchema.Timestamp`), so it is a
free assertion. Could we add `createLocalTimestampMillis()` expecting `true`
and `createLocalTimestampMicros()` expecting `false`? Not blocking.
##########
hudi-common/src/main/java/org/apache/hudi/common/schema/HoodieSchemaRepair.java:
##########
@@ -39,8 +39,8 @@
* {@code
org.apache.hudi.common.schema.internal.utils.AvroSchemaEvolutionUtils}.</p>
*
* <p>{@link #hasTimestampMillisField(HoodieSchema)} is the cheap pre-check
used to decide whether the
- * repair is worth wiring in at all. Its sibling in the metadata-table domain
is
- * {@code HoodieTableMetadataUtil#isTimestampMillisField}, which answers the
same question for one field
+ * repair is worth wiring in at all. Its per-field sibling is
+ * {@code HoodieSchemaUtils#isTimestampMillisField}, which answers the same
question for one field
Review Comment:
**nit:** The PR body still describes the earlier direction: "Reuse
`HoodieTableMetadataUtil.isTimestampMillisField` in `HoodieSchemaRepair`" and
"make the timestamp-millis predicate accessible across the relevant packages",
which is what 9cb20ca8 reversed. Since the body is what a committer reads at
merge time, could it say the predicate moved into `HoodieSchemaUtils` with both
callers delegating? Feel free to ignore.
##########
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);
Review Comment:
**nit:** `constructHiveOrderedSchema` just below is the third twin of this
loop (both were born in 12523c379fe9, HUDI-298): same lowercased-map lookup,
same `createNewSchemaField`, same 5-arg `createRecord`, differing only in
skipping Hive virtual columns instead of throwing. Leaving it is reasonable
given the skip semantics; is it deliberately out of #19841's scope? Feel free
to ignore.
--
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]
