hudi-agent commented on code in PR #19348:
URL: https://github.com/apache/hudi/pull/19348#discussion_r3632972459
##########
hudi-common/src/main/java/org/apache/hudi/common/util/ConfigUtils.java:
##########
@@ -151,7 +151,13 @@ public static TypedProperties
supplementOrderingFields(TypedProperties props, Li
* Ensures that the prefixed merge properties are populated for mergers.
*/
public static TypedProperties getMergeProps(TypedProperties props,
HoodieTableConfig tableConfig) {
- Map<String, String> mergeProps = tableConfig.getTableMergeProperties();
+ // Resolve the effective payload class from the reader/write props first
-- it may be set only
+ // there (e.g. hoodie.datasource.write.payload.class for a pre-v9 table
that never persisted the
+ // payload class into its table config), falling back to the table config.
This ensures the
+ // pre-v9 delete-marker derivation in getTableMergeProperties fires for
such tables.
+ String payloadClass =
HoodieRecordPayload.getPayloadClassNameIfPresent(props)
Review Comment:
🤖 This resolves the payload class from `props` first (via
`getPayloadClassNameIfPresent`), which fixes the write-config-only case nicely.
I wonder about the reverse direction: for a pre-v9 table that *did* persist a
Debezium/DMS payload under `hoodie.compaction.payload.class`, could a caller
ever pass `props` that carry a different effective payload — e.g. a
`hoodie.compaction.payload.class`/`hoodie.datasource.write.payload.class`
override or a `COMMIT_TIME_ORDERING` merge mode — that now shadows the
table-config payload and silently skips the delete-marker derivation that
previously fired off `getPayloadClass()`? The FileGroupReader read path
(`HoodieFileGroupReader#init`) passes query-side props here, so it'd be worth
confirming those always carry the same payload class as the persisted table
config (or none). Since a mismatch would silently drop Debezium deletes on read
rather than error, it seems worth a quick sanity check.
<sub><i>⚠️ AI-generated; verify before applying. React 👍/👎 to flag
quality.</i></sub>
##########
hudi-spark-datasource/hudi-spark/src/test/scala/org/apache/hudi/functional/TestPayloadDeprecationFlow.scala:
##########
@@ -503,6 +503,83 @@ class TestPayloadDeprecationFlow extends
SparkClientFunctionalTestHarness {
.build()
}
+ /**
+ * End-to-end reproduction for a Copy-on-Write, pre-v9 Postgres-Debezium
table whose payload class
+ * is supplied ONLY via the write config
(hoodie.datasource.write.payload.class) and is NOT
+ * persisted in the table properties. A Debezium delete
(_change_operation_type='d') for a key that
+ * is absent from the base file it gets merged into goes down the
FileGroupReader write path's
+ * previousRecord==null branch. Before the fix the reader cannot derive the
Debezium delete markers
+ * (getTableMergeProperties resolves the payload class from the table
config, which does not have
+ * it), so the delete is classified isDelete=false, routed into
+ * PayloadUpdateProcessor.handleNonDeletes, and .get()s the empty Option ->
NoSuchElementException.
+ *
+ * The parameter is the "similar test case": stripPayloadClass=false keeps
the payload class in the
+ * table properties (delete detected, always works) and acts as the control;
stripPayloadClass=true
+ * reproduces the incident (broken before the fix, correct after).
+ */
+ @ParameterizedTest
+ @ValueSource(strings = Array("true", "false"))
+ def
testDebeziumDeleteForAbsentKeyWithPayloadClassNotInTableConfig(stripPayloadClassStr:
String): Unit = {
+ val stripPayloadClass = stripPayloadClassStr.equals("true")
+ val payloadClazz = classOf[PostgresDebeziumAvroPayload].getName
+ // Payload class is only a write option, never a table property (mirrors
gw-agent provisioning).
+ val opts: Map[String, String] = Map(
+ HoodieWriteConfig.WRITE_PAYLOAD_CLASS_NAME.key() -> payloadClazz,
+ HoodieMetadataConfig.ENABLE.key() -> "false")
+ // Single-bucket index so every record (including the new delete key)
hashes to the one existing
+ // file group and is merged against its base file -- deterministically
forcing the FileGroupReader
+ // write path's previousRecord==null branch rather than a fresh insert
file group.
Review Comment:
🤖 nit: `serviceOpts` is a bit misleading here — in Hudi, "service" usually
refers to table services (compaction, clustering). Could you rename this to
`indexOpts` or `bucketIndexOpts` to make it clear these are index-configuration
options?
<sub><i>⚠️ AI-generated; verify before applying. React 👍/👎 to flag
quality.</i></sub>
##########
hudi-spark-datasource/hudi-spark/src/test/scala/org/apache/hudi/functional/TestPayloadDeprecationFlow.scala:
##########
@@ -503,6 +503,83 @@ class TestPayloadDeprecationFlow extends
SparkClientFunctionalTestHarness {
.build()
}
+ /**
+ * End-to-end reproduction for a Copy-on-Write, pre-v9 Postgres-Debezium
table whose payload class
+ * is supplied ONLY via the write config
(hoodie.datasource.write.payload.class) and is NOT
+ * persisted in the table properties. A Debezium delete
(_change_operation_type='d') for a key that
+ * is absent from the base file it gets merged into goes down the
FileGroupReader write path's
+ * previousRecord==null branch. Before the fix the reader cannot derive the
Debezium delete markers
+ * (getTableMergeProperties resolves the payload class from the table
config, which does not have
+ * it), so the delete is classified isDelete=false, routed into
+ * PayloadUpdateProcessor.handleNonDeletes, and .get()s the empty Option ->
NoSuchElementException.
+ *
+ * The parameter is the "similar test case": stripPayloadClass=false keeps
the payload class in the
+ * table properties (delete detected, always works) and acts as the control;
stripPayloadClass=true
+ * reproduces the incident (broken before the fix, correct after).
+ */
+ @ParameterizedTest
+ @ValueSource(strings = Array("true", "false"))
+ def
testDebeziumDeleteForAbsentKeyWithPayloadClassNotInTableConfig(stripPayloadClassStr:
String): Unit = {
Review Comment:
🤖 nit: `stripPayloadClassStr.toBoolean` is the idiomatic Scala equivalent of
`.equals("true")` and reads a bit more naturally — could you swap it in?
<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]
