[FLINK-39349][table] Support TO_CHANGELOG retract/upsert stream to upsert stream with deletion flag (4.2.2.1)#27847
Conversation
| throwOnFailure, | ||
| String.format( | ||
| "Invalid target mapping for argument 'op_mapping'. " | ||
| + "Unknown RowKind: '%s'. Valid values are: %s.", |
There was a problem hiding this comment.
RowKind is a Java class. And is confusing to SQL persona. This is why it called op:
| + "Unknown RowKind: '%s'. Valid values are: %s.", | |
| + "Unknown change operation: '%s'. Valid values are: %s.", |
| throwOnFailure, | ||
| String.format( | ||
| "Invalid target mapping for argument 'op_mapping'. " | ||
| + "Duplicate RowKind: '%s'.", |
There was a problem hiding this comment.
| + "Duplicate RowKind: '%s'.", | |
| + "Duplicate change operation: '%s'.", |
There was a problem hiding this comment.
please update all other locations
|
Thanks for the review, @twalthr 🙂 Addressed your comments, take a look |
| final Map<RowKind, String> map = new EnumMap<>(RowKind.class); | ||
| opMapping.forEach((name, code) -> map.put(RowKind.valueOf(name), code)); | ||
| return map; | ||
| final Map<RowKind, String> result = new EnumMap<>(RowKind.class); |
There was a problem hiding this comment.
Recommend adding logging for conflicts or invalid mappings to help users debug configuration issues.
There was a problem hiding this comment.
Hey, for TO_CHANGELOG invalid mappings are already caught at planning time by ToChangelogTypeStrategy, the user will see a validation error
| TableTestProgram.of( | ||
| "to-changelog-invalid-op-mapping", | ||
| "fails when op_mapping has invalid RowKind name") | ||
| "fails when op_mapping has invalid change operation name") |
There was a problem hiding this comment.
Recommend adding notes on why UPDATE_BEFORE is dropped and practical use cases of deletion flag pattern.
| * be valid and appear at most once across all entries. | ||
| */ | ||
| @SuppressWarnings("rawtypes") | ||
| private static Optional<List<DataType>> validateOpMappingKeys( |
There was a problem hiding this comment.
Recommend adding comments about whitespace and case handling for better user understanding.
There was a problem hiding this comment.
I've updated the comment to be more specific about this e8d4956
| * map("INSERT", "I", "UPDATE_AFTER", "U").asArgument("op_mapping") | ||
| * ); | ||
| * | ||
| * // Deletion flag pattern: comma-separated keys map multiple change operations to the same code |
There was a problem hiding this comment.
uggest enhancing Javadoc to clarify how multiple RowKinds mapping to the same output may affect UPDATE_BEFORE behavior.
There was a problem hiding this comment.
Thanks for the review, Feat Zhang. The existing JavaDoc and documentation already explain that unmapped operations are dropped. Adding UPDATE_BEFORE-specific details to the generic toChangelog() JavaDoc would be too narrow, since it depends on the use case
…ng validation JavaDoc
e8d4956 to
e47fadb
Compare
twalthr
left a comment
There was a problem hiding this comment.
Thanks @gustavodemorais for the additional improvements. Will merge now...
What is the purpose of the change
Extends TO_CHANGELOG's op_mapping parameter to support comma-separated RowKind names as keys, enabling multiple RowKinds to map to the same output code. This implements FLIP-564 section 4.2.2.1 - the deletion flag pattern (Kafka Connect
style):
Brief change log
Verifying this change
Does this pull request potentially affect one of the following parts:
@Public(Evolving): (no)Documentation