feat(sink): change sink type to append only when plan is append-only #20558
Conversation
| if let Some(user_defined_sink_type) = user_defined_sink_type { | ||
| if user_defined_sink_type == SinkType::AppendOnly { | ||
| if user_force_append_only { | ||
| return Ok(SinkType::ForceAppendOnly); |
There was a problem hiding this comment.
When upstream is append-only, and user specifies both append-only and force-append-only, the SinkType changed from AppendOnly to ForceAppendOnly.
| } | ||
| } | ||
|
|
||
| Ok(user_defined_sink_type) |
There was a problem hiding this comment.
Regardless of upstream, when user specifies both upsert and force-append-only, it changed from returning error to Ok(SinkType::Upsert).
This is the CI failure.
|
|
||
| Ok(user_defined_sink_type) | ||
| } else { | ||
| if user_force_append_only { |
There was a problem hiding this comment.
If upstream is append-only, and omitting type = will result in SInkType::AppendOnly (the intended new behavior), isn't it more natural to tolerate an unnecessary force-append-only?
We can move this check after the match below and only reject for Upsert.
|
Thanks for @xiangjinwu's carefully review and I think it much complex than I think... I convert the PR to draft and will add test to cover the branches all... |
|
I thought we had enforced the FORMAT ... ENCODE ... syntax. |
I hereby agree to the terms of the RisingWave Labs, Inc. Contributor License Agreement.
What's changed and what's your intention?
When user does not specify sink type(
append-only,force-append-onlyorupsert), use plan's append only perporty for sink's type.The sink's type used in sink executor for some " chunk compact with key" behavior. We have found some unexpected compact in the flamegraph which is unnessary for an append-only sink.
Checklist
Documentation
Release note