voonhous commented on code in PR #19713:
URL: https://github.com/apache/hudi/pull/19713#discussion_r3851475695
##########
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: the key lands in a second file group
unless the slice is still a small-file candidate (no log file, base under
`parquet.small.file.limit`, `SparkUpsertDeltaCommitPartitioner.java:107-116`),
where `BaseSparkDeltaCommitActionExecutor.java:76-79` merges it through the COW
handle and dedups by key, which is why `TestMergeIntoTable.scala:393` passes.
Pre-existing from #13830 and shipped (`git tag --contains 7943e1e18849`:
release-1.1.0, 1.1.1, 1.2.0); with #19709 merged, a partial `update set t.dt =
s.dt, t.amount = s.amount` reaches it too.
Given the release exposure, could
`tagRecord(merged.newInstance(existing.getKey()),
existing.getCurrentLocation())` be its own one-line PR, backportable on its
own, with a test that sets `hoodie.merge.small.file.group.candidates.limit=0`
and asserts a single file slice after the partition change?
##########
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:
Two additions. This shipped: `git tag --contains 7943e1e18849` lists
release-1.1.0, 1.1.1 and 1.2.0, and #13830 own index tests are COW, where the
arm is unreachable. And the masking is exact:
`SparkUpsertDeltaCommitPartitioner.java:107-116` only packs the untagged record
into a slice with no log file and a base under `parquet.small.file.limit`,
after which `BaseSparkDeltaCommitActionExecutor.java:76-79` merges it through
the COW handle and dedups by key; `TestMergeIntoTable.scala:393` passes because
six small inserts leave exactly that state.
Given the release exposure, could this be its own one-line PR, backportable
on its own, rather than part of this one?
--
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]
