-
Notifications
You must be signed in to change notification settings - Fork 1.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add ignoreMerger for partial upsert #7907
Conversation
Codecov Report
@@ Coverage Diff @@
## master #7907 +/- ##
=============================================
- Coverage 71.32% 14.35% -56.98%
+ Complexity 4092 80 -4012
=============================================
Files 1589 1545 -44
Lines 82139 80273 -1866
Branches 12270 12067 -203
=============================================
- Hits 58589 11525 -47064
- Misses 19578 67892 +48314
+ Partials 3972 856 -3116
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
About naming: how about adding this behavior to What do you think @deemoliu @Jackie-Jiang |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't really need an IgnoreMerger
, but just not put a merger in PartialUpsertHandler._column2Mergers
map when the strategy is IGNORE
. That can also save the unnecessary overhead
@yupeng9 I feel |
8583ab3
to
b8af001
Compare
This reverts commit 7ec47c4.
Description
Add ignore mergers.
Recently we got interesting use cases from industry about partial upsert.
Users have two event as follows, t is the timestamp column and t1<t2
{t1, a1, b1, c1, d1}
{t2, a2, nil, nil, nil}
user specified field "a" as Overwrite field, and "b", "c", "d" field are empty in the second event.
she expected merge result to be {a2, b1, c1, d1}
However the merge result was {a2, nil, nil, nil} which is the same as full upsert.
The reason of this issue is because she didn't specify the mergers for "b", "c", "d" fields. Thus these fields will use the default behavior, "Overwrite regardless null".
Her issue can be fixed with the following config, since the "overwrite" merger behavior is "Overwrite unless null".
Even though the overwrite mergers works here (because it ignore null value), the behavior is ignore but not overwrite which is confusing. In this PR, i created ignore merger to address this so partial upsert can have a cleaner interface.
Upgrade Notes
Does this PR prevent a zero down-time upgrade? (Assume upgrade order: Controller, Broker, Server, Minion)
backward-incompat
, and complete the section below on Release Notes)Does this PR fix a zero-downtime upgrade introduced earlier?
backward-incompat
, and complete the section below on Release Notes)Does this PR otherwise need attention when creating release notes? Things to consider:
release-notes
and complete the section on Release Notes)Release Notes
Documentation