Generate tombstones when running MSQ's replace #13706

LakshSingla · 2023-01-24T06:24:02Z

Description

When running REPLACE queries, the segments which contain no data are dropped (marked as unused). This PR aims to generate tombstones in place of segments which contain no data to mark their deletion, as is the behavior with the native ingestion.

This will cause InsertCannotReplaceExistingSegmentFault to be removed since it was generated if the interval to be marked unused didn't fully overlap one of the existing segments to replace.

Release note

REPLACE in MSQ now generates tombstones instead of marking segment as unused.

Key changed/added classes in this PR

ControllerImpl
TombstoneHelper

This PR has:

LakshSingla · 2023-01-31T05:38:09Z

Tried it out with some of the overlapping segment cases. Test data consists of a datasource partitioned by month (2022-02-01/2022-03-01), 28 rows, containing one row of data for each day.
Following is the query used to ingest the data

SELECT TIME_PARSE("rtttime") AS __time,* FROM TABLE(
  EXTERN(
    '{"type":"local","files":["/Users/lakshsingla/month_data.json"]}',
    '{"type": "json"}',
    '[{"name":"rtttime","type":"string"}, {"name":"sr_serial_number","type":"string"} , {"name":"drive_duration","type":"long"} ]'
  )
)
PARTITIONED BY MONTH

Ran a replace which partially overlaps the segment and lies completely inside

OVERWRITE WHERE __time >= TIMESTAMP '2022-02-05' AND __time < TIMESTAMP '2022-02-25'
SELECT * FROM "test_table_4"
WHERE __time >= TIMESTAMP '2022-02-05' AND __time < TIMESTAMP '2022-02-07'
PARTITIONED BY DAY

Earlier this used to throw the InsertCannotReplaceExistingSegment error, however now it works as expected. The tombstones for the 2022-02-07-2022-02-25 are generated.

Ran a replace which partially overlaps the segment and lies outside it as well

OVERWRITE WHERE __time >= TIMESTAMP '2022-01-05' AND __time < TIMESTAMP '2022-02-25'
SELECT * FROM "test_table_4"
WHERE __time >= TIMESTAMP '2022-02-05' AND __time < TIMESTAMP '2022-02-07'
PARTITIONED BY DAY

Earlier this used to throw the InsertCannotReplaceExistingSegment error, however now it works as expected. The tombstones for the 2022-02-01- 2022-02-04 2022-02-07-2022-02-25 are generated.

rohangarg · 2023-02-01T15:14:47Z

extensions-core/multi-stage-query/src/main/java/org/apache/druid/msq/exec/ControllerImpl.java

-                        Segments.ONLY_VISIBLE
-                    )
-                )
+      if (!intervalsToDrop.isEmpty()) {


I was trying to compare this code to the one present in the native ingestion - I couldn't understand the reason for not using TombstoneHelper class to compute the tombstone intervals and the segments.
Is there a specific reason that both the code paths can be common?

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

...vice/src/main/java/org/apache/druid/indexing/common/task/batch/parallel/TombstoneHelper.java

rohangarg

Thanks for moving the changes to TombstoneHelper. Left some comments

extensions-core/multi-stage-query/src/main/java/org/apache/druid/msq/exec/ControllerImpl.java

...vice/src/main/java/org/apache/druid/indexing/common/task/batch/parallel/TombstoneHelper.java

extensions-core/multi-stage-query/src/test/java/org/apache/druid/msq/test/MSQTestBase.java

cryptoe · 2023-02-06T12:01:53Z

extensions-core/multi-stage-query/src/test/java/org/apache/druid/msq/exec/MSQFaultsTest.java

@@ -114,30 +111,6 @@ public void testInsertCannotOrderByDescendingFault()
                     .verifyResults();
  }

-  @Test
-  public void testInsertCannotReplaceExistingSegmentFault()
-  {


We should have a test case which tests tombstone segments. This would give us more confidence in the PR.

I moved the same test case to the replace tests, and ensured that it passes. Changed the granularity of the query a bit so as to not blow up the tombstone segments that are generated.

cryptoe · 2023-02-06T12:30:19Z

extensions-core/multi-stage-query/src/test/java/org/apache/druid/msq/exec/MSQReplaceTest.java

-                         "test",
-                         0
-                     )))
+                     .setExpectedTombstoneIntervals(ImmutableSet.of(Intervals.of("2001-01-01/2001-02-01")))


