Enable force overwrite for partial upserts #15107

himanish-star · 2025-02-21T18:27:28Z

This PR removes the logic that stops prev column value to be overwritten by new column value if the new value is NULL.

Before:

(null, newValue) -> newValue
(oldValue, null) -> oldValue
(null, null) -> null

After:

(null, newValue) -> newValue
(oldValue, null) -> null
(null, null) -> null

tibrewalpratik17 · 2025-02-21T18:55:35Z

.../src/main/java/org/apache/pinot/segment/local/upsert/merger/PartialUpsertColumnarMerger.java

-          if (newRow.isNullValue(column)) {
-            resultHolder.put(column, prevValue);
-          } else {
-            resultHolder.put(column, merger.merge(prevValue, newRow.getValue(column)));
-          }
+          resultHolder.put(column, merger.merge(prevValue, newRow.getValue(column)));


This change is backward-incompatible. Going through the attached issue, I would recommend to create a new merger strategy and allow null-overwrites there.

Okay 👍🏻

codecov-commenter · 2025-02-22T10:14:54Z

Codecov Report

Attention: Patch coverage is 55.55556% with 4 lines in your changes missing coverage. Please review.

Project coverage is 63.53%. Comparing base (59551e4) to head (41fa65b).
Report is 1858 commits behind head on master.

Files with missing lines	Patch %	Lines
...cal/upsert/merger/PartialUpsertColumnarMerger.java	25.00%	2 Missing and 1 partial ⚠️
...l/upsert/merger/columnar/ForceOverwriteMerger.java	50.00%	1 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##             master   #15107      +/-   ##
============================================
+ Coverage     61.75%   63.53%   +1.78%     
- Complexity      207     1459    +1252     
============================================
  Files          2436     2760     +324     
  Lines        133233   155337   +22104     
  Branches      20636    23916    +3280     
============================================
+ Hits          82274    98695   +16421     
- Misses        44911    49189    +4278     
- Partials       6048     7453    +1405

