[GitHub] [druid] LakshSingla commented on a diff in pull request #13706: Generate tombstones when running MSQ's replace

["LakshSingla (via GitHub)" <gi...@apache.org>]

LakshSingla commented on code in PR #13706:
URL: https://github.com/apache/druid/pull/13706#discussion_r1093878894


##########
extensions-core/multi-stage-query/src/main/java/org/apache/druid/msq/exec/ControllerImpl.java:
##########
@@ -1247,48 +1248,67 @@ private void postResultPartitionBoundariesForStage(
   /**
    * Publish the list of segments. Additionally, if {@link DataSourceMSQDestination#isReplaceTimeChunks()},
    * also drop all other segments within the replacement intervals.
-   * <p>
-   * If any existing segments cannot be dropped because their intervals are not wholly contained within the
-   * replacement parameter, throws a {@link MSQException} with {@link InsertCannotReplaceExistingSegmentFault}.
    */
   private void publishAllSegments(final Set<DataSegment> segments) throws IOException
   {
     final DataSourceMSQDestination destination =
         (DataSourceMSQDestination) task.getQuerySpec().getDestination();
-    final Set<DataSegment> segmentsToDrop;
+    Set<DataSegment> segmentsWithTombstones = new HashSet<>(segments);
 
     if (destination.isReplaceTimeChunks()) {
       final List<Interval> intervalsToDrop = findIntervalsToDrop(Preconditions.checkNotNull(segments, "segments"));
 
-      if (intervalsToDrop.isEmpty()) {
-        segmentsToDrop = null;
-      } else {
-        // Determine which segments to drop as part of the replace operation. This is safe because, in the case where we
-        // are doing a replace, the isReady method (which runs prior to the task starting) acquires an exclusive lock.
-        segmentsToDrop =
-            ImmutableSet.copyOf(
-                context.taskActionClient().submit(
-                    new RetrieveUsedSegmentsAction(
-                        task.getDataSource(),
-                        null,
-                        intervalsToDrop,
-                        Segments.ONLY_VISIBLE
-                    )
-                )
+      if (!intervalsToDrop.isEmpty()) {

Review Comment:
   There are some minute differences between how the TombstoneHelper expects the arguments v/s as to how the MSQ is generating the segments:
   The TombstoneHelper is using the DataSchema and its granularitySpec to compute the empty segments, v/s here we have the empty intervals for which we know that the segments corresponding to it should be empty, due to which I wasn't able to reconcile the code paths cleanly. One way was to create a dummy data schema corresponding to the empty intervals. 
   Also, the `pushedSegments` argument in the helper was of no use since we know the empty intervals in replace, therefore we would also need to dummy that to something which would never overlap. Due to these, I decided to drop the usage of TombstoneHelper



-- 
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@druid.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org