voonhous commented on code in PR #19713:
URL: https://github.com/apache/hudi/pull/19713#discussion_r3851475707
##########
hudi-spark-datasource/hudi-spark/src/test/scala/org/apache/spark/sql/hudi/dml/others/TestPartialUpdateForMergeInto.scala:
##########
@@ -732,12 +732,144 @@ class TestPartialUpdateForMergeInto extends
HoodieSparkSqlTestBase {
}
}
+ // A partial update names only the columns being changed, so the record key
is normally absent
+ // from the assignments. On MOR the global-index tagging stage merges the
incoming record with its
+ // existing version and then asks the key generator for the merged record's
partition path - and
+ // that merged record is materialised against WRITE_PARTIAL_UPDATE_SCHEMA,
which carries only the
+ // assigned columns. Resolving the partition path must therefore not also
require the record key.
+ //
+ // The index types split on whether that tagging stage runs at all:
GLOBAL_BLOOM and GLOBAL_SIMPLE
+ // set mayContainDuplicateLookup on MOR and so reach it, while the
record-index spellings pass
+ // false and short-circuit. Both cells are covered, so the fix is pinned
where it applies and the
+ // already-working path is guarded against regression. The source projects
the partition column in
+ // every case, to keep this independent of partition-column resolution
(ENG-46864).
+ Seq(
+ ("GLOBAL_BLOOM re-keying", Map(
+ "hoodie.index.type" -> "GLOBAL_BLOOM",
+ "hoodie.bloom.index.update.partition.path" -> "false")),
+ ("GLOBAL_SIMPLE re-keying", Map(
+ "hoodie.index.type" -> "GLOBAL_SIMPLE",
+ "hoodie.simple.index.update.partition.path" -> "false")),
+ ("RECORD_INDEX", Map(
+ "hoodie.index.type" -> "RECORD_INDEX",
+ "hoodie.record.index.update.partition.path" -> "false",
+ "hoodie.metadata.enable" -> "true",
+ "hoodie.metadata.record.index.enable" -> "true")),
+ ("GLOBAL_RECORD_LEVEL_INDEX", Map(
+ "hoodie.index.type" -> "GLOBAL_RECORD_LEVEL_INDEX",
+ "hoodie.record.index.update.partition.path" -> "false",
+ "hoodie.metadata.enable" -> "true",
+ "hoodie.metadata.record.index.enable" -> "true"))
Review Comment:
This cell is a runtime duplicate of `RECORD_INDEX`:
`SparkHoodieIndexFactory.java:65-67` maps both spellings to
`SparkMetadataTableGlobalRecordLevelIndex`, reading the same
`hoodie.record.index.update.partition.path`. Meanwhile nothing in the matrix
observes `partitionResolvableFromRecord`: with `update.partition.path=false`
the merged key is reset to the existing key whatever the keygen returned. The
one cell where a wrong `__HIVE_DEFAULT_PARTITION__` is visible (delete against
the real partition, partial record inserted into the default one) is this
spelling with `update.partition.path=true`, which the NOTE below names and
skips; #19610 is the open PR adding the enum mapping.
Could we make this cell the `update.partition.path=true` variant with a
source `dt` that differs, asserting the record stays in its partition, and cite
#19610 so the cell is retired when the mapping lands?
##########
hudi-client/hudi-client-common/src/main/java/org/apache/hudi/index/HoodieIndexUtils.java:
##########
@@ -531,7 +591,8 @@ public static <R> HoodieData<HoodieRecord<R>>
mergeForPartitionUpdatesAndDeletio
HoodieRecord<R> existing = existingOpt.get();
Option<HoodieRecord<R>> mergedOpt = mergeIncomingWithExistingRecord(
- incoming, existing, writerSchema, writerSchemaWithMetaFields,
updatedConfig,
+ incoming, existing, writerSchema, writerSchemaWithMetaFields,
+ mergedSchema, mergedSchemaWithMetaFields,
partitionResolvableFromRecord, updatedConfig,
Review Comment:
The arm this feeds at line 616 (`merged.newInstance(existing.getKey())`,
changed partition with `update.partition.path=false`) returns an untagged
record: `HoodieAvroRecord.newInstance(HoodieKey)` sets the location to
`Option.empty()`, unlike the sibling arm at 608 that calls `tagRecord`.
Untagged means it is written as an insert, so the key lands in a second file
group unless small-file routing happens to pick the same slice. Pre-existing
from #13830 and untested (`TestGlobalIndexEnableUpdatePartitions` only runs
with the flag true), but with #19709 merged a partial `update set t.dt = s.dt,
t.amount = s.amount` now reaches it.
Could we `tagRecord(merged.newInstance(existing.getKey()),
existing.getCurrentLocation())` there, matching the arm's own comment, and add
a matrix row that assigns `dt` to a new value asserting a single file slice?
##########
hudi-spark-datasource/hudi-spark/src/test/scala/org/apache/spark/sql/hudi/dml/others/TestPartialUpdateForMergeInto.scala:
##########
@@ -732,12 +732,144 @@ class TestPartialUpdateForMergeInto extends
HoodieSparkSqlTestBase {
}
}
+ // A partial update names only the columns being changed, so the record key
is normally absent
Review Comment:
The existing test at lines 611-681 is the nearest shape to #19712
(partitioned MOR, `GLOBAL_SIMPLE`, partial `UPDATE SET`, mixed `INSERT`,
partition change) and has no assertions at all; it also runs with the default
`update.partition.path=true`, so partial updates are off in it, which is why it
never caught this.
Could we give it a `checkAnswer` on the rows plus `_hoodie_partition_path`
and set `update.partition.path=false` there, so the mixed-clause and compaction
shapes this matrix skips are covered without another cell?
--
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]