How is this tombstone since we are generating data for this interval?

Updated the test cases, and moved this line along with the destination segments.

cryptoe · 2023-02-06T12:36:15Z

extensions-core/multi-stage-query/src/test/java/org/apache/druid/msq/test/MSQTestBase.java

@@ -1127,6 +1136,35 @@ public void verifyResults()
            Assert.assertTrue(segmentIdVsOutputRowsMap.get(diskSegment).contains(Arrays.asList(row)));
          }
        }
+        if (!testTaskActionClient.getPublishedSegments().isEmpty()) {
+          Set<SegmentId> expectedPublishedSegmentIds = segmentManager.getAllDataSegments()


Should't we substract segments which are present in segmentIdVsOutputRowMap.keys() when we are asserting against tombstone segments ?

Thanks for this, the previous logic was flawed where a duplicate segment id can be present in the tombstone interval and the test case would still pass. This is a better way of testing out the tombstone segments, updated it.

cryptoe · 2023-02-06T12:41:17Z

...-core/multi-stage-query/src/test/java/org/apache/druid/msq/test/MSQTestTaskActionClient.java


-  public MSQTestTaskActionClient(ObjectMapper mapper)
+  public MSQTestTaskActionClient(


Looks like we have state now in this client. Might want to mention that somewhere. Does it work with the calciteTests for MSQ ?

Yes, CalciteTests for MSQ only work on testing the SELECT engine of MSQ (since the original Calcite tests do not have any analog for ingestion). Changing this doesn't affect the test cases for MSQ.

cryptoe · 2023-02-06T12:53:04Z

...vice/src/main/java/org/apache/druid/indexing/common/task/batch/parallel/TombstoneHelper.java

+    for (Interval tombstoneInterval : tombstoneIntervals) {
+      String version = null;
+      for (final TaskLock lock : locks) {
+        if (lock.getInterval().contains(tombstoneInterval)) {


Shouldn't we do a data source filter here ?

Since we only fetch the locks corresponding to the ones acquired by the task, we should be good to go without filtering the locks on the data source.
I checked the usage of this in ControllerImpl and rest of the places in the ingestion code, and we aren't filtering on the data source in other places as well.

extensions-core/multi-stage-query/src/main/java/org/apache/druid/msq/exec/ControllerImpl.java

extensions-core/multi-stage-query/src/test/java/org/apache/druid/msq/exec/MSQReplaceTest.java

rohangarg · 2023-02-15T09:11:57Z

...-core/multi-stage-query/src/test/java/org/apache/druid/msq/test/MSQTestTaskActionClient.java

      return (RetType) SegmentPublishResult.ok(segments);
    } else {
      return null;
    }
  }
+
+  public Set<DataSegment> getPublishedSegments()


can we maintain this state in MSQTestBase instead? The action client can update the state on test base state. Also can the MSQTestSegmentManager be used to fetch the segments?

MSQTestSegmentManager cannot be used because we are publishing the segments from the Controller where we also compute and publish the tombstone segments, but we are generating the segments in the SegmentGeneratorFrameProcessor that takes the MSQTestSegmentManager. Since tombstones are not generated (they don't contain any data), we cannot fetch the segments (as there are none). We have to get it from the published segments.
Regarding maintaining the state in the MSQTestBase, I thought it was cleaner that we do it directly because the CalciteSelectQueryTests also use this class for MSQ, which won't utilize the published segments (so it would need to provide an extra dummy argument)

...vice/src/main/java/org/apache/druid/indexing/common/task/batch/parallel/TombstoneHelper.java

rohangarg · 2023-02-15T10:46:10Z

...vice/src/main/java/org/apache/druid/indexing/common/task/batch/parallel/TombstoneHelper.java

+          continue;
+        }
+
+        // Overlap might not be aligned with the granularity if the used interval is not aligned with the granularity


should we add the whole intervalToDrop in the set of intervals? I didn't quite understand the part with overlap interval + iterator over it - an example would be great if possible.

Updated with the comment in the code

rohangarg · 2023-02-15T11:18:29Z

.../src/test/java/org/apache/druid/indexing/common/task/batch/parallel/TombstoneHelperTest.java

+  }
+
+  @Test
+  public void tombstonesCreatedForReplaceWhenUsedIntervalsDonotAlign() throws Exception


I think these tests should either be about tombstoneIntervals or if they are about tombstoneSegments, then they should check the segments as well

Renamed the tests to specify that we are testing intervals only (since that's the valuable and variable part in the generated segments).

cryptoe

LGTM!!
Will merge POST CICD is green.
Thanks @LakshSingla for this contribution.
One thing to mention in the release notes is that we can only allow downgrades till tombstones were introduced post this change.

LakshSingla added 2 commits January 24, 2023 11:48

tombstone genration

8689338

tests update, documentation update

c945549

clintropolis added Area - Ingestion Area - MSQ For multi stage queries - https://github.com/apache/druid/issues/12262 and removed Area - Ingestion labels Jan 26, 2023

replace tombstone for used intervals only

0c168a1

rohangarg reviewed Feb 1, 2023

View reviewed changes

LakshSingla added 4 commits February 2, 2023 15:21

refactor to use TombstoneHelper

8850493

refactor

bc1cf47

add tests, refactor

54367d5

remove MSQTombstoneHelper

ed3d5f8

github-advanced-security bot found potential problems Feb 6, 2023

View reviewed changes

...vice/src/main/java/org/apache/druid/indexing/common/task/batch/parallel/TombstoneHelper.java Fixed Show fixed Hide fixed

LakshSingla added 2 commits February 6, 2023 10:13

make updates to the replace tests

8a202a1

codeql

866269b

rohangarg reviewed Feb 6, 2023

View reviewed changes

review comments

c64466c

cryptoe reviewed Feb 6, 2023

View reviewed changes

review comments

0b4eb15

LakshSingla requested a review from cryptoe February 7, 2023 06:07

rohangarg reviewed Feb 15, 2023

View reviewed changes

LakshSingla added 2 commits February 24, 2023 11:51

review comments

739b4ae

Merge branch 'master' into msq-tombstone

99b606b

github-actions bot added the Area - Documentation label Feb 24, 2023

LakshSingla added 3 commits February 28, 2023 08:05

Merge branch 'master' into msq-tombstone

e494445

branch coverage

f8b6163

fix test for null compat

5e4d001

cryptoe approved these changes Feb 28, 2023

View reviewed changes

cryptoe merged commit ca68fd9 into apache:master Mar 1, 2023

gianm mentioned this pull request Mar 7, 2023

Add warning comments to Granularity.getIterable. #13888

Merged

LakshSingla mentioned this pull request Mar 7, 2023

Fix for OOM in the Tombstone generating logic in MSQ #13893

Merged

9 tasks

clintropolis added this to the 26.0 milestone Apr 10, 2023

techdocsmith mentioned this pull request Apr 12, 2023

[DRAFT] 26.0.0 release notes #14064

Closed

gianm mentioned this pull request Oct 25, 2023

MSQ generates tombstones honoring granularity specified in a REPLACE query. #15243

Merged

10 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Generate tombstones when running MSQ's replace #13706

Generate tombstones when running MSQ's replace #13706

LakshSingla commented Jan 24, 2023 •

edited

Loading

LakshSingla commented Jan 31, 2023 •

edited

Loading

rohangarg Feb 1, 2023

LakshSingla Feb 2, 2023

rohangarg left a comment

cryptoe Feb 6, 2023

LakshSingla Feb 7, 2023

cryptoe Feb 6, 2023

LakshSingla Feb 7, 2023

cryptoe Feb 6, 2023

LakshSingla Feb 7, 2023

cryptoe Feb 6, 2023

LakshSingla Feb 7, 2023

cryptoe Feb 6, 2023

LakshSingla Feb 7, 2023

rohangarg Feb 15, 2023

LakshSingla Feb 24, 2023

rohangarg Feb 15, 2023

LakshSingla Feb 24, 2023

rohangarg Feb 15, 2023

LakshSingla Feb 24, 2023

cryptoe left a comment


		public MSQTestTaskActionClient(ObjectMapper mapper)
		public MSQTestTaskActionClient(

Generate tombstones when running MSQ's replace #13706

Generate tombstones when running MSQ's replace #13706

Conversation

LakshSingla commented Jan 24, 2023 • edited Loading

Description

Release note

Key changed/added classes in this PR

LakshSingla commented Jan 31, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rohangarg left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cryptoe left a comment

Choose a reason for hiding this comment

LakshSingla commented Jan 24, 2023 •

edited

Loading

LakshSingla commented Jan 31, 2023 •

edited

Loading