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`. The one cell where the two
spellings diverge, and where a wrong `__HIVE_DEFAULT_PARTITION__` would surface
as a delete against the real partition plus a 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. The flag
is not even needed to reach that arm: while the RLI partition is not yet built,
`SparkMetadataTableGlobalRecordLevelIndex.java:79-88` falls back to
`GLOBAL_SIMPLE`, whose flag defaults to `true`.
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-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:
**Correction** to my second paragraph: the matrix does observe
`partitionResolvableFromRecord`. With the arm reverted the untagged record
counts as an insert, `UpsertPartitioner.java:189-216` opens a small-file UPDATE
bucket at the existing fileId, and
`BaseSparkDeltaCommitActionExecutor.java:76-79` diverts it to the COW merge
handle: zero log files, so `validateLogBlock(..., 1, ...)` fails at `:877`. The
duplicate-cell point and the `update.partition.path=true` cell stand. That flag
is not even needed to reach the delete+insert arm: while the RLI partition is
not yet built, `SparkMetadataTableGlobalRecordLevelIndex.java:79-88` falls back
to `GLOBAL_SIMPLE`, whose flag defaults to `true`.
--
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]
