You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@paimon.apache.org by "irodriguezclaveria (via GitHub)" <gi...@apache.org> on 2026/07/03 15:45:46 UTC
[PR] add flink support FLIP-510 [paimon]
irodriguezclaveria opened a new pull request, #8452:
URL: https://github.com/apache/paimon/pull/8452
### Purpose
### Tests
--
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: issues-unsubscribe@paimon.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [flink] Support FLIP-510 key-only deletes on primary-key table sink [paimon]
Posted by "yunfengzhou-hub (via GitHub)" <gi...@apache.org>.
yunfengzhou-hub commented on code in PR #8452:
URL: https://github.com/apache/paimon/pull/8452#discussion_r3575830225
##########
docs/generated/flink_connector_configuration.html:
##########
@@ -272,6 +272,12 @@
<td>MemorySize</td>
<td>Weight of managed memory for RocksDB in cross-partition update, Flink will compute the memory size according to the weight, the actual memory used depends on the running environment.</td>
</tr>
+ <tr>
+ <td><h5>sink.key-only-deletes.enabled</h5></td>
+ <td style="word-wrap: break-word;">false</td>
+ <td>Boolean</td>
+ <td>If true, a primary-key table sink advertises the key-only (partial) deletes capability, allowing the Flink planner to drop the upstream ChangelogNormalize node when the source produces deletes by key. Requires Flink 2.1+; no effect on Flink 1.x. Does not apply when the table has no primary key, when 'changelog-producer' is 'input', or when 'merge-engine' is 'aggregation' or 'partial-update' with aggregation functions; in those cases a warning is logged. Disabled by default.</td>
Review Comment:
> no effect on Flink 1.x.
Also no effect on Flink 2.0.
--
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: issues-unsubscribe@paimon.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [flink] Support FLIP-510 key-only deletes on primary-key table sink [paimon]
Posted by "yunfengzhou-hub (via GitHub)" <gi...@apache.org>.
yunfengzhou-hub commented on code in PR #8452:
URL: https://github.com/apache/paimon/pull/8452#discussion_r3548655469
##########
paimon-flink/paimon-flink-common/src/main/java/org/apache/paimon/flink/sink/FlinkTableSinkBase.java:
##########
Review Comment:
I think your idea is basically correct, that key-only deletes cannot work with `input` changelog producer or merge engines with aggregation semantics.
Apart from our discussion here, a complete test for all possible combinations with `ParameterizedTest` could help provide a better guarantee for the supported situations in the long term.
--
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: issues-unsubscribe@paimon.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [flink] Support FLIP-510 key-only deletes on primary-key table sink [paimon]
Posted by "JingsongLi (via GitHub)" <gi...@apache.org>.
JingsongLi merged PR #8452:
URL: https://github.com/apache/paimon/pull/8452
--
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: issues-unsubscribe@paimon.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [flink] Support FLIP-510 key-only deletes on primary-key table sink [paimon]
Posted by "irodriguezclaveria (via GitHub)" <gi...@apache.org>.
irodriguezclaveria commented on code in PR #8452:
URL: https://github.com/apache/paimon/pull/8452#discussion_r3546943070
##########
paimon-flink/paimon-flink-common/src/main/java/org/apache/paimon/flink/sink/FlinkTableSinkBase.java:
##########
Review Comment:
Thanks, good point. So far I have validated it with deduplicate + none, which works correctly.
For the other combinations, this is my understanding — could you confirm?
Changelog producers:
input: the docs say it expects the input to already be a complete changelog, so with key-only deletes it would forward incomplete deletes. Should we treat it as not compatible here?
lookup / full-compaction: both reconstruct the complete changelog, so I would expect them to work with key-only deletes.
Merge engines:
partial-update: I think it works as long as delete handling is enabled (remove-record-on-delete), since it merges and removes by key.
aggregate: this one probably does not work, because it needs the previous values to retract on updates and deletes, and a key-only delete does not carry them. Would you agree it should be blocked?
first-row: it ignores updates and deletes by design, so a key-only delete would have no effect. I understand it does not really apply here.
For the combinations that do not apply, I could document them as unsupported. I am not sure yet whether we also need a check or a test for that, or if documentation would be enough — what would you prefer?
For the other cases I will test adding the corresponding junits/IT.
Thanks
--
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: issues-unsubscribe@paimon.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [flink] Support FLIP-510 key-only deletes on primary-key table sink [paimon]
Posted by "irodriguezclaveria (via GitHub)" <gi...@apache.org>.
irodriguezclaveria commented on PR #8452:
URL: https://github.com/apache/paimon/pull/8452#issuecomment-4952810227
Hi @yunfengzhou-hub ,
Let me explain you the changes that I did:
On test coverage (generate a Flink plan and check ChangelogNormalize is removed via Table#explain + check data correctness):
I added end-to-end data correctness tests in KeyOnlyDeletesITCase:
- One test with deduplicate + changelog-producer=none + sink.key-only-deletes.enabled=true, where the source sends updates only as +U (no -U) and deletes with only the primary key. It checks the final table content is correct.
- A parameterized test over {deduplicate, partial-update} × {lookup, full-compaction} that checks both the batch result and the streaming reconstructed changelog (the -U/+U pair with the before-image, and the -D with the full row).
About checking ChangelogNormalize removal with explain: I tried it, but I could not make it observable. The option only changes the sink ChangelogMode. In a normal INSERT INTO paimon SELECT ... FROM source, the plan has no ChangelogNormalize in either case, because the Paimon PK sink already accepts upsert. So the plan does not change with the option on/off. The real effect of the capability is on the sink ChangelogMode, which is already covered by the unit test ChangelogModeTest.
---
On throwing an exception / warning when a prior condition changes the ChangelogMode:
Done. When sink.key-only-deletes.enabled=true but a prior condition prevents the optimization (no primary key, changelog-producer=input, merge-engine=aggregation, or partial-update with aggregation functions), we now log a warning and keep the job running (no exception). I also documented these limitations in the option description.
---
On the auto option (true / false / auto):
I thought about it, but I think true/false is enough. The current true already works as "best effort": it enables the optimization where possible, and where a prior condition prevents it, it just skips it and logs a warning — it never throws. So true already has the "auto" semantics. The only difference from the proposed auto would be hiding the warning, and the warning is exactly what tells the user why the optimization did not apply. So I would prefer to keep true/false.
---
On the combinations (input / aggregation / etc.):
I agree with your understanding: key-only deletes cannot work with input changelog producer or with aggregation merge engines. The code already blocks these cases. I also added a test in ChangelogModeTest to check that the option is ignored when changelog-producer=input.
---
On not mentioning FLIP-510 in the user docs:
Done. I removed FLIP-510 from the option description (user docs). I kept the reference only in JavaDocs/comments for developers, as you suggested.
---
Thanks!
--
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: issues-unsubscribe@paimon.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [flink] Support FLIP-510 key-only deletes on primary-key table sink [paimon]
Posted by "yunfengzhou-hub (via GitHub)" <gi...@apache.org>.
yunfengzhou-hub commented on code in PR #8452:
URL: https://github.com/apache/paimon/pull/8452#discussion_r3541067053
##########
paimon-flink/paimon-flink2-common/src/main/java/org/apache/paimon/flink/utils/ChangelogModeUtils.java:
##########
@@ -0,0 +1,30 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.paimon.flink.utils;
+
+import org.apache.flink.table.connector.ChangelogMode;
+
+/** Utility methods about Flink {@link ChangelogMode} to resolve compatibility issues. */
+public class ChangelogModeUtils {
+
+ /** FLIP-510 key-only deletes, available from Flink 2.1. */
+ public static ChangelogMode.Builder enableKeyOnlyDeletes(ChangelogMode.Builder builder) {
+ return builder.keyOnlyDeletes(true);
Review Comment:
This may throw `NoSuchMethodException` for Flink 2.0. Maybe an additional stub code is needed for `paimon-flink-2.0` module.
##########
paimon-flink/paimon-flink-common/src/test/java/org/apache/paimon/flink/ChangelogModeTest.java:
##########
@@ -89,6 +90,21 @@ public void testDefault() throws Exception {
.build());
}
+ @Test
+ public void testKeyOnlyDeletesEnabled() throws Exception {
Review Comment:
The test coverage seems a little not enough. Maybe we can add some test cases that actually generate a Flink Plan from a SQL query, check whether ChangelogNormalizer has actually been removed through `Table#explain`, and check the correctness of result data.
##########
paimon-flink/paimon-flink-common/src/main/java/org/apache/paimon/flink/sink/FlinkTableSinkBase.java:
##########
Review Comment:
In the current implementation, if some prior conditions like this have made this method return in advance and `ChangelogModeUtils.enableKeyOnlyDeletes` is not invoked, users won't receive any notification, until the Flink job is started and users found that the ChangelogNormalizer operator is still there. Thus how about adding the following behavior?
1. When users set `sink.key-only-deletes.enabled` to true, but prior conditions lead to a different ChangelogMode, throw an exception or at least generate a warning log.
2. Apart from `true` and `false`, add a third option, `auto`, for `sink.key-only-deletes.enabled`, When set to auto, paimon will try its best to enable the optimization when condition is met, but it is not guaranteed. In this case, no exception would be thrown if it failed to achieve the optimization.
3. Also document these limitations in the description of the option.
##########
paimon-flink/paimon-flink-common/src/main/java/org/apache/paimon/flink/FlinkConnectorOptions.java:
##########
@@ -493,6 +493,17 @@ public class FlinkConnectorOptions {
.withDescription(
"Enable sink writer coordinator to plan data files in Job Manager.");
+ public static final ConfigOption<Boolean> SINK_KEY_ONLY_DELETES_ENABLED =
+ key("sink.key-only-deletes.enabled")
+ .booleanType()
+ .defaultValue(false)
+ .withDescription(
+ "If true, a primary-key table sink advertises the FLIP-510 key-only "
Review Comment:
I personally recommend not mentioning FLIP-510 in user document. It is mostly an API for developers. For users, mentioning the semantics change and performance benefits should be enough.
If you think this information is helpful for developers, It might be more proper to reference FLIP-510 in JavaDocs and comments.
##########
paimon-flink/paimon-flink-common/src/main/java/org/apache/paimon/flink/sink/FlinkTableSinkBase.java:
##########
Review Comment:
It might be better to check whether these changelog producers and merge engines support enabling `sink.key-only-deletes.enabled`, and add tests to verify the correctness of results with different combinations of changelog producers and merge engines.
--
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: issues-unsubscribe@paimon.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [flink] Support FLIP-510 key-only deletes on primary-key table sink [paimon]
Posted by "irodriguezclaveria (via GitHub)" <gi...@apache.org>.
irodriguezclaveria commented on code in PR #8452:
URL: https://github.com/apache/paimon/pull/8452#discussion_r3546943070
##########
paimon-flink/paimon-flink-common/src/main/java/org/apache/paimon/flink/sink/FlinkTableSinkBase.java:
##########
Review Comment:
Thanks, good point. So far I have validated it with deduplicate + none, which works correctly.
For the other combinations, this is my understanding — could you confirm?
Changelog producers:
input: the docs say it expects the input to already be a complete changelog, so with key-only deletes it would forward incomplete deletes. Should we treat it as not compatible here?
lookup / full-compaction: both reconstruct the complete changelog, so I would expect them to work with key-only deletes. Do you agree?
Merge engines:
partial-update: I think it works as long as delete handling is enabled (remove-record-on-delete), since it merges and removes by key. Is that correct?
aggregate: this one probably does not work, because it needs the previous values to retract on updates and deletes, and a key-only delete does not carry them. Would you agree it should be blocked?
first-row: it ignores updates and deletes by design, so a key-only delete would have no effect. I understand it does not really apply here — is that right?
For the combinations that do not apply, I could document them as unsupported. I am not sure yet whether we also need a check or a test for that, or if documentation would be enough — what would you prefer?
Thanks
--
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: issues-unsubscribe@paimon.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [flink] Support FLIP-510 key-only deletes on primary-key table sink [paimon]
Posted by "irodriguezclaveria (via GitHub)" <gi...@apache.org>.
irodriguezclaveria commented on PR #8452:
URL: https://github.com/apache/paimon/pull/8452#issuecomment-4912873993
Hi @yunfengzhou-hub , thanks for the review, I'm going to check your comments and I'll let you know when the changes are done. Thanks again.
--
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: issues-unsubscribe@paimon.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [flink] Support FLIP-510 key-only deletes on primary-key table sink [paimon]
Posted by "irodriguezclaveria (via GitHub)" <gi...@apache.org>.
irodriguezclaveria commented on PR #8452:
URL: https://github.com/apache/paimon/pull/8452#issuecomment-4968445065
Hi @yunfengzhou-hub , thanks to review again! the change is done !
--
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: issues-unsubscribe@paimon.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org