abhishekrb19 commented on PR #15243: URL: https://github.com/apache/druid/pull/15243#issuecomment-1780335396
Thank you for the review, @gianm! 1. Yep, I agree. It looks like @LakshSingla did some analysis re reusing `computeTombstones`- https://github.com/apache/druid/pull/13706#discussion_r1093878894. I also looked into it, and I think with some refactoring like moving [IntervalUtils](https://github.com/apache/druid/blob/master/extensions-core/multi-stage-query/src/main/java/org/apache/druid/msq/util/IntervalUtils.java) into core druid and passing in specific arguments, this should be possible. I'd prefer to revisit the refactor as a separate PR from the functional change in this patch. For now, I've added javadocs to the two methods the native batch and MSQ engines use. 2. Good idea! I updated the code per your suggestion and added unit tests in `MSQReplaceTest.java` and `TombstoneHelperTest.java`. -- 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]
