JingsongLi commented on code in PR #8928:
URL: https://github.com/apache/paimon/pull/8928#discussion_r3703503721
##########
paimon-core/src/main/java/org/apache/paimon/schema/SchemaValidation.java:
##########
@@ -1427,6 +1428,36 @@ private static void validatePrimaryKeyBlobConfiguration(
return;
}
+ checkArgument(
+ options.mergeEngine() == MergeEngine.DEDUPLICATE
+ || options.mergeEngine() == MergeEngine.PARTIAL_UPDATE,
Review Comment:
Allowing `PARTIAL_UPDATE` here exposes an incompatible retract path for
managed BLOBs. `PrimaryKeyBlobExternalizer.externalize` replaces every non-null
managed BLOB with `null` for `DELETE`/`UPDATE_BEFORE`, while
`PartialUpdateMergeFunction` passes that field to `FieldAggregator.retract`.
With `fields.ts.sequence-group=payload` and
`fields.payload.aggregate-function=last_non_null_value`, inserting `(payload=A,
ts=1)` and then retracting `(payload=A, ts=2)` incorrectly keeps `A`, because a
null retract field means ‘keep the accumulator.’ Please either
preserve/externalize non-null managed BLOB values on retract (and retain their
references), or reject retract-sensitive aggregation for managed BLOB fields.
An end-to-end retract regression test would also be needed.
##########
paimon-core/src/main/java/org/apache/paimon/schema/SchemaValidation.java:
##########
@@ -1427,6 +1428,36 @@ private static void validatePrimaryKeyBlobConfiguration(
return;
}
+ checkArgument(
+ options.mergeEngine() == MergeEngine.DEDUPLICATE
+ || options.mergeEngine() == MergeEngine.PARTIAL_UPDATE,
+ "Primary-key managed BLOB tables only support the deduplicate
or "
+ + "partial-update merge engine.");
+ checkArgument(
+ options.changelogProducer() == ChangelogProducer.NONE,
+ "Primary-key managed BLOB tables only support
changelog-producer 'none'.");
+ checkArgument(
+ options.dataFileExternalPaths() == null,
+ "Primary-key managed BLOB tables do not support '%s'.",
+ CoreOptions.DATA_FILE_EXTERNAL_PATHS.key());
+ checkArgument(
+ !options.pkClusteringOverride(),
+ "Primary-key managed BLOB tables do not support '%s'.",
+ CoreOptions.PK_CLUSTERING_OVERRIDE.key());
+ }
+
+ private static void validatePrimaryKeyBlobKeyConfiguration(
+ TableSchema schema, CoreOptions options) {
+ if (schema.primaryKeys().isEmpty()) {
+ return;
+ }
+
+ Set<String> managedBlobFields =
Review Comment:
This validation only checks the global `sequence.field`; it misses
sequence-group ordering fields encoded in option keys. For example,
`fields.payload.sequence-group=name` is accepted when `payload` is a managed
BLOB in a partial-update table. `PartialUpdateMergeFunction.Factory` later
creates a comparator for `payload`, and code generation fails with
`IllegalArgumentException: Illegal type: BLOB`. Please parse the key portion of
every `fields.<key[,key...]>.sequence-group` option and reject managed `BLOB`,
`ARRAY<BLOB>`, and `MAP<K, BLOB>` fields there, while continuing to allow BLOBs
on the protected-value side.
--
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]
