Skip to content
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

Enable force overwrite for partial upserts #15107

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

himanish-star
Copy link

@himanish-star himanish-star commented Feb 21, 2025

Fixes #14924

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

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)));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay 👍🏻

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done!

@codecov-commenter
Copy link

codecov-commenter commented Feb 22, 2025

Codecov Report

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

Project coverage is 63.42%. Comparing base (59551e4) to head (d7fa44f).
Report is 1754 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.42%   +1.67%     
- Complexity      207     1482    +1275     
============================================
  Files          2436     2749     +313     
  Lines        133233   154451   +21218     
  Branches      20636    23817    +3181     
============================================
+ Hits          82274    97964   +15690     
- Misses        44911    49088    +4177     
- Partials       6048     7399    +1351     
Flag Coverage Δ
custom-integration1 100.00% <ø> (+99.99%) ⬆️
integration 100.00% <ø> (+99.99%) ⬆️
integration1 100.00% <ø> (+99.99%) ⬆️
integration2 0.00% <ø> (ø)
java-11 63.39% <55.55%> (+1.68%) ⬆️
java-21 63.32% <55.55%> (+1.69%) ⬆️
skip-bytebuffers-false 63.42% <55.55%> (+1.67%) ⬆️
skip-bytebuffers-true 63.30% <55.55%> (+35.57%) ⬆️
temurin 63.42% <55.55%> (+1.67%) ⬆️
unittests 63.42% <55.55%> (+1.67%) ⬆️
unittests1 56.04% <11.11%> (+9.15%) ⬆️
unittests2 33.99% <55.55%> (+6.26%) ⬆️

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.

@raghukn
Copy link

raghukn commented Feb 23, 2025

Pls update the documentation also with this.

@tibrewalpratik17
Copy link
Contributor

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

@himanish-star
Copy link
Author

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
Copy link
Author

Pls update the documentation also with this.

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

@himanish-star
Copy link
Author

added unit tests

@himanish-star
Copy link
Author

@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?

@himanish-star himanish-star changed the title [bugfix] Enable force overwrite for partial upserts Enable force overwrite for partial upserts Feb 25, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Null handing issue with Partial upserts and partialUpsertStrategies
4 participants