hudi-agent commented on code in PR #19123:
URL: https://github.com/apache/hudi/pull/19123#discussion_r3501783792
##########
hudi-spark-datasource/hudi-spark-common/src/main/scala/org/apache/spark/sql/execution/datasources/parquet/HoodieFileGroupReaderBasedFileFormat.scala:
##########
@@ -253,7 +255,11 @@ class HoodieFileGroupReaderBasedFileFormat(tablePath:
String,
val augmentedStorageConf = new
HadoopStorageConfiguration(hadoopConf).getInline
setSchemaEvolutionConfigs(augmentedStorageConf)
augmentedStorageConf.set(ENABLE_LOGICAL_TIMESTAMP_REPAIR,
hasTimestampMillisFieldInTableSchema.toString)
- val (remainingPartitionSchemaArr, fixedPartitionIndexesArr) =
partitionSchema.fields.toSeq.zipWithIndex.filter(p =>
!mandatoryFields.contains(p._1.name)).unzip
+ // Nested partition columns (e.g. "nested_record.level") are never read
from the data file: the
+ // flattened dotted name is not a valid top-level field and the value is
materialized from the
+ // partition path. Always keep them in the appended ("remaining")
partition fields so they are
+ // not converted into a top-level Avro field below, which would fail Avro
name validation.
+ val (remainingPartitionSchemaArr, fixedPartitionIndexesArr) =
partitionSchema.fields.toSeq.zipWithIndex.filter(p =>
!mandatoryFields.contains(p._1.name) || isNestedPartitionField(p._1.name)).unzip
Review Comment:
🤖 Now that a nested partition field can be appended (remaining) while a
top-level partition field is still read from the file, the snapshot/MOR path
hits `appendPartitionAndProject`, which calls
`getFixedPartitionValues(partitionValues, remainingPartitionSchema,
fixedPartitionIndexes)` (line 426). But `fixedPartitionIndexes` are positions
in the *full* `partitionSchema`, and `getFixedPartitionValues` does
`allPartitionValues.toSeq(partitionSchema)` — the base-file path correctly
passes the full schema (line 529), whereas here it gets
`remainingPartitionSchema`. Could a nested partition field mixed with a
top-level field that's read from the file (e.g. timestamp keygen over multiple
partition cols) drop/misread the nested value here? @nsivabalan could you
verify this path?
<sub><i>⚠️ AI-generated; verify before applying. React 👍/👎 to flag
quality.</i></sub>
##########
hudi-spark-datasource/hudi-spark-common/src/main/scala/org/apache/spark/sql/execution/datasources/parquet/HoodieFileGroupReaderBasedFileFormat.scala:
##########
@@ -265,9 +271,9 @@ class HoodieFileGroupReaderBasedFileFormat(tablePath:
String,
val exclusionFields = new java.util.HashSet[String]()
exclusionFields.add("op")
partitionSchema.fields.foreach(f => exclusionFields.add(f.name))
- val requestedStructType = StructType(requiredSchema.fields ++
partitionSchema.fields.filter(f => mandatoryFields.contains(f.name)))
+ val requestedStructType = StructType(requiredSchema.fields ++
partitionSchema.fields.filter(f => mandatoryFields.contains(f.name) &&
!isNestedPartitionField(f.name)))
Review Comment:
🤖 A partition field lands in `mandatoryFields` precisely because its
partition-path encoding isn't the true column value (precombine, or variable
timestamp/custom keygen), so it's read from the file. By excluding nested
mandatory fields here and appending them from the partition path instead, do we
risk the output value being the path-encoded form (or null on a type mismatch)
for those keygen cases? The merge precombine value can still come from the
nested struct read via the data schema, but the appended flat partition column
value would now be path-derived — is that intended?
<sub><i>⚠️ AI-generated; verify before applying. React 👍/👎 to flag
quality.</i></sub>
##########
hudi-spark-datasource/hudi-spark-common/src/main/scala/org/apache/spark/sql/execution/datasources/parquet/HoodieFileGroupReaderBasedFileFormat.scala:
##########
@@ -210,7 +210,9 @@ class HoodieFileGroupReaderBasedFileFormat(tablePath:
String,
}
originalVectorTypes.map {
o: Seq[String] => o.zipWithIndex.map(a => {
- if (a._2 >= requiredSchema.length &&
mandatoryFields.contains(partitionSchema.fields(a._2 -
requiredSchema.length).name)) {
+ if (a._2 >= requiredSchema.length
Review Comment:
🤖 nit: `partitionSchema.fields(a._2 - requiredSchema.length).name` is
evaluated twice in this condition — could you pull it into a `val fieldName =
...` before the `if` to make the predicate easier to scan?
<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]
