rohityadav1993 commented on PR #11584: URL: https://github.com/apache/pinot/pull/11584#issuecomment-1752604357
Thanks for the feedback @Jackie-Jiang > We should not mix value based merger and row based merger in the same class because it will be very hard to maintain, and the sequence of applying the mergers can be very confusing. I agree with the maintenance perspective. There could be a requirement to have both row merger and column level merger for values which were not computed in row merger. Hence was the motivation to keep them in the same class. > I'm leaning towards the second approach because that allows us to reuse the existing mergers. This is a cleaner approach if we can stick to a good contract for merge. from the design doc [Approach 2](https://docs.google.com/document/d/1bBTCYZFP2stvzc6xZUOEh-XweVgC9WfD7uiSPbKtaZY/edit#heading=h.qkglmejmd9qa), if we can define merge as `merge(GenericRow previousRecord, GenericRow newRecord, Map<String, Object> mergerResult)` and pass an instance of `LazyRow extends GenericRow` > Since each column can reference the value from other columns, the contract should be to not change the column value before all the mergers are applied `mergerResult` will be used to store the intermediate merger results and avoid modification to newRecord until all mergers are applied. We can ensure that results are applied after column level mergers are executed. If we don't want to have `Map<String, Object> mergerResult` in merge method then we have to initialise PartialUpsertHandler at `PartitionUpsertMetadataManager` instead of `TableUpsertMetadataManager` to avoid concurrent modification. -- 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