Flag	Coverage Δ
custom-integration1	`100.00% <ø> (+99.99%)`	⬆️
integration	`100.00% <ø> (+99.99%)`	⬆️
integration1	`100.00% <ø> (+99.99%)`	⬆️
integration2	`0.00% <ø> (ø)`
java-11	`63.50% <55.55%> (+1.79%)`	⬆️
java-21	`63.43% <55.55%> (+1.80%)`	⬆️
skip-bytebuffers-false	`63.52% <55.55%> (+1.77%)`	⬆️
skip-bytebuffers-true	`63.40% <55.55%> (+35.68%)`	⬆️
temurin	`63.53% <55.55%> (+1.78%)`	⬆️
unittests	`63.53% <55.55%> (+1.78%)`	⬆️
unittests1	`56.15% <11.11%> (+9.26%)`	⬆️
unittests2	`33.95% <55.55%> (+6.22%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

raghukn · 2025-02-23T16:49:32Z

Pls update the documentation also with this.

tibrewalpratik17 · 2025-02-23T17:07:47Z

Hey @himanish-star the solution looks good to me! Can you please add unit-test / integration-test for your change too?

himanish-star · 2025-02-24T04:18:42Z

Hey @himanish-star the solution looks good to me! Can you please add unit-test / integration-test for your change too?

Sure, I'll add today

himanish-star · 2025-02-24T04:19:00Z

Pls update the documentation also with this.

Yes, I'll add documentation mentioning the new config value to be set

himanish-star · 2025-02-24T10:11:07Z

added unit tests

himanish-star · 2025-02-25T06:24:10Z

@tibrewalpratik17 I've tested locally using partial upsert quick start and added unit tests and they're working correctly. Shall we move ahead with this PR?

…manish-star/accept-null-overwrite

tibrewalpratik17 · 2025-02-28T13:36:00Z

LGTM!

cc @Jackie-Jiang @klsince in case you have any comments!

.../src/main/java/org/apache/pinot/segment/local/upsert/merger/PartialUpsertColumnarMerger.java

himanish-star · 2025-03-11T02:49:21Z

@Jackie-Jiang do the changes look good to go?

himanish-star · 2025-03-18T04:00:11Z

Hi @deemoliu @tibrewalpratik17 @klsince .... can any one of you merge this PR please?

allow overwrite merge with null values

06fb8ae

tibrewalpratik17 reviewed Feb 21, 2025

View reviewed changes

add new merger and strategy

11dbafc

himanish-star requested a review from tibrewalpratik17 February 22, 2025 04:47

added unit tests

d7fa44f

himanish-star changed the title ~~[bugfix] Enable force overwrite for partial upserts~~ Enable force overwrite for partial upserts Feb 25, 2025

Merge branch 'master' of https://github.com/apache/pinot into user/hi…

a38c2a6

…manish-star/accept-null-overwrite

tibrewalpratik17 approved these changes Feb 28, 2025

View reviewed changes

klsince reviewed Feb 28, 2025

View reviewed changes

.../src/main/java/org/apache/pinot/segment/local/upsert/merger/PartialUpsertColumnarMerger.java Show resolved Hide resolved

deemoliu reviewed Feb 28, 2025

View reviewed changes

.../src/main/java/org/apache/pinot/segment/local/upsert/merger/PartialUpsertColumnarMerger.java Show resolved Hide resolved

klsince approved these changes Mar 6, 2025

View reviewed changes

.../src/main/java/org/apache/pinot/segment/local/upsert/merger/PartialUpsertColumnarMerger.java Show resolved Hide resolved

deemoliu approved these changes Mar 6, 2025

View reviewed changes

add comments

41fa65b

Jackie-Jiang added feature documentation upsert labels Mar 18, 2025

Jackie-Jiang merged commit 8997f3a into apache:master Mar 18, 2025
22 checks passed

tsekityam pushed a commit to tsekityam/pinot that referenced this pull request Mar 19, 2025

Enable force overwrite for partial upserts (apache#15107)

42c5470

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enable force overwrite for partial upserts #15107

Enable force overwrite for partial upserts #15107

himanish-star commented Feb 21, 2025 •

edited

Loading

tibrewalpratik17 Feb 21, 2025

himanish-star Feb 22, 2025

himanish-star Feb 22, 2025

codecov-commenter commented Feb 22, 2025 •

edited

Loading

raghukn commented Feb 23, 2025

tibrewalpratik17 commented Feb 23, 2025

himanish-star commented Feb 24, 2025

himanish-star commented Feb 24, 2025

himanish-star commented Feb 24, 2025

himanish-star commented Feb 25, 2025

tibrewalpratik17 commented Feb 28, 2025

himanish-star commented Mar 11, 2025

himanish-star commented Mar 18, 2025

Enable force overwrite for partial upserts #15107

Enable force overwrite for partial upserts #15107

Conversation

himanish-star commented Feb 21, 2025 • edited Loading

tibrewalpratik17 Feb 21, 2025

Choose a reason for hiding this comment

himanish-star Feb 22, 2025

Choose a reason for hiding this comment

himanish-star Feb 22, 2025

Choose a reason for hiding this comment

codecov-commenter commented Feb 22, 2025 • edited Loading

Codecov Report

raghukn commented Feb 23, 2025

tibrewalpratik17 commented Feb 23, 2025

himanish-star commented Feb 24, 2025

himanish-star commented Feb 24, 2025

himanish-star commented Feb 24, 2025

himanish-star commented Feb 25, 2025

tibrewalpratik17 commented Feb 28, 2025

himanish-star commented Mar 11, 2025

himanish-star commented Mar 18, 2025

himanish-star commented Feb 21, 2025 •

edited

Loading

codecov-commenter commented Feb 22, 2025 •

edited

Loading