Jackie-Jiang commented on code in PR #10927:
URL: https://github.com/apache/pinot/pull/10927#discussion_r1290767908
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/upsert/PartialUpsertHandler.java:
##########
@@ -60,29 +72,57 @@ public PartialUpsertHandler(Schema schema, Map<String,
UpsertConfig.Strategy> pa
* For example, overwrite merger will only override the prev value if the
new value is not null.
* Null values will override existing values if not configured. They can be
ignored by using ignoreMerger.
*
- * @param previousRecord the last derived full record during ingestion.
+ * @param indexSegment the segment of the last derived full record during
ingestion.
+ * @param docId the docId of the last derived full record during ingestion
in the segment.
* @param newRecord the new consumed record.
- * @return a new row after merge
*/
- public GenericRow merge(GenericRow previousRecord, GenericRow newRecord) {
- for (String column : previousRecord.getFieldToValueMap().keySet()) {
+ public void merge(IndexSegment indexSegment, int docId, GenericRow
newRecord) {
+ for (String column: newRecord.getFieldToValueMap().keySet()) {
Review Comment:
I think we should still go over the fields within the segment (schema)
instead of the new record
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/upsert/PartialUpsertHandler.java:
##########
@@ -60,29 +72,57 @@ public PartialUpsertHandler(Schema schema, Map<String,
UpsertConfig.Strategy> pa
* For example, overwrite merger will only override the prev value if the
new value is not null.
* Null values will override existing values if not configured. They can be
ignored by using ignoreMerger.
*
- * @param previousRecord the last derived full record during ingestion.
+ * @param indexSegment the segment of the last derived full record during
ingestion.
+ * @param docId the docId of the last derived full record during ingestion
in the segment.
* @param newRecord the new consumed record.
- * @return a new row after merge
*/
- public GenericRow merge(GenericRow previousRecord, GenericRow newRecord) {
- for (String column : previousRecord.getFieldToValueMap().keySet()) {
+ public void merge(IndexSegment indexSegment, int docId, GenericRow
newRecord) {
+ for (String column: newRecord.getFieldToValueMap().keySet()) {
if (!_primaryKeyColumns.contains(column)) {
- if (!previousRecord.isNullValue(column)) {
+ // Non-overwrite mergers
+ // (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 (!(getMergerForColumn(column) instanceof OverwriteMerger)) {
Review Comment:
(minor) Cache the merger
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/upsert/PartialUpsertHandler.java:
##########
@@ -37,6 +43,7 @@ public class PartialUpsertHandler {
private final PartialUpsertMerger _defaultPartialUpsertMerger;
private final List<String> _comparisonColumns;
private final List<String> _primaryKeyColumns;
+ private final Logger _logger;
Review Comment:
Suggest not adding per-schema logger. To match the existing behavior, we can
directly throw `RuntimeException` if any exception happens
--
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-unsubscr...@pinot.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscr...@pinot.apache.org
For additional commands, e-mail: commits-h...@pinot.apache.org
