You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pinot.apache.org by "himanish-star (via GitHub)" <gi...@apache.org> on 2025/02/21 18:27:32 UTC
[PR] [bugfix] Enable force overwrite for partial upserts [pinot]
himanish-star opened a new pull request, #15107:
URL: https://github.com/apache/pinot/pull/15107
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
```
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Enable force overwrite for partial upserts [pinot]
Posted by "himanish-star (via GitHub)" <gi...@apache.org>.
himanish-star commented on code in PR #15107:
URL: https://github.com/apache/pinot/pull/15107#discussion_r1977467721
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/upsert/merger/PartialUpsertColumnarMerger.java:
##########
@@ -74,7 +75,10 @@ public void merge(LazyRow previousRow, GenericRow newRow, Map<String, Object> re
// (1) If the value of the previous is null value, skip merging and use the new value
// (2) Else If the value of new value is null, use the previous value (even for comparison columns)
// (3) Else If the column is not a comparison column, we applied the merged value to it
- if (!(merger instanceof OverwriteMerger)) {
+ if (merger instanceof ForceOverwriteMerger) {
Review Comment:
Adding
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Enable force overwrite for partial upserts [pinot]
Posted by "deemoliu (via GitHub)" <gi...@apache.org>.
deemoliu commented on code in PR #15107:
URL: https://github.com/apache/pinot/pull/15107#discussion_r1976093775
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/upsert/merger/PartialUpsertColumnarMerger.java:
##########
@@ -74,7 +75,10 @@ public void merge(LazyRow previousRow, GenericRow newRow, Map<String, Object> re
// (1) If the value of the previous is null value, skip merging and use the new value
// (2) Else If the value of new value is null, use the previous value (even for comparison columns)
// (3) Else If the column is not a comparison column, we applied the merged value to it
- if (!(merger instanceof OverwriteMerger)) {
+ if (merger instanceof ForceOverwriteMerger) {
Review Comment:
please add some comment Similar to Overwrite mergers.
If the merge strategy is force overwrite merger, always use the new value no matter what it's.
The idea is to minimize the access to the previous row value to reduce disk IO.
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] [bugfix] Enable force overwrite for partial upserts [pinot]
Posted by "tibrewalpratik17 (via GitHub)" <gi...@apache.org>.
tibrewalpratik17 commented on PR #15107:
URL: https://github.com/apache/pinot/pull/15107#issuecomment-2676991639
Hey @himanish-star the solution looks good to me! Can you please add unit-test / integration-test for your change too?
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Enable force overwrite for partial upserts [pinot]
Posted by "himanish-star (via GitHub)" <gi...@apache.org>.
himanish-star commented on code in PR #15107:
URL: https://github.com/apache/pinot/pull/15107#discussion_r1982473423
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/upsert/merger/PartialUpsertColumnarMerger.java:
##########
@@ -74,7 +75,10 @@ public void merge(LazyRow previousRow, GenericRow newRow, Map<String, Object> re
// (1) If the value of the previous is null value, skip merging and use the new value
// (2) Else If the value of new value is null, use the previous value (even for comparison columns)
// (3) Else If the column is not a comparison column, we applied the merged value to it
- if (!(merger instanceof OverwriteMerger)) {
+ if (merger instanceof ForceOverwriteMerger) {
Review Comment:
Added comments
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Enable force overwrite for partial upserts [pinot]
Posted by "Jackie-Jiang (via GitHub)" <gi...@apache.org>.
Jackie-Jiang merged PR #15107:
URL: https://github.com/apache/pinot/pull/15107
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Enable force overwrite for partial upserts [pinot]
Posted by "klsince (via GitHub)" <gi...@apache.org>.
klsince commented on code in PR #15107:
URL: https://github.com/apache/pinot/pull/15107#discussion_r1975904977
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/upsert/merger/PartialUpsertColumnarMerger.java:
##########
@@ -74,7 +75,10 @@ public void merge(LazyRow previousRow, GenericRow newRow, Map<String, Object> re
// (1) If the value of the previous is null value, skip merging and use the new value
// (2) Else If the value of new value is null, use the previous value (even for comparison columns)
// (3) Else If the column is not a comparison column, we applied the merged value to it
- if (!(merger instanceof OverwriteMerger)) {
+ if (merger instanceof ForceOverwriteMerger) {
+ Object prevValue = previousRow.getValue(column);
+ resultHolder.put(column, merger.merge(prevValue, newRow.getValue(column)));
Review Comment:
is there need to check `if (prevValue != null)` like the OverwriteMerger below? or does merger.merge() already handle null prevValue cleanly
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] [bugfix] Enable force overwrite for partial upserts [pinot]
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #15107:
URL: https://github.com/apache/pinot/pull/15107#issuecomment-2676133237
## [Codecov](https://app.codecov.io/gh/apache/pinot/pull/15107?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
Attention: Patch coverage is `11.11111%` with `8 lines` in your changes missing coverage. Please review.
> Project coverage is 55.93%. Comparing base [(`59551e4`)](https://app.codecov.io/gh/apache/pinot/commit/59551e45224f1535c4863fd577622b37366ccc97?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) to head [(`11dbafc`)](https://app.codecov.io/gh/apache/pinot/commit/11dbafceaddd5b2935f1c12f6f24771b2f120817?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
> Report is 1751 commits behind head on master.
| [Files with missing lines](https://app.codecov.io/gh/apache/pinot/pull/15107?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Patch % | Lines |
|---|---|---|
| [...cal/upsert/merger/PartialUpsertColumnarMerger.java](https://app.codecov.io/gh/apache/pinot/pull/15107?src=pr&el=tree&filepath=pinot-segment-local%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fpinot%2Fsegment%2Flocal%2Fupsert%2Fmerger%2FPartialUpsertColumnarMerger.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGlub3Qtc2VnbWVudC1sb2NhbC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3Qvc2VnbWVudC9sb2NhbC91cHNlcnQvbWVyZ2VyL1BhcnRpYWxVcHNlcnRDb2x1bW5hck1lcmdlci5qYXZh) | 0.00% | [4 Missing :warning: ](https://app.codecov.io/gh/apache/pinot/pull/15107?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [...l/upsert/merger/columnar/ForceOverwriteMerger.java](https://app.codecov.io/gh/apache/pinot/pull/15107?src=pr&el=tree&filepath=pinot-segment-local%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fpinot%2Fsegment%2Flocal%2Fupsert%2Fmerger%2Fcolumnar%2FForceOverwriteMerger.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGlub3Qtc2VnbWVudC1sb2NhbC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3Qvc2VnbWVudC9sb2NhbC91cHNlcnQvbWVyZ2VyL2NvbHVtbmFyL0ZvcmNlT3ZlcndyaXRlTWVyZ2VyLmphdmE=) | 0.00% | [2 Missing :warning: ](https://app.codecov.io/gh/apache/pinot/pull/15107?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [...ger/columnar/PartialUpsertColumnMergerFactory.java](https://app.codecov.io/gh/apache/pinot/pull/15107?src=pr&el=tree&filepath=pinot-segment-local%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fpinot%2Fsegment%2Flocal%2Fupsert%2Fmerger%2Fcolumnar%2FPartialUpsertColumnMergerFactory.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGlub3Qtc2VnbWVudC1sb2NhbC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3Qvc2VnbWVudC9sb2NhbC91cHNlcnQvbWVyZ2VyL2NvbHVtbmFyL1BhcnRpYWxVcHNlcnRDb2x1bW5NZXJnZXJGYWN0b3J5LmphdmE=) | 0.00% | [2 Missing :warning: ](https://app.codecov.io/gh/apache/pinot/pull/15107?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
> :exclamation: There is a different number of reports uploaded between BASE (59551e4) and HEAD (11dbafc). Click for more details.
>
> <details><summary>HEAD has 48 uploads less than BASE</summary>
>
>| Flag | BASE (59551e4) | HEAD (11dbafc) |
>|------|------|------|
>|integration|7|0|
>|integration2|3|0|
>|temurin|12|2|
>|java-21|7|2|
>|skip-bytebuffers-true|3|1|
>|skip-bytebuffers-false|7|1|
>|unittests|5|2|
>|java-11|5|0|
>|unittests2|3|0|
>|integration1|2|0|
>|custom-integration1|2|0|
></details>
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## master #15107 +/- ##
============================================
- Coverage 61.75% 55.93% -5.82%
- Complexity 207 710 +503
============================================
Files 2436 2148 -288
Lines 133233 115615 -17618
Branches 20636 18580 -2056
============================================
- Hits 82274 64671 -17603
- Misses 44911 45633 +722
+ Partials 6048 5311 -737
```
| [Flag](https://app.codecov.io/gh/apache/pinot/pull/15107/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [custom-integration1](https://app.codecov.io/gh/apache/pinot/pull/15107/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [integration](https://app.codecov.io/gh/apache/pinot/pull/15107/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [integration1](https://app.codecov.io/gh/apache/pinot/pull/15107/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [integration2](https://app.codecov.io/gh/apache/pinot/pull/15107/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [java-11](https://app.codecov.io/gh/apache/pinot/pull/15107/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [java-21](https://app.codecov.io/gh/apache/pinot/pull/15107/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `55.93% <11.11%> (-5.69%)` | :arrow_down: |
| [skip-bytebuffers-false](https://app.codecov.io/gh/apache/pinot/pull/15107/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `55.90% <11.11%> (-5.85%)` | :arrow_down: |
| [skip-bytebuffers-true](https://app.codecov.io/gh/apache/pinot/pull/15107/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `55.87% <11.11%> (+28.14%)` | :arrow_up: |
| [temurin](https://app.codecov.io/gh/apache/pinot/pull/15107/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `55.93% <11.11%> (-5.82%)` | :arrow_down: |
| [unittests](https://app.codecov.io/gh/apache/pinot/pull/15107/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `55.93% <11.11%> (-5.82%)` | :arrow_down: |
| [unittests1](https://app.codecov.io/gh/apache/pinot/pull/15107/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `55.93% <11.11%> (+9.04%)` | :arrow_up: |
| [unittests2](https://app.codecov.io/gh/apache/pinot/pull/15107/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#carryforward-flags-in-the-pull-request-comment) to find out more.
</details>
[:umbrella: View full report in Codecov by Sentry](https://app.codecov.io/gh/apache/pinot/pull/15107?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Enable force overwrite for partial upserts [pinot]
Posted by "himanish-star (via GitHub)" <gi...@apache.org>.
himanish-star commented on PR #15107:
URL: https://github.com/apache/pinot/pull/15107#issuecomment-2731552861
Hi @deemoliu @tibrewalpratik17 @klsince .... can any one of you merge this PR please?
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Enable force overwrite for partial upserts [pinot]
Posted by "klsince (via GitHub)" <gi...@apache.org>.
klsince commented on code in PR #15107:
URL: https://github.com/apache/pinot/pull/15107#discussion_r1982399364
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/upsert/merger/PartialUpsertColumnarMerger.java:
##########
@@ -74,7 +75,10 @@ public void merge(LazyRow previousRow, GenericRow newRow, Map<String, Object> re
// (1) If the value of the previous is null value, skip merging and use the new value
// (2) Else If the value of new value is null, use the previous value (even for comparison columns)
// (3) Else If the column is not a comparison column, we applied the merged value to it
- if (!(merger instanceof OverwriteMerger)) {
+ if (merger instanceof ForceOverwriteMerger) {
+ Object prevValue = previousRow.getValue(column);
+ resultHolder.put(column, merger.merge(prevValue, newRow.getValue(column)));
Review Comment:
ah, I see ForceOverwriteMerge simply ignore prev value
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Enable force overwrite for partial upserts [pinot]
Posted by "tibrewalpratik17 (via GitHub)" <gi...@apache.org>.
tibrewalpratik17 commented on PR #15107:
URL: https://github.com/apache/pinot/pull/15107#issuecomment-2690667837
LGTM!
cc @Jackie-Jiang @klsince in case you have any comments!
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] [bugfix] Enable force overwrite for partial upserts [pinot]
Posted by "himanish-star (via GitHub)" <gi...@apache.org>.
himanish-star commented on PR #15107:
URL: https://github.com/apache/pinot/pull/15107#issuecomment-2677403662
> 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
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] [bugfix] Enable force overwrite for partial upserts [pinot]
Posted by "tibrewalpratik17 (via GitHub)" <gi...@apache.org>.
tibrewalpratik17 commented on code in PR #15107:
URL: https://github.com/apache/pinot/pull/15107#discussion_r1966023827
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/upsert/merger/PartialUpsertColumnarMerger.java:
##########
@@ -77,11 +77,7 @@ public void merge(LazyRow previousRow, GenericRow newRow, Map<String, Object> re
if (!(merger instanceof OverwriteMerger)) {
Object prevValue = previousRow.getValue(column);
if (prevValue != 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)));
Review Comment:
This change is backward-incompatible. Going through the attached issue, I would recommend to create a new merger strategy and allow null-overwrites there.
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] [bugfix] Enable force overwrite for partial upserts [pinot]
Posted by "himanish-star (via GitHub)" <gi...@apache.org>.
himanish-star commented on code in PR #15107:
URL: https://github.com/apache/pinot/pull/15107#discussion_r1966432780
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/upsert/merger/PartialUpsertColumnarMerger.java:
##########
@@ -77,11 +77,7 @@ public void merge(LazyRow previousRow, GenericRow newRow, Map<String, Object> re
if (!(merger instanceof OverwriteMerger)) {
Object prevValue = previousRow.getValue(column);
if (prevValue != 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)));
Review Comment:
Done!
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Enable force overwrite for partial upserts [pinot]
Posted by "himanish-star (via GitHub)" <gi...@apache.org>.
himanish-star commented on code in PR #15107:
URL: https://github.com/apache/pinot/pull/15107#discussion_r1977467180
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/upsert/merger/PartialUpsertColumnarMerger.java:
##########
@@ -74,7 +75,10 @@ public void merge(LazyRow previousRow, GenericRow newRow, Map<String, Object> re
// (1) If the value of the previous is null value, skip merging and use the new value
// (2) Else If the value of new value is null, use the previous value (even for comparison columns)
// (3) Else If the column is not a comparison column, we applied the merged value to it
- if (!(merger instanceof OverwriteMerger)) {
+ if (merger instanceof ForceOverwriteMerger) {
+ Object prevValue = previousRow.getValue(column);
+ resultHolder.put(column, merger.merge(prevValue, newRow.getValue(column)));
Review Comment:
What's the use case? Even if `prevValue` is null this force overwrite will simply overwrite it with a null or non-null value
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] [bugfix] Enable force overwrite for partial upserts [pinot]
Posted by "raghukn (via GitHub)" <gi...@apache.org>.
raghukn commented on PR #15107:
URL: https://github.com/apache/pinot/pull/15107#issuecomment-2676983861
Pls update the documentation also with this.
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] [bugfix] Enable force overwrite for partial upserts [pinot]
Posted by "himanish-star (via GitHub)" <gi...@apache.org>.
himanish-star commented on PR #15107:
URL: https://github.com/apache/pinot/pull/15107#issuecomment-2680767362
@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?
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Enable force overwrite for partial upserts [pinot]
Posted by "himanish-star (via GitHub)" <gi...@apache.org>.
himanish-star commented on PR #15107:
URL: https://github.com/apache/pinot/pull/15107#issuecomment-2712376016
@Jackie-Jiang do the changes look good to go?
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] [bugfix] Enable force overwrite for partial upserts [pinot]
Posted by "himanish-star (via GitHub)" <gi...@apache.org>.
himanish-star commented on code in PR #15107:
URL: https://github.com/apache/pinot/pull/15107#discussion_r1966385111
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/upsert/merger/PartialUpsertColumnarMerger.java:
##########
@@ -77,11 +77,7 @@ public void merge(LazyRow previousRow, GenericRow newRow, Map<String, Object> re
if (!(merger instanceof OverwriteMerger)) {
Object prevValue = previousRow.getValue(column);
if (prevValue != 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)));
Review Comment:
Okay 👍🏻
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] [bugfix] Enable force overwrite for partial upserts [pinot]
Posted by "himanish-star (via GitHub)" <gi...@apache.org>.
himanish-star commented on PR #15107:
URL: https://github.com/apache/pinot/pull/15107#issuecomment-2677954640
added unit tests
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] [bugfix] Enable force overwrite for partial upserts [pinot]
Posted by "himanish-star (via GitHub)" <gi...@apache.org>.
himanish-star commented on PR #15107:
URL: https://github.com/apache/pinot/pull/15107#issuecomment-2677403945
> Pls update the documentation also with this.
Yes, I'll add documentation mentioning the new config value to be set
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org