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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
