Jackie-Jiang commented on code in PR #20261:
URL: https://github.com/apache/pinot/pull/20261#discussion_r4570788115
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/upsert/merger/PartialUpsertColumnarMerger.java:
##########
@@ -70,9 +70,15 @@ public void merge(LazyRow previousRow, GenericRow newRow,
Map<String, Object> re
PartialUpsertColumnMerger merger = _column2Mergers.getOrDefault(column,
_defaultColumnValueMerger);
if (merger instanceof ForceOverwriteMerger) {
// Force Overwrite mergers
- // If the merge strategy is Force Overwrite merger and prevValue is
always overwritten by newValue
- Object prevValue = previousRow.getValue(column);
- resultHolder.put(column, merger.merge(prevValue,
newRow.getValue(column)));
+ // If the merge strategy is Force Overwrite merger and prevValue is
always overwritten by newValue.
+ // A null newValue is stored in the row as the column's default value,
so put null here to keep the
+ // column null instead of turning the default value into a real value.
+ if (newRow.isNullValue(column)) {
+ resultHolder.put(column, null);
Review Comment:
**[P1] Preserve the incoming time placeholder and null marker**
When an explicit comparison column (e.g. `version`) differs from the table's
time column, `FORCE_OVERWRITE` can apply to time. For a missing time value,
realtime ingestion already puts a valid segment-creation timestamp in `newRow`
and marks the column null. Returning `null` here makes
`PartialUpsertHandler.setMergedValue()` replace that timestamp with its cached
original schema default. The handler uses the table schema, whereas
`RealtimeTableDataManager.setDefaultTimeValueIfInvalid()` corrects only the
cloned segment schema.
This is newly exposed by this change: the old branch returned
`newRow.getValue(column)`, preserving the valid timestamp even though it
incorrectly cleared the null marker. With a STRING time column formatted as
`yyyyMMdd`, the original default can be `"null"`; `BaseSegmentCreator` then
fails to parse the time-column bounds before the optional time-validity check,
preventing segment commit with the default `continueOnError=false`. Numeric
time defaults can instead produce invalid segment times that bypass normal
retention.
The desired result is to preserve the incoming normalized placeholder and
its null marker, so the null bitmap is set when the row is indexed.
`FORCE_OVERWRITE` can simply skip adding a result entry and leave the incoming
column untouched; this also handles non-null updates. Please add a regression
case with separate time and comparison columns and an ingestion-normalized time
placeholder.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
